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

Add explanation about prefix decision #10109

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

greg0ire
Copy link
Contributor

See #9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.

@javiereguiluz
Copy link
Member

I'm not sure I like this proposal 😕 for two reasons:

  • "... to not pollute ..." I do't think this is polluting. You freely decided to install a bundle and that bundle provides some services. It makes sense to me.
  • "... to avoid collisions on service IDs ...". How can a FQCN-based ID collide with other FQCN IDs? Acme\Somebundle\... will always be unique to that bundle right?

@greg0ire
Copy link
Contributor Author

greg0ire commented Jul 23, 2018

@javiereguiluz so you are in favor of #9886 ?

"... to avoid collisions on service IDs ...". How can a FQCN-based ID collide with other FQCN IDs? Acme\Somebundle... will always be unique to that bundle right?

Some bundles define services for libs that are not even in their namespaces. Quoting @iltar 👍

If 2 bundles use the same Symfony class, there would be a service ID collision which neither can solve and will cause problems.

"... to not pollute ..." I do't think this is polluting. You freely decided to install a bundle and that bundle provides some services. It makes sense to me.

Aren't most services defined in a bundle for consumption by other services in that bundle ?

@javiereguiluz
Copy link
Member

Aren't most services defined in a bundle for consumption by other services in that bundle ?

Yes ... and those are private ... and I don't care if you use FQCN IDs. or snake_case IDs. But for public services, why would they be different than standard Symfony services?

@greg0ire
Copy link
Contributor Author

I don't care if you use FQCN IDs

I think you should, because then those appear when you use bin/console debug:container --types, even if they are private (that is what the polluting part is about).

@javiereguiluz
Copy link
Member

I think you should ... because those appear ... even if they are private

Yes. I keep forgetting about that. In the past things were easy: "public" = services you can use; "private" = don't use these services, they are for internal purposes.

But now "public" and "private" are almost the same. I was thinking about "hidden" services.

However, I still don't understand why we should treat bundles differently. They are part of your application. They are like your code ... except that you didn't write it.

@greg0ire
Copy link
Contributor Author

greg0ire commented Jul 23, 2018

From @nicolas-grekas in #9886 :

bundles should keep using non-fqcn service IDs, which means no autodiscovery. They should also not use autowiring nor autoconfiguration. The reason is that by doing so, they would impose an overhead on the compilation steps (loading PHP code is slow on the CLI where OPcache can't leverage shared memory.)

I'm not 100% sure that "by doing so" means no auto{discovery,wiring,configuration} or just no auto{wiring,configuration}, but if it does, it's a good reason not to do that. Please clarify, @nicolas-grekas

@nicolas-grekas
Copy link
Member

I mean auto*: anything that relies on Reflection at some point should be avoided for bundles (they are pure band-aid anyway, which is the point, bundle authors should be encouraged to make the extra miles to be top-quality.)

See symfony#9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.
@javiereguiluz javiereguiluz changed the base branch from master to 4.3 September 23, 2019 15:52
@javiereguiluz javiereguiluz force-pushed the no_autowireable_types_pollution branch from d67faac to 305879d Compare September 23, 2019 15:52
javiereguiluz added a commit that referenced this pull request Sep 23, 2019
This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes #10109).

Discussion
----------

Add explanation about prefix decision

See #9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.

Commits
-------

305879d Add explanation about prefix decision
@javiereguiluz javiereguiluz merged commit 305879d into symfony:4.3 Sep 23, 2019
@javiereguiluz
Copy link
Member

Thanks for this contribution and sorry for the delay merging it. We finally merged it.

@greg0ire greg0ire deleted the no_autowireable_types_pollution branch September 24, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants