search

craue / CraueFormFlowBundle

Multi-step forms for your Symfony project.
MIT License
736 stars 118 forks source link

add autoconfiguration support #333

Closed craue closed 5 years ago

craue commented 5 years ago

With this, it's possible to enable autoconfiguration for your flow service definitions instead of specifying the parent service.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.002%) to 99.686% when pulling a5ad53905aab8a1c03668d7c3573075e0f63ceb5 on autoconfiguration into aa3a30af6be2349ba19fe05fa2ed7a49bcdc1922 on master.

artoroz commented 5 years ago

Everything seems to working fine when using Dependency Injection and AbstractController.

I have set autowire and autoconfigure turned on, this means i do not have to specify all flows in my services.yaml

gnutix commented 5 years ago

@craue what's needed for this PR to get merged?

craue commented 5 years ago

Feedback, @gnutix. 😏 Wanna make sure it's working as expected.

gnutix commented 5 years ago

@craue I tried this branch on a project of mine. The setParent() call in this PR does not work ; it only exists on the ChildDefinition class (and findDefinition returns a Definition instance).

I've pushed https://github.com/craue/CraueFormFlowBundle/pull/337 and the tests pass locally (PHP 7.3 / Symfony 4.3.0-BETA2). I'm not sure the addTag call still is useful, except to make it clearer to developers that their Flow classes were "detected, tagged and modified" by Symfony's DI.

One question: can the service craue.form.flow be used "directly" ? or was it only used as the "parent" of other services ? If so, shouldn't it be "abstract" ?

craue commented 5 years ago

@gnutix, I've rebased the branch to be in sync with master.

@craue I tried this branch on a project of mine. The setParent() call in this PR does not work ; it only exists on the ChildDefinition class (and findDefinition returns a Definition instance).

I just took the setParent approach to avoid adding all method calls (defined for the craue.form.flow service) on the found Definitions as well. I wasn't aware of the getMethodCalls/setMethodCalls methods. I guess using them is cleaner then. 😄 Thanks for the tip. I've added the change as a separate commit for now. (Will squash commits before merging.)

I've pushed #337 and the tests pass locally (PHP 7.3 / Symfony 4.3.0-BETA2). I'm not sure the addTag call still is useful, except to make it clearer to developers that their Flow classes were "detected, tagged and modified" by Symfony's DI.

Afaik, the tag needs to be added in load first to be able to find all tagged services in process later. No?

One question: can the service craue.form.flow be used "directly" ? or was it only used as the "parent" of other services ? If so, shouldn't it be "abstract" ?

Basically, yes, it should be abstract. Not sure why it wasn't from the beginning or if there's any drawback if we change this now.

gnutix commented 5 years ago

@craue I've tried the code from your latest commit and it works in my project. :) 'guess the abstract is just a clean-up / better DX, not related to this issue. Maybe for a next version ?

craue commented 5 years ago

Version 3.2.0 is tagged now. Thanks for test-driving these changes, @gnutix.

craue commented 5 years ago

@gnutix, yeah, it could (and probably should) be made abstract for the next major version (in the far future) because it could be considered as BC break.