8000 [DI] Do not process bindings in AbstractRecursivePass by chalasr · Pull Request #24602 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DI] Do not process bindings in AbstractRecursivePass #24602

New issue 8000

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2017
Merged

[DI] Do not process bindings in AbstractRecursivePass #24602

merged 1 commit into from
Oct 18, 2017

Conversation

chalasr
Copy link
Member
@chalasr chalasr commented Oct 18, 2017
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24559
License MIT
Doc PR n/a

@chalasr chalasr added this to the 3.4 milestone Oct 18, 2017
@chalasr chalasr mentioned this pull request Oct 18, 2017
Closed
@chalasr
Copy link
Member Author
chalasr commented Oct 18, 2017

Build failure unrelated.

@dmaicher
Copy link
Contributor

Does this mean that in the example mentioned in #24559 the inlined bar service is not removed anymore within RemoveUnusedDefinitionsPass with this fix?

@chalasr
Copy link
Member Author
chalasr commented Oct 18, 2017

@dmaicher yes

@dmaicher
Copy link
Contributor

👍 Should it not be merged into 3.4 instead of master?

@chalasr chalasr changed the base branch from master to 3.4 October 18, 2017 10:01
@chalasr chalasr force-pushed the add-bindings-to-object-graph branch from 79c4875 to 98cb64f Compare October 18, 2017 10:36
@stof
Copy link
Member
stof commented Oct 18, 2017

Are you sure about it ? Bindings are used by the autowiring layer, which runs before the removal passes. If the autowiring has not used the binding to set a reference, we don't have a usage (and we don't need to prevent removal AFAIK). And if the binding was used to fill an argument, we will have a reference in the arguments (and so processed by the existing code)

@dmaicher
Copy link
Contributor

And if the binding was used to fill an argument, we will have a reference in the arguments (and so processed by the existing code)

@stof if the service was inlined this it not true I believe. It will have an inlined Definition in the arguments and not a Reference.

@stof
Copy link
Member
stof commented Oct 18, 2017

@dmaicher and this is fine as well, as the argument is still filled properly. It does not need the binding anymore.

Actually, I think the issue is that AbstractRecursivePass is always processing bindings in the exact same way it processes arguments, method calls and so on.
And so, it will treat any reference in a binding as a usage of the service. But once bindings are used to fill arguments, they become useless. So they should not be visited by the validator passes checking for circular references, invalid references and so on.
I'm not even sure there is a single case where a recursive pass should visit them.

@dmaicher
Copy link
Contributor

@stof ok I see 😉 Then we should indeed not check the References contained in bindings I guess. So the Definitions can be removed correctly for inlined services.

@chalasr chalasr force-pushed the add-bindings-to-object-graph branch from 98cb64f to d86f126 Compare October 18, 2017 10:55
@chalasr chalasr changed the title [DI] Detect references from bound arguments when populating the object graph [DI] Do not process bindings in AbstractRecursivePass Oct 18, 2017
@chalasr chalasr force-pushed the add-bindings-to-object-graph branch 2 times, most recently from 72ffcb1 to b175419 Compare October 18, 2017 11:07
@chalasr
Copy link
Member Author
chalasr commented Oct 18, 2017

PR updated to not process bindings in AbstractRecursivePass.

dmaicher
dmaicher reviewed Oct 18, 2017
View reviewed changes
src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php
@@ -67,7 +67,6 @@ protected function processValue($value, $isRoot = false)
$value->setArguments($this->processValue($value->getArguments()));
$value->setProperties($this->processValue($value->getProperties()));
$value->setMethodCalls($this->processValue($value->getMethodCalls()));
$value->setBindings($this->processValue($value->getBindings()));
Copy link
Contributor
@dmaicher dmaicher Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

can we have a test case for this? for example within the RemoveUnusedDefinitionsPassTest?

Copy link
Contributor
@dmaicher dmaicher Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I mean the test for the pass that checks if services exist ;p

==> CheckExceptionOnInvalidReferenceBehaviorPassTest

Copy link
Member Author

Choose a reason for hiding this comment

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

Test added

@chalasr chalasr force-pushed the add-bindings-to-object-graph branch 2 times, most recently from 6b2282f to 148bdec Compare October 18, 2017 12:09
dmaicher
dmaicher reviewed Oct 18, 2017
View reviewed changes
...nent/DependencyInjection/Tests/Compiler/CheckExceptionOnInvalidReferenceBehaviorPassTest.php Outdated
{
$container = new ContainerBuilder();

$container->register('a', 'stdClass');
Copy link
Contributor
@dmaicher dmaicher Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this test will now also pass without your change I believe? we should remove this line so the Reference a in the bindings does actually not exist on the container?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops :) fixed

@chalasr chalasr force-pushed the add-bindings-to-object-graph branch from 148bdec to 6a6256c Compare October 18, 2017 12:27
@fabpot
Copy link
Member
fabpot commented Oct 18, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 6a6256c into symfony:3.4 Oct 18, 2017
fabpot added a commit that referenced this pull request Oct 18, 2017
@fabpot
bug #24602 [DI] Do not process bindings in AbstractRecursivePass (cha…
79caee2 
…lasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Do not process bindings in AbstractRecursivePass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24559
| License       | MIT
| Doc PR        | n/a

Commits
-------

6a6256c Do not process bindings in AbstractRecursivePass
@chalasr chalasr deleted the add-bindings-to-object-graph branch October 18, 2017 14:17
@ogizanagi ogizanagi mentioned this pull request Nov 7, 2017
Merged
nicolas-grekas added a commit that referenced this pull request Nov 7, 2017
@nicolas-grekas
bug #24850 [DI] Fix cannot bind env var (ogizanagi)
b2edd51 
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix cannot bind env var

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see comment below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #24845 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

In #24602 we removed the processing of bindings from the `AbstractRecursivePass`. But there is actually one case where we want a recursive pass to process them: to resolve env param placeholders.

Commits
-------

f8f3a15 [DI] Fix cannot bind env var
@MatTheCat MatTheCat mentioned this pull request Nov 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dmaicher dmaicher dmaicher approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@chalasr @dmaicher @stof @fabpot @nicolas-grekas @ogizanagi @carsonbot
0