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

[DependencyInjection] check for circular refs caused by method calls #21436

Merged
merged 1 commit into from Feb 16, 2017

Conversation

Projects
None yet
5 participants
@xabbuh
Member

xabbuh commented Jan 27, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #19362
License MIT
Doc PR

Before we check for circular references, dependencies coming from method calls are not part of the dependency graph. That why the pass is not able to detect circular references like the one described in #19362 during compilation of the container.

If we add another check after all the optimisation passes have been processed, we should be able to detect these circular references too.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 27, 2017

Member

Clearly not acceptable without tests

Member

stof commented Jan 27, 2017

Clearly not acceptable without tests

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jan 27, 2017

Member

@stof Sure, I just wanted to make sure first that there is no reason I did not think of that this is a stupid solution.

Member

xabbuh commented Jan 27, 2017

@stof Sure, I just wanted to make sure first that there is no reason I did not think of that this is a stupid solution.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 27, 2017

Member

Please add a test ensuring that circular references in method calls stay allowed when inlining the services (i.e. we can instantiate both objects and then make method calls on them before leaving the get*Service method)

Member

stof commented Jan 27, 2017

Please add a test ensuring that circular references in method calls stay allowed when inlining the services (i.e. we can instantiate both objects and then make method calls on them before leaving the get*Service method)

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jan 28, 2017

Member

@stof Not sure I understand what you mean. Do you have an example of what you have in mind?

Member

xabbuh commented Jan 28, 2017

@stof Not sure I understand what you mean. Do you have an example of what you have in mind?

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jan 29, 2017

@@ -64,7 +64,6 @@
;
$container
->register('baz', 'Baz')
->addMethodCall('setFoo', array(new Reference('foo_with_inline')))

This comment has been minimized.

@stof

stof Jan 30, 2017

Member

Why removing this ?

@stof

stof Jan 30, 2017

Member

Why removing this ?

This comment has been minimized.

@xabbuh

xabbuh Jan 30, 2017

Member

Readding the method call leads to a circular reference detected by the added pass (foo_with_inline -> inlined -> baz -> foo_with_inline).

@xabbuh

xabbuh Jan 30, 2017

Member

Readding the method call leads to a circular reference detected by the added pass (foo_with_inline -> inlined -> baz -> foo_with_inline).

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 16, 2017

Member

Thank you @xabbuh.

Member

fabpot commented Feb 16, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit fe4f7ec into symfony:2.7 Feb 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 16, 2017

bug #21436 [DependencyInjection] check for circular refs caused by me…
…thod calls (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[DependencyInjection] check for circular refs caused by method calls

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19362
| License       | MIT
| Doc PR        |

Before we check for circular references, dependencies coming from method calls are not part of the dependency graph. That why the pass is not able to detect circular references like the one described in #19362 during compilation of the container.

If we add another check after all the optimisation passes have been processed, we should be able to detect these circular references too.

Commits
-------

fe4f7ec check for circular refs caused by method calls
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 16, 2017

Member

@xabbuh Can you have a look at the master branch as it breaks container29.php, which is legitimate. So, maybe, it's more complex than this PR.

Member

fabpot commented Feb 16, 2017

@xabbuh Can you have a look at the master branch as it breaks container29.php, which is legitimate. So, maybe, it's more complex than this PR.

@xabbuh xabbuh deleted the xabbuh:issue-19362 branch Feb 16, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 16, 2017

Member

I am looking into it.

Member

xabbuh commented Feb 16, 2017

I am looking into it.

@fabpot fabpot referenced this pull request Feb 16, 2017

Closed

Infinite loop in services #21632

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 16, 2017

Member

@xabbuh What about reverting it for now?

Member

fabpot commented Feb 16, 2017

@xabbuh What about reverting it for now?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 16, 2017

Member

@fabpot Fine for me. I try to fix the tests tomorrow and can resubmit this PR then if you agree.

Member

xabbuh commented Feb 16, 2017

@fabpot Fine for me. I try to fix the tests tomorrow and can resubmit this PR then if you agree.

fabpot added a commit that referenced this pull request Feb 16, 2017

Revert "bug #21436 [DependencyInjection] check for circular refs caus…
…ed by method calls (xabbuh)"

This reverts commit 3441b15, reversing
changes made to d1f4cb2.
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 16, 2017

Member

ok, reverted for now, until we have a fix for the issue on master.

Member

fabpot commented Feb 16, 2017

ok, reverted for now, until we have a fix for the issue on master.

fabpot added a commit that referenced this pull request Feb 16, 2017

Merge branch '2.7' into 2.8
* 2.7:
  Revert "bug #21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)"
  Static code analysis with Php Inspections (EA Extended)
  [VarDumper] Added missing persistent stream cast

fabpot added a commit that referenced this pull request Feb 16, 2017

Merge branch '2.8' into 3.2
* 2.8:
  Revert "bug #21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)"
  Static code analysis with Php Inspections (EA Extended)
  [VarDumper] Added missing persistent stream cast

fabpot added a commit that referenced this pull request Feb 16, 2017

Merge branch '3.2'
* 3.2:
  Revert "bug #21436 [DependencyInjection] check for circular refs caused by method calls (xabbuh)"
  Static code analysis with Php Inspections (EA Extended)
  [VarDumper] Added missing persistent stream cast
  remove unused translation file
  reverted usage of isNan

@fabpot fabpot referenced this pull request Feb 17, 2017

Merged

Release v3.2.4 #21640

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 17, 2017

Member

Resubmitted in #21642 (I opened the PR against master first to prove that checks now pass with the changes from #21639, see https://travis-ci.org/symfony/symfony/builds/202523603).

Member

xabbuh commented Feb 17, 2017

Resubmitted in #21642 (I opened the PR against master first to prove that checks now pass with the changes from #21639, see https://travis-ci.org/symfony/symfony/builds/202523603).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 17, 2017

Member

@fabpot this PR has been reverted, but the revert is not part of the release. Should we do another set of releases including it rather than waiting 1 month with releases breaking existing projects ?

Member

stof commented Feb 17, 2017

@fabpot this PR has been reverted, but the revert is not part of the release. Should we do another set of releases including it rather than waiting 1 month with releases breaking existing projects ?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 17, 2017

Member

@stof 68d6415 is part of the 3.2.4 release.

Member

xabbuh commented Feb 17, 2017

@stof 68d6415 is part of the 3.2.4 release.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 17, 2017

Member

The changelog is misleading as it doesn't consider the revert.

Member

xabbuh commented Feb 17, 2017

The changelog is misleading as it doesn't consider the revert.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 17, 2017

Member

well, this explains it.

Member

stof commented Feb 17, 2017

well, this explains it.

This was referenced Mar 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment