Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ProxyManagerBridge] Polyfill for unmaintained version#32992
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
[ProxyManagerBridge] Polyfill for unmaintained version#32992
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ef62893 tob98bbb3Comparesrc/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
bbe865e to51d7c0cCompare51d7c0c to33f722dComparenicolas-grekas commentedAug 7, 2019
Thank you@jderusse. |
…erusse)This PR was squashed before being merged into the 3.4 branch (closes#32992).Discussion----------[ProxyManagerBridge] Polyfill for unmaintained version| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#32844| License | MIT| Doc PR | NAThe current implementation of proxy-manager triggers a PHP 7.4 deprecation `ReflectionType::__toString`, and the patch won't be applied to a version prior to 2.5 (seeOcramius/ProxyManager#484) will older version of proxy-manager (2.1 to 2.4 are also compatible with php 7.4).This PR fixes the implementation of `ProxiedMethodReturnExpression` for version prior to 2.5Commits-------33f722d [ProxyManagerBridge] Polyfill for unmaintained version
| }, | ||
| "autoload": { | ||
| "psr-4": {"Symfony\\Bridge\\ProxyManager\\":"" }, | ||
| "classmap": ["Legacy/ProxiedMethodReturnExpression.php" ], |
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.
shouldn't this be ported in the main composer file ?
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.
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.
Oh right, didn't see. 👍
This PR was merged into the 3.4 branch.Discussion----------[ProxyManager] fix closure binding| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#33037| License | MIT| Doc PR | -Follows#32992Commits-------31f9208 [ProxyManager] fix closure binding
| if (null !==$classLoader) { | ||
| $classLoader->addClassMap([ProxiedMethodReturnExpression::class =>null]); | ||
| $classLoader->loadClass(ProxiedMethodReturnExpression::class); |
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.
modifying the configuration of the main composer autoloader at runtime is a very bad idea: it cancels optimizations performed by@nicolas-grekas to ensure that the whole composer config stays in shared memory (as it triggers copy-on-write on the classmap).
Also, is it compatible with the classmap-authoritative mode of composer ?
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 happens only when a proxy is dumped, which doesn't happen at runtime, so the performance consideration is not an issue actually.
Ocramius commentedAug 28, 2019
This should be reverted: just use ProxyManager 2.5, which is perfectly compatible with 2.4 and previous releases. |
stof commentedAug 28, 2019
we cannot. ProxyManager 2.5.0 requires PHP |
Ocramius commentedAug 28, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof |
Ocramius commentedAug 28, 2019
And no, you don't use one |
jderusse commentedAug 28, 2019
then, please, mergeOcramius/ProxyManager#484 |
Ocramius commentedAug 28, 2019
That's already fixed in 2.5.0, which is stable and targets 7.4. 2.2.x is only getting relevant fixes that block folks on 7.3 or lower: using 2.5.0 is perfectly viable for anybody running 7.4, and already contains relevant refactoring and fixes. |
jderusse commentedAug 28, 2019
nothing prevents <2.5 to be installed with 7.4. Running symfony's test suite with |
nicolas-grekas commentedAug 28, 2019
prefer-lowest takes the appropriate version. Please refrain from using bad/wrong/must/mistake/don't/etc words as that doesn't help. The issue is that we have conflicting versioning practices. Accepting that is the only way to solve the issue. Technically, we have no other alternative here, and although I would also considerOcramius/ProxyManager#484 is the correct way to go, I respect Marco's opinion. |
Ocramius commentedAug 28, 2019
Yes, which indeed is an actual bug: anything below Adding support for new PHP versions is a feature addition, and only goes to minor releases anyway. In general, Symfony's point of view on |
Ocramius commentedAug 28, 2019
I can probably send patches to lock the stable releases of ProxyManager onto smaller ranges of PHP versions, and then you can bump to those versions here, which should be done anyway (new releases for new software: don't expect bugfixes on old releases unless BC breaks prevent an upgrade). |
stof commentedAug 28, 2019
then, why using a semver operator for your PHP constraint if you consider than semver does not apply there ? |
Ocramius commentedAug 28, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hmm, no, I just checked, and this would simply not work: you need the newest stable dependency when working on 7.4, due to PHP breaking BC. In general, going back and retrofitting stricter dependency ranges is not possible for the grand majority of PHP components, so I suggest restricting the
Mostly legacy: I will gladly accept a patch that locks to |
nicolas-grekas commentedAug 28, 2019
That's precisely the point where we disagree: the policy of Symfony is to provide support for newer PHP versions as bug fixes. That's the case since many years and it works very well for the community, allowing smoother transitions. |
Ocramius commentedAug 28, 2019
That's cool: I disagree with that, and also with the idea of pushing this amount of overhead and reported issues to your dependencies. If you want to support new software with oldstable releases, then please do so without interfering with the package ecosystem. |
Ocramius commentedAug 28, 2019
Even easier: add I'm supposed to do it (and will do it) on my packages anyway, not sure why it shouldn't happen here too. |
sstok commentedAug 28, 2019
If I understand correctly if you use a lock file for PHP 7.3 (or 7.2) you cannot install this on PHP 7.4 due to a compatibility issue in PHP 7.4? This seems like an edge-case to me, you should ensure that your PHP version in production equals that of your development system. Especially when you use a lock file. Otherwise you can update only the |
jderusse commentedAug 28, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@sstok no, the issue is: The root issue is Ocramius/ProxyManager#492 should fix the issue and this PR may be reverted. |
Ocramius commentedAug 28, 2019
I can't (and please don't even think about doing it, if you are reading this!) re-tag anything, so a |
jderusse commentedAug 28, 2019
what's about tagging a did I missed a point? |
Ocramius commentedAug 28, 2019
Yes, very much feasible, but you'd still get |
stof commentedAug 28, 2019
well, we would need a fixed patch release for each minor version. then we can build a requirement for that. |
Ocramius commentedAug 28, 2019
I can most certainly fix ranges that way 👍 |
jderusse commentedAug 28, 2019
2.3 and 2.4 require 7.4 (https://github.com/Ocramius/ProxyManager/blob/2.3.1/composer.json) adding a constraint They should either be fixed with@nicolas-grekas patch or totally excluded. |
Ocramius commentedAug 28, 2019
They'd probably have to be excluded then, which is also fine. |
mindplay-dk commentedAug 29, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Ocramius couldn't you just tag new patch releases of both the 2.2, 2.3 and 2.4 line? Then the ^ constraint should work fine, no matter which feature line composer resolves to. |
Ocramius commentedAug 29, 2019
It wouldn't help, because the SAT mechanism would still resolve This is similar to |
nicolas-grekas commentedAug 29, 2019
Fixed in#33382, this is not needed anymore. |
The current implementation of proxy-manager triggers a PHP 7.4 deprecation
ReflectionType::__toString, and the patch won't be applied to a version prior to 2.5 (seeOcramius/ProxyManager#484) will older version of proxy-manager (2.1 to 2.4 are also compatible with php 7.4).This PR fixes the implementation of
ProxiedMethodReturnExpressionfor version prior to 2.5