New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FixtureBundle] Add autoconfiguration for instance of FixtureInterface #9789

Merged
merged 1 commit into from Oct 6, 2018

Conversation

Projects
None yet
3 participants
@adrienlucas
Contributor

adrienlucas commented Oct 6, 2018

Q A
Bug fix? no
New feature? kindda
BC breaks? no
Deprecations? no
Related tickets
License MIT

Improve DX by bringing autoconfiguration to services implementing the Sylius\Bundle\FixturesBundle\Fixture\FixtureInterface. This allow developper to omit the sylius_fixtures.fixture tag on their service definitions (and even omit the whole definition if autowiring is on).

I would suggest to put the tag value in a constant of the FixtureRegistryPass, seems like a good idea to you ?

@adrienlucas adrienlucas requested a review from Sylius/core-team as a code owner Oct 6, 2018

@lchrusciel

This comment has been minimized.

Show comment
Hide comment
@lchrusciel

lchrusciel Oct 6, 2018

Member

Good idea with the const, but the same should be done with the listener tag I suppose. Maybe we should create a FixturesTag class, which would be final with a private constructor, and put both constants there?

Member

lchrusciel commented Oct 6, 2018

Good idea with the const, but the same should be done with the listener tag I suppose. Maybe we should create a FixturesTag class, which would be final with a private constructor, and put both constants there?

@adrienlucas

This comment has been minimized.

Show comment
Hide comment
@adrienlucas

adrienlucas Oct 6, 2018

Contributor

About the const : I think we should have each tag const in the corresponding compiler pass, in order to keep SPR. Anyway, let me know if you want a new class to be created here :)

Contributor

adrienlucas commented Oct 6, 2018

About the const : I think we should have each tag const in the corresponding compiler pass, in order to keep SPR. Anyway, let me know if you want a new class to be created here :)

@lchrusciel

This comment has been minimized.

Show comment
Hide comment
@lchrusciel

lchrusciel Oct 6, 2018

Member

That makes sense, would you like to add them as a part of this pr?

Member

lchrusciel commented Oct 6, 2018

That makes sense, would you like to add them as a part of this pr?

@adrienlucas

This comment has been minimized.

Show comment
Hide comment
@adrienlucas

adrienlucas Oct 6, 2018

Contributor

@lchrusciel done for the fixtures and the listeners

as it makes the PR really bigger, let me know if you want the const issue to be addressed in an other one.

Contributor

adrienlucas commented Oct 6, 2018

@lchrusciel done for the fixtures and the listeners

as it makes the PR really bigger, let me know if you want the const issue to be addressed in an other one.

@pamil

pamil approved these changes Oct 6, 2018

@lchrusciel

I have nothing else to add :)

@lchrusciel

I have nothing else to add :)

@lchrusciel lchrusciel merged commit 298b77b into Sylius:master Oct 6, 2018

2 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment