Retro-fit proxy code to make it deterministic for older proxy manager implementations #26176
Conversation
|
|
||
| if (version_compare(Version::getVersion(), '2.2', '<')) { | ||
| $code = preg_replace( | ||
| '/((?:\$(?:this|initializer|instance)->)?(?:publicProperties|initializer|valueHolder))[0-9a-f]+/', |
There was a problem hiding this comment.
should use a possessive quantifier to avoid backtracking (as done in the previous regex)
There was a problem hiding this comment.
I had no idea what possessive quantifiers are but I hope I got what it means
| '/(\$this->initializer[0-9a-f]++) && \1->__invoke\(\$this->(valueHolder[0-9a-f]++), (.*?), \1\);/', | ||
| '$1 && ($1->__invoke(\$$2, $3, $1) || 1) && $this->$2 = \$$2;', | ||
| $this->classGenerator->generate($this->generateProxyClass($definition)) |
There was a problem hiding this comment.
you could even keep this code here
There was a problem hiding this comment.
I could but it’s less symmetric
There was a problem hiding this comment.
@nicolas-grekas is this replacement related to backporting some ProxyManager too, or would be also keep it after dropping support for ProxyManager 2.1 and older ?
If it is meant to stay, I would not try to make it symmetric with the feature backporting.
There was a problem hiding this comment.
this is Symfony-only feature, has been refused in ProxyManager (at least not merged yet), so not part of any versions of it
| if (version_compare(Version::getVersion(), '2.2', '<')) { | ||
| $code = preg_replace( | ||
| '/((?:\$(?:this|initializer|instance)->)?(?:publicProperties|initializer|valueHolder))[0-9a-f]++/', | ||
| '${1}'.$this->getSuffix($definition), |
There was a problem hiding this comment.
@nicolas-grekas no. We need the braces here, because the suffix could start with a number, which would change the placeholder when concatenating the string
| ); | ||
|
|
||
| if (version_compare(Version::getVersion(), '2.2', '<')) { |
There was a problem hiding this comment.
the class doesn't exist in the lowest versions we support, per composer.json
There was a problem hiding this comment.
and so should be changed to !\class_exists(Version::class) || version_compare(Version::getVersion(), '2.2', '<')
| @@ -122,4 +135,16 @@ private function generateProxyClass(Definition $definition) | ||
|
|
||
| return $generatedClass; | ||
| } | ||
|
|
||
| /** | ||
| * Generate unique suffix for a definition. |
There was a problem hiding this comment.
I would remove this docblock, it adds little value IMHO
| @@ -122,4 +135,14 @@ private function generateProxyClass(Definition $definition) | ||
|
|
||
| return $generatedClass; | ||
| } | ||
|
|
||
| /** | ||
| * @param Definition $definition |
There was a problem hiding this comment.
I would still suggest to drop the docblock entirely :)
There was a problem hiding this comment.
At least it provides the return value :)
There was a problem hiding this comment.
so let's keep only the return value if you want... :)
|
Ready to be merged from my POV |
|
Tests do not pass and it looks related to the changes made here. |
|
@fabpot good point, made the version check somewhat more complicated. |
|
Tests look good now |
|
Thank you @lstrojny. |
…oxy manager implementations (lstrojny) This PR was merged into the 3.4 branch. Discussion ---------- Retro-fit proxy code to make it deterministic for older proxy manager implementations Follow up on #25958 (comment) ProxyManager >= 7.2 already implements a deterministic identifier naming strategy which is critical for reproducible builds (#25958). but versions below that don’t. This is what this PR fixes. Here is more context: Ocramius/ProxyManager#411 | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 0f16056 Retro-fit proxy code to make it deterministic for older proxy manager implementations
Follow up on #25958 (comment)
ProxyManager >= 7.2 already implements a deterministic identifier naming strategy which is critical for reproducible builds (#25958). but versions below that don’t. This is what this PR fixes. Here is more context: Ocramius/ProxyManager#411