Skip to content

Update compiler_passes.rst #9676

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

Closed
wants to merge 1 commit into from
Closed

Conversation

DonCallisto
Copy link
Contributor

@DonCallisto DonCallisto commented Apr 24, 2018

I think that findDefinition is better than getDefinition as it will also look at aliases.
I found this by myself while migrating from sf < 3.4 to sf >= 3.4.
In particular I was following this and at point 2 services managed directly by CompilerPass were never founded.

Moreover - not related directly to this PR - I'm not sure that this "split" getDefinition/findDefinition/getAlias is really useful, but maybe I don't have the bigger picture.

WDYT?

PS.: Maybe this edit could be applied not only for sf 4 but also for previous versions?

I think that `findDefinition` is better than `getDefinition` as it will also look at aliases.
I found it myself while migrating from sf < 3.4 to sf >= 3.4.
In particular I was following [this](https://symfony.com/doc/current/service_container/3.3-di-changes.html) and at point 2 services managed directly by `CompilerPass` were never founded.

Moreover - not related directly to this PR - I'm not sure that this "split" `getDefinition`/`findDefinition`/`getAlias` is really useful, but maybe I don't have the bigger picture.

WDYT?
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted! Thanks for fixing it.

@DonCallisto
Copy link
Contributor Author

@javiereguiluz Do you thing we should also point it out here?

@javiereguiluz
Copy link
Member

I'm afraid I don't understand you. Where exactly do you want to do the change? Thanks!

@DonCallisto
Copy link
Contributor Author

I would let users aware that, when changing services id(s) from dot notation to FQCN, if any of those services are handled by a compiler pass that leverage on getDefinition (that was pointed out in the old documentation so is likely that a lot of user have implemented this), they will receive an error and the solution is to change from getDefinition to findDefinition: it should be a box just to tackle this possible issue while migrating

@javiereguiluz
Copy link
Member

@DonCallisto thanks for explaining it! Let's do that change too then.

@DonCallisto
Copy link
Contributor Author

I'll do that in the next hours

DonCallisto added a commit to DonCallisto/symfony-docs that referenced this pull request Apr 27, 2018
@javiereguiluz javiereguiluz modified the milestones: 2.7, 3.4 Apr 27, 2018
@javiereguiluz
Copy link
Member

I saw that you made the other related change in a separate pull request, so let's merge this! Thank you.

javiereguiluz added a commit that referenced this pull request Apr 27, 2018
This PR was submitted for the 4.0 branch but it was merged into the 3.4 branch instead (closes #9676).

Discussion
----------

Update compiler_passes.rst

I think that `findDefinition` is better than `getDefinition` as it will also look at aliases.
I found this by myself while migrating from sf < 3.4 to sf >= 3.4.
In particular I was following [this](https://symfony.com/doc/current/service_container/3.3-di-changes.html) and at point 2 services managed directly by `CompilerPass` were never founded.

Moreover - not related directly to this PR - I'm not sure that this "split" `getDefinition`/`findDefinition`/`getAlias` is really useful, but maybe I don't have the bigger picture.

WDYT?

PS.: Maybe this edit could be applied not only for sf 4 but also for previous versions?

Commits
-------

9b26fce Update compiler_passes.rst
javiereguiluz added a commit that referenced this pull request May 21, 2018
This PR was submitted for the 4.0 branch but it was squashed and merged into the 3.4 branch instead (closes #9695).

Discussion
----------

Update 3.3-di-changes.rst

Related to #9676 (review)

Commits
-------

bebe9a0 Update 3.3-di-changes.rst
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.

3 participants