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

Recommend using the FQCN as a service ID #9886

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Jun 5, 2018

In most cases, service definitions live in the same root namespace as
the class they use.
I think this rules dates from before autowiring, and made some sense at
that time because you wanted people to name their services
guzzle.http_client rather than http_client, which would have caused
collisions. Now, the possible collisions come from far less likely
scenarios where a bundle defines a service from another bundle, and can
be solved by requiring the prefix only for those. Likewise, if a bundle
needs to define several services from one class, they should use the
prefix (since they cannot use the FQCN anyway). Finally the paragraph
below recommends to create an alias from the interface/alias to the
service id. That's a risk of collision, and if it is acceptable, then
this change is acceptable too IMO.

In most cases, service definitions live in the same root namespace as
the class they use.
I think this rules dates from before autowiring, and made some sense at
that time because you wanted people to name their services
guzzle.http_client rather than http_client, which would have caused
collisions. Now, the possible collisions come from far less likely
scenarios where a bundle defines a service from another bundle, and can
be solved by requiring the prefix only for those. Likewise, if a bundle
needs to define several services from one class, they should use the
prefix (since they cannot use the FQCN anyway). Finally the paragraph
below recommends to create an alias from the interface/alias to the
service id. That's a risk of collision, and if it is acceptable, then
this change is acceptable too IMO.
@linaori
Copy link
Contributor

linaori commented Jun 6, 2018

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

For re-usable bundles I would suggest to simply use a bundle prefix (IltarHttpBundle -> iltar_http) that matches the extension prefix. That way every service id will be unique. Interface aliases can also cause this problem, so it should be used with caution. Yes I know this is not autowire friendly, but it's the most robust solution.

That said, if it's for your own library or not an external package, feel free to do as you like.

@greg0ire
Copy link
Contributor Author

greg0ire commented Jun 6, 2018

That said, if it's for your own library or not an external package, feel free to do as you like.

That's the whole point of this PR, this is by far the most common use case in my experience.

@linaori
Copy link
Contributor

linaori commented Jun 6, 2018

That said, if it's for your own library or not an external package, feel free to do as you like.

That's the whole point of this PR, this is by far the most common use case in my experience.

Is it an idea to document both use-cases? It's an edge-case, but imo worth documenting.

@greg0ire
Copy link
Contributor Author

greg0ire commented Jun 6, 2018

Maybe I should phrase it better, but yeah, I want to document both cases : owning the class or not

@juliendufresne
Copy link

To sum up in different words

Services that refers to classes defined inside the bundle should use the FQCN.
When the service refers to a class defined outside the bundle should use the bundle prefix.

But having two scenario is confusing and is not easy to remember.

@linaori
Copy link
Contributor

linaori commented Jun 6, 2018

But you have different scenarios and not doing it right, will introduce hard to trace bugs.

@greg0ire
Copy link
Contributor Author

greg0ire commented Jun 6, 2018

The second paragraph says "For public services,
:ref:aliases should be created <service-autowiring-alias> from the interface/class
-to the service id."

So if I understand this correctly, you should end up with

my_bundle.some_hopefully_meaningful_string:
    class: Vendor\MyLib\SomeNamespace\SomeClass

Vendor\MyLib\SomeNamespace\SomeClass: '@my_bundle.some_hopefully_meaningful_string'

This means that you would end up with 2 names for the same service, it also means that you still can have collisions between aliases, so it re-introduces the issue the first paragraph aims to solve, and in addition, the first name is error-prone, often simply derived from the class name, and can lead to more copy/paste mistakes IMO. This PR proposes to simply drop the less reliable of the 2 names when possible. Fewer names means fewer hard to trace bugs, I think.

Doesn't this look easier not to mess up?

Vendor\MyLib\SomeNamespace\SomeClass: ~

This would have the additional benefit to draw more attention to special cases when they occur.

@greg0ire
Copy link
Contributor Author

I talked about this with @nicolas-grekas last week, and he told me using FQCNs would pollute the debug:container --types list with things that might be irrelevant to the end user.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 25, 2018

💯 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.)

@greg0ire greg0ire closed this Jun 25, 2018
@greg0ire greg0ire deleted the fqcns_as_service_ids branch June 25, 2018 12:08
@greg0ire
Copy link
Contributor Author

greg0ire commented Jun 25, 2018

👍 I might change this PR later to add your comment as a side note

greg0ire added a commit to greg0ire/symfony-docs that referenced this pull request Jul 21, 2018
See symfony#9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.
greg0ire added a commit to greg0ire/symfony-docs that referenced this pull request Nov 6, 2018
See symfony#9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.
javiereguiluz pushed a commit that referenced this pull request Sep 23, 2019
See #9886, I left out
reasons related to overhead during compilation steps because that might
be too technical.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants