Skip to content
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

Flip back service ids and FQCN #551

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Jan 3, 2023

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

Replace #549

Fix the BC-break from #498

@loic425 loic425 requested a review from a team as a code owner January 3, 2023 13:59
@loic425
Copy link
Member Author

loic425 commented Jan 3, 2023

@mbabker During internal discussions, we suggest to duplicate the services (both with IDs and FQCN) and adding deprecations to the IDs ones. WDYT?

@loic425
Copy link
Member Author

loic425 commented Jan 3, 2023

@mbabker Exactly what I did here for only 2 services for now #549

@mbabker
Copy link
Contributor

mbabker commented Jan 3, 2023

I think that'd be a little overkill. At that point, you have two independent services and an asinine number of logged container deprecations that you can't do anything about.

The closest that Symfony suggests to a naming convention for service IDs in bundles in the best practices is prefixing service IDs. That to me implies that it's better for bundles to not use FQCN's as service IDs, and to purposefully create aliases for the FQCN of interfaces that a service defines where it's feasible.

@loic425 loic425 force-pushed the fix/flip-back-service-ids-fqcn branch from 797ec38 to 947dc82 Compare January 3, 2023 16:06
@GSadee GSadee added the Bug Confirmed bugs or bugfixes. label Jan 4, 2023
@GSadee GSadee merged commit 91cf04b into Sylius:1.10 Jan 4, 2023
@GSadee
Copy link
Member

GSadee commented Jan 4, 2023

Thank you, Loïc! 🥇

@loic425 loic425 deleted the fix/flip-back-service-ids-fqcn branch January 4, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants