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] PhpDumper.php: hasReference() shouldn't search references in lazy service. #19902

Merged
merged 1 commit into from Dec 3, 2016

Conversation

Projects
None yet
9 participants
@antanas-arvasevicius
Contributor

antanas-arvasevicius commented Sep 10, 2016

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

more info:
#19901

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 11, 2016

Will this also solve doctrine/DoctrineBundle#562?

@lemoinem

Hello,

Thank you very much for your contribution... Could you re-insert the complete PR summary at the top of your PR. If a line seems irrelevant for your PR, just put N/A instead of removing it.

I think it would also be a nice touch to add a test for this behavior so we make sure the bug doesn't come back to haunt us later on.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 3, 2016

@antanas-arvasevicius looking at your comment here, I think we can close this PR, isn't it?

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Oct 3, 2016

Hi, no this PR fixes the issue, I'll add some test cases to this PR and
hope this will be merged into release. But why do you think that this PR
should be closed?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 3, 2016

why do you think that this PR should be closed?

Because reading your comment, you say "it's not due lazy->lazy" :)
And also because "lazy" is an optional feature that requires proxy-manager.
Or should this be done conditionally when proxy-manager is used?

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Oct 3, 2016

Yes, that doctrine bug is not due lazy->lazy. It's due this bug which was
fixed with this PR. It's due a fact that hasReference() also scans lazy
service dependencies to find out circular dependencies, why is that so
because lazy is indirectly referenced and instantiated lazily. And that
Doctrine lazy listeners has worked only by miracle because it used
Configuration as inlined service definition and thats why hasReference
hadn't detected any circular dep.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 3, 2016

So, what about:

Or should this be done conditionally when proxy-manager is used?

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Oct 3, 2016

It would be reasonable to do it conditionally because if proxy-manager
doesn't exists then everything is instantiated eagerly and we need
detection of circular references in this case. If proxy-manager is
available then we need to "skip reference search in lazy service's
arguments". What do you think?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 5, 2016

I agree :)

@fabpot

This comment has been minimized.

Member

fabpot commented Nov 10, 2016

Anything to be done before merging this one?

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 10, 2016

On Nov 10, 2016 2:26 AM, "Fabien Potencier" notifications@github.com
wrote:

Anything to be done before merging this one?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19902 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEAcnFDb7RZgYeYoDImXkxBprTf1OUu2ks5q8mTOgaJpZM4J5yIT
.

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 10, 2016

Hi, needed to add unit test with DI setup which shows correct behavior (no
circular reference error while injecting argument in lazy service which is
dependent of that service itself) e.g. graph: new A(new B(A)). If B is
marked as lazy we should allow this configuration.

On Nov 10, 2016 7:32 AM, "Antanas Arvasevicius" <
antanas.arvasevicius@gmail.com> wrote:

On Nov 10, 2016 2:26 AM, "Fabien Potencier" notifications@github.com
wrote:

Anything to be done before merging this one?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19902 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEAcnFDb7RZgYeYoDImXkxBprTf1OUu2ks5q8mTOgaJpZM4J5yIT
.

@stof

This comment has been minimized.

Member

stof commented Nov 10, 2016

if B is lazy and we can actually make it lazy (if we cannot generate the lazy proxy, it is the same than not being lazy)

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 10, 2016

Right, but what's the point to allow lazy:true without proxy mngr? Are
there any use cases for that?
Now, if you need a lazy for avoiding circular refs and proxy manager is not
there you'll get circular reference error and not an error which tells you
that you must enable proxy manager for this.

On Nov 10, 2016 12:11 PM, "Christophe Coevoet" notifications@github.com
wrote:

if B is lazy and we can actually make it lazy (if we cannot generate
the lazy proxy, it is the same than not being lazy)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19902 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEAcnF7eP7HvJbyqeC04_6KkyTVKGC3hks5q8u3rgaJpZM4J5yIT
.

@fabpot

This comment has been minimized.

Member

fabpot commented Nov 10, 2016

At least for Open-Source bundles, people might tag a service as lazy as an optimization if the end user has the proxy manager and as a noop if it's not installed.

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 11, 2016

Added unit tests, And also had found out that CheckCircularReferencesPass detects circular references. I've applied patch for that too. Please review.

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 22, 2016

@fabpot could this be merged now? Or there are any problems?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 25, 2016

rebase needed

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 28, 2016

Rebase onto what? Haven't thought that bug fix could take so long to be applied... I really do not understand how your bug fix apply policy works, but it sucks. Bye.

@HeahDude

This comment has been minimized.

Member

HeahDude commented Nov 28, 2016

A rebase on the target branch of this PR which is 2.7, since some other commits have changed some files modified in this PR, you need to resolve some conflicts so maintainers are able to merge it automatically. Thank you for contributing.

[DependencyInjection] PhpDumper.php: hasReference() should not search…
… references in lazy service arguments.
@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Nov 28, 2016

Thanks for explanation. Now rebased my branch. Is this fix will be available in 2.8 .. 3.1 also?

@HeahDude

This comment has been minimized.

Member

HeahDude commented Nov 28, 2016

Yes it will ;)

@HeahDude

This comment has been minimized.

Member

HeahDude commented Dec 3, 2016

👍

Status: Reviewed

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 3, 2016

👍 (the $isLazy temp var could be removed)

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 3, 2016

@fabpot fabpot merged commit 595a978 into symfony:2.7 Dec 3, 2016

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 Dec 3, 2016

bug #19902 [DependencyInjection] PhpDumper.php: hasReference() should…
…n't search references in lazy service. (antanas-arvasevicius)

This PR was merged into the 2.7 branch.

Discussion
----------

[DependencyInjection] PhpDumper.php: hasReference() shouldn't search references in lazy service.

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

more info:
#19901

Commits
-------

595a978 [DependencyInjection] PhpDumper.php: hasReference() should not search references in lazy service arguments.
@gxolin

This comment has been minimized.

gxolin commented Dec 5, 2016

Thank you @antanas-arvasevicius.
@fabpot do you know in which version this fix will be available for 7.2.* ?

@wouterj

This comment has been minimized.

Member

wouterj commented Dec 5, 2016

@gxolin it's always the next release. So 2.7.22

@antanas-arvasevicius

This comment has been minimized.

Contributor

antanas-arvasevicius commented Dec 5, 2016

no prob :)

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