Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
#17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features#17919
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…anager 2.x by detecting proxy features
0809f53 tof3d41d3CompareOcramius commentedFeb 24, 2016
Failures seem to be unrelated with this diff. |
| $methodName ='get'.Container::camelize($id).'Service'; | ||
| $proxyClass =$this->getProxyClassName($definition); | ||
| $generatedClass =$this->generateProxyClass($definition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this method internally now callsgetProxyClassName as well (like two lines above). This should be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Aware: my initial implementation was using aclass_exists() check, but that leads to compatibility hell later on (if I move the class, which is really internal anyway).
Since this is at dump-time only, it is not a big problem anyway. No I/O is happening due to this repeated call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just pass$proxyClass as optional argument togenerateProxyClass. its a private method anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure I follow...getProxyCode will still be called from an external scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not about getProxyCode but aboutgetProxyClassName being called twice.
stof commentedFeb 25, 2016
I suggest applying this as a bugfix in older branches, as ProxyManager 2 is the only version being officially compatible with PHP 7 AFAIK |
Ocramius commentedFeb 25, 2016
1.x works with PHP7, but doesn't provide scalar- and return- type-hinting support, heh... |
stof commentedFeb 25, 2016
OK, then that's OK to include this feature in Symfony 3.1. It will be an incentive for people to upgrade their projects to Symfony 3 if they want to use lazy services together with PHP 7 features |
stof commentedFeb 25, 2016
@Ocramius please also update the composer.json of the component |
Ocramius commentedFeb 25, 2016
Thanks for the reminder! |
fabpot commentedFeb 26, 2016
Is it really a feature? Looks a bug fix to me. And in the linked ticket, some people seems to have this issue on 3.0 as well. |
Ocramius commentedFeb 26, 2016
@fabpot this is 100% a bugfix |
stof commentedFeb 26, 2016
Well, people have this issue because they install ProxyManager v2, while Symfony 3.0 does not support it. The question is whether this support is considered as a bug fix (and so should go in 2.3) or as a new feature (and so should stay in 3.1). |
xabbuh commentedFeb 28, 2016
If otherwise they can't use scalar type hints on PHP 7, it's a bugfix to me. |
robfrawley commentedFeb 28, 2016
Please merge this as a bug fix in the currently supported releases. @stof,@fabpot: While I'm aware only 0.5 and 1.0 Proxy Manager releases are endorsed in the documentation, I assert support for 2.x should be treated as a bugfix as the previous releases never reached parity with many PHP 5.6.x/7.x language features (for example, variadics, scalar types hints, etc) while the 2.0 release is receiving these improvements (can you correct me if I am wrong in this regard@Ocramius). Also, the 2.x code base is beautiful --- much appreciation for the amazing work by@Ocramius and other contributors to ProxyManager/zend-code/etc! |
fabpot commentedFeb 28, 2016
So, as 2.3 supports PHP 7, this is definitely a bug fix.@Ocramius Would you mind creating a PR on Symfony 2.3? |
Ocramius commentedFeb 28, 2016
@fabpot hmm, this code hasn't changed since the precambrian eon: can it be cherry-picked? |
Ocramius commentedFeb 28, 2016
Hmmm, no, never mind. Cherry-picking leads to conflicts. I'll try sending a new PR for 2.3 only |
fabpot commentedFeb 28, 2016
Resolving the conflicts should be relatively easy. I've done most of the work but then, you need to update the tests, and I gave up :( |
Ocramius commentedFeb 28, 2016
Yeah, all those horrible |
Ocramius commentedFeb 28, 2016
PR for 2.3 @#17947 |
This PR was merged into the 2.3 branch.Discussion----------Fix -#17676 (backport#17919 to 2.3)| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#17676| License | MIT| Doc PR |This is a backport of#17919Commits-------0c6400a#17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features
fabpot commentedFeb 28, 2016
Thank you@Ocramius. |
…oxyManager 2.x by detecting proxy features (Ocramius)This PR was submitted for the master branch but it was merged into the 3.0 branch instead (closes#17919).Discussion----------#17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#17676| License | MIT| Doc PR |Note: tests don't pass because the test asset being used does not respect the return type hints (PHP7) required for ProxyManager v2. Not sure if I should just hack around it, and use the PHP7 hints.Commits-------a8f1a10#17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features
robfrawley commentedFeb 28, 2016
Works perfect on Symfony 2.8.3 and 3.0.3 against PHP 7.0.3, including a few services that previously could not be lazy loaded against Proxy Manager <2.0.0 due to variadic methods and other new syntax challenges. As always, thanks for the amazing work@Ocramius and@fabpot and crazy quick turnaround on this! |
Note: tests don't pass because the test asset being used does not respect the return type hints (PHP7) required for ProxyManager v2. Not sure if I should just hack around it, and use the PHP7 hints.