#17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features #17919
Conversation
…anager 2.x by detecting proxy features
0809f53 to
f3d41d3
Compare
|
Failures seem to be unrelated with this diff. |
| @@ -74,10 +74,16 @@ public function getProxyFactoryCode(Definition $definition, $id) | |||
| $methodName = 'get'.Container::camelize($id).'Service'; | |||
| $proxyClass = $this->getProxyClassName($definition); | |||
|
|
|||
| $generatedClass = $this->generateProxyClass($definition); | |||
There was a problem hiding this comment.
this method internally now calls getProxyClassName as well (like two lines above). This should be avoided.
There was a problem hiding this comment.
Aware: my initial implementation was using a class_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.
just pass $proxyClass as optional argument to generateProxyClass. its a private method anyway.
There was a problem hiding this comment.
Not sure I follow... getProxyCode will still be called from an external scope
There was a problem hiding this comment.
It's not about getProxyCode but about getProxyClassName being called twice.
|
I suggest applying this as a bugfix in older branches, as ProxyManager 2 is the only version being officially compatible with PHP 7 AFAIK |
|
1.x works with PHP7, but doesn't provide scalar- and return- type-hinting support, heh... |
|
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 |
|
@Ocramius please also update the composer.json of the component |
|
Thanks for the reminder! |
|
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. |
|
@fabpot this is 100% a bugfix |
|
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). |
|
If otherwise they can't use scalar type hints on PHP 7, it's a bugfix to me. |
|
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! |
|
So, as 2.3 supports PHP 7, this is definitely a bug fix. @Ocramius Would you mind creating a PR on Symfony 2.3? |
|
@fabpot hmm, this code hasn't changed since the precambrian eon: can it be cherry-picked? |
|
Hmmm, no, never mind. Cherry-picking leads to conflicts. I'll try sending a new PR for 2.3 only |
|
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 :( |
|
Yeah, all those horrible |
|
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 #17919 Commits ------- 0c6400a #17676 - making the proxy instantiation compatible with ProxyManager 2.x by detecting proxy features
|
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
|
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.