[ProxyManagerBridge] Polyfill for unmaintained version #32992

jderusse · 2019-08-06T15:04:48Z

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	NA

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 (see Ocramius/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.5

src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php

src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php

nicolas-grekas · 2019-08-07T08:26:49Z

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 | NA 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 (see Ocramius/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.5 Commits ------- 33f722d [ProxyManagerBridge] Polyfill for unmaintained version

Taluu · 2019-08-08T15:06:14Z

src/Symfony/Bridge/ProxyManager/composer.json

@@ -25,6 +25,7 @@
    },
    "autoload": {
        "psr-4": { "Symfony\\Bridge\\ProxyManager\\": "" },
+        "classmap": [ "Legacy/ProxiedMethodReturnExpression.php" ],


shouldn't this be ported in the main composer file ?

https://github.com/symfony/symfony/pull/32992/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R127

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 #32992 Commits ------- 31f9208 [ProxyManager] fix closure binding

stof · 2019-08-28T09:32:35Z

src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php

+
+    if (null !== $classLoader) {
+        $classLoader->addClassMap([ProxiedMethodReturnExpression::class => null]);
+        $classLoader->loadClass(ProxiedMethodReturnExpression::class);


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 ?

This happens only when a proxy is dumped, which doesn't happen at runtime, so the performance consideration is not an issue actually.

Ocramius · 2019-08-28T09:52:08Z

This should be reverted: just use ProxyManager 2.5, which is perfectly compatible with 2.4 and previous releases.

stof · 2019-08-28T09:59:06Z

we cannot. ProxyManager 2.5.0 requires PHP ^7.4. So if your lock file needs to support both 7.3 and 7.4, you are out of luck (btw, unmaintaining all versions compatible with released PHP versions looks bad to me).

Ocramius · 2019-08-28T10:03:53Z

@stof 2.2.x is still being maintained. Besides that, you lock onto "ocramius/proxy-manager": "^2.0" (or whichever minimum you want) and that's the end of the story.

Ocramius · 2019-08-28T10:04:39Z

And no, you don't use one composer.lock for multiple environments: that's an upfront thought mistake.

jderusse · 2019-08-28T10:44:36Z

then, please, merge Ocramius/ProxyManager#484

Ocramius · 2019-08-28T10:50:39Z

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 · 2019-08-28T11:36:17Z

nothing prevents <2.5 to be installed with 7.4. Running symfony's test suite with prefer-lowest take a bad version

nicolas-grekas · 2019-08-28T12:12:53Z

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 consider Ocramius/ProxyManager#484 is the correct way to go, I respect Marco's opinion.
Sorry about the composer warning but that's a false positive.

Ocramius · 2019-08-28T12:14:05Z

nothing prevents <2.5 to be installed with 7.4. Running symfony's test suite with prefer-lowest take a bad version

Yes, which indeed is an actual bug: anything below 2.5 should have a hard lock on <7.4, but that's not fixable anymore.

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 composer.lock is nonsensical to me, if this is an official stance.

Ocramius · 2019-08-28T12:16:45Z

prefer-lowest is not applicable due to PHP not supporting SemVer, and pre-existing ProxyManager releases locking onto a too wide range of PHP releases.

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 · 2019-08-28T12:21:34Z

prefer-lowest is not applicable due to PHP not supporting SemVer, and pre-existing ProxyManager releases locking onto a too wide range of PHP releases.

then, why using a semver operator for your PHP constraint if you consider than semver does not apply there ?

Ocramius · 2019-08-28T12:24:02Z

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 "ocramius/proxy-manager" dependency supported range as much as possible, and not allowing --prefer-lowest for 7.4.

then, why using a semver operator for your PHP constraint if you consider than semver does not apply there ?

Mostly legacy: I will gladly accept a patch that locks to "php": "7.4.*" in that repository (as well as any other repo I maintain: I already do in roave/better-reflection), but it is not fixable on already released versions, and won't be fixed either.

nicolas-grekas · 2019-08-28T12:24:22Z

Adding support for new PHP versions is a feature addition, and only goes to minor releases anyway.

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 · 2019-08-28T12:30:13Z

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 · 2019-08-28T12:38:27Z

Even easier: add "php": "<7.4" to symfony/symfony on 3.4.x and you will be fine 👍

I'm supposed to do it (and will do it) on my packages anyway, not sure why it shouldn't happen here too.

sstok · 2019-08-28T13:34:05Z

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 ocramius/proxy-manager package when using a newer version? Personally I would expect more issues like these, when installing a lock for PHP 7.3 on 7.4 as many things changed.

jderusse · 2019-08-28T14:08:43Z

@sstok no, the issue is: ocramius/proxy-manager version 2.4.x may be chosen by composer will runing 7.4 no matter the .lock file
ie. when user define ocramius/proxy-manager: ~2.4.0 or when user use a dependency that have a constraint ocramius/proxy-manager: <2.5 or when symfony run tests with the lowest version of dependencies or for whatever reason.

The root issue is ocramius/proxy-manager: <2.5.0 is not compatible with php 7.4 (and will never be from the point of view of the maintainers (which I respect)) will it composer.json allows that version.

Ocramius/ProxyManager#492 should fix the issue and this PR may be reverted.

Ocramius · 2019-08-28T14:19:14Z

I can't (and please don't even think about doing it, if you are reading this!) re-tag anything, so a --prefer-lowest will always land onto something incompatible with "php": "7.4.0".

jderusse · 2019-08-28T14:46:04Z

what's about tagging a 2.2.4 (or the lowest maintained minor version) with the bug fix updating the composer.json with php: <7.4
then, symfony may use ^2.2.4 to fix this.

did I missed a point?

Ocramius · 2019-08-28T14:47:50Z

Yes, very much feasible, but you'd still get 2.3.0 and 2.4.0, which aren't containing those fixes either ;-)

stof · 2019-08-28T14:53:07Z

well, we would need a fixed patch release for each minor version. then we can build a requirement for that.

Ocramius · 2019-08-28T15:03:09Z

I can most certainly fix ranges that way 👍

jderusse · 2019-08-28T15:07:45Z

2.3 and 2.4 require 7.4 (https://github.com/Ocramius/ProxyManager/blob/2.3.1/composer.json) adding a constraint < 7.4 does not make sense.

They should either be fixed with @nicolas-grekas patch or totally excluded.

Ocramius · 2019-08-28T15:09:47Z

They'd probably have to be excluded then, which is also fine.

mindplay-dk · 2019-08-29T06:05:59Z

@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 · 2019-08-29T07:22:58Z

It wouldn't help, because the SAT mechanism would still resolve 2.4.0 even if I tagged 2.4.1 with a PHP version restriction.

This is similar to roave/security-advisories and the reason only dev-master exists there

nicolas-grekas · 2019-08-29T15:15:06Z

Fixed in #33382, this is not needed anymore.

carsonbot added Status: Needs Review Bug labels Aug 6, 2019

nicolas-grekas added this to the 3.4 milestone Aug 6, 2019

chalasr changed the title ~~[ProxyManbagerBridge] Polyfill for unmaintained version~~ [ProxyManagerBridge] Polyfill for unmaintained version Aug 6, 2019

chalasr added the ProxyManagerBridge label Aug 6, 2019

jderusse force-pushed the polyfill-ProxiedMethodReturnExpression branch from ef62893 to b98bbb3 Compare August 6, 2019 15:37

nicolas-grekas reviewed Aug 6, 2019

View reviewed changes

src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Aug 6, 2019

View reviewed changes

src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Aug 6, 2019

View reviewed changes

src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php Show resolved Hide resolved

nicolas-grekas reviewed Aug 6, 2019

View reviewed changes

src/Symfony/Bridge/ProxyManager/Legacy/ProxiedMethodReturnExpression.php Outdated Show resolved Hide resolved

jderusse force-pushed the polyfill-ProxiedMethodReturnExpression branch 6 times, most recently from bbe865e to 51d7c0c Compare August 7, 2019 08:16

nicolas-grekas approved these changes Aug 7, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 7, 2019

[ProxyManagerBridge] Polyfill for unmaintained version

33f722d

nicolas-grekas force-pushed the polyfill-ProxiedMethodReturnExpression branch from 51d7c0c to 33f722d Compare August 7, 2019 08:26

nicolas-grekas merged commit 33f722d into symfony:3.4 Aug 7, 2019

nicolas-grekas mentioned this pull request Aug 7, 2019

PHP 7.4 + PHPUnit 8.2 support #32844

Closed

23 tasks

jderusse deleted the polyfill-ProxiedMethodReturnExpression branch August 8, 2019 11:36

Taluu reviewed Aug 9, 2019

View reviewed changes

nicolas-grekas mentioned this pull request Aug 15, 2019

[ProxyManager] fix closure binding #33180

Merged

fabpot mentioned this pull request Aug 26, 2019

Release v3.4.31 #33344

Merged

stof reviewed Aug 28, 2019

View reviewed changes

stof mentioned this pull request Aug 28, 2019

Class name conflict with Symfony Bridge proxy manager and Ocramius ProxyManager #33364

Closed

Ocramius mentioned this pull request Aug 28, 2019

Lock onto PHP 7.4.x (not 7.5.x or newer) Ocramius/ProxyManager#492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ProxyManagerBridge] Polyfill for unmaintained version #32992

[ProxyManagerBridge] Polyfill for unmaintained version #32992

jderusse commented Aug 6, 2019

nicolas-grekas commented Aug 7, 2019

Taluu Aug 8, 2019

nicolas-grekas Aug 9, 2019

Taluu Aug 9, 2019

stof Aug 28, 2019

nicolas-grekas Aug 28, 2019

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019 •

edited

Loading

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

nicolas-grekas commented Aug 28, 2019

Ocramius commented Aug 28, 2019

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019 •

edited

Loading

nicolas-grekas commented Aug 28, 2019

Ocramius commented Aug 28, 2019

Ocramius commented Aug 28, 2019

sstok commented Aug 28, 2019

jderusse commented Aug 28, 2019 •

edited

Loading

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

mindplay-dk commented Aug 29, 2019 •

edited

Loading

Ocramius commented Aug 29, 2019

nicolas-grekas commented Aug 29, 2019

[ProxyManagerBridge] Polyfill for unmaintained version #32992

[ProxyManagerBridge] Polyfill for unmaintained version #32992

Conversation

jderusse commented Aug 6, 2019

nicolas-grekas commented Aug 7, 2019

Taluu Aug 8, 2019

Choose a reason for hiding this comment

nicolas-grekas Aug 9, 2019

Choose a reason for hiding this comment

Taluu Aug 9, 2019

Choose a reason for hiding this comment

stof Aug 28, 2019

Choose a reason for hiding this comment

nicolas-grekas Aug 28, 2019

Choose a reason for hiding this comment

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019 • edited Loading

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

nicolas-grekas commented Aug 28, 2019

Ocramius commented Aug 28, 2019

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019 • edited Loading

nicolas-grekas commented Aug 28, 2019

Ocramius commented Aug 28, 2019

Ocramius commented Aug 28, 2019

sstok commented Aug 28, 2019

jderusse commented Aug 28, 2019 • edited Loading

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

stof commented Aug 28, 2019

Ocramius commented Aug 28, 2019

jderusse commented Aug 28, 2019

Ocramius commented Aug 28, 2019

mindplay-dk commented Aug 29, 2019 • edited Loading

Ocramius commented Aug 29, 2019

nicolas-grekas commented Aug 29, 2019

Ocramius commented Aug 28, 2019 •

edited

Loading

Ocramius commented Aug 28, 2019 •

edited

Loading

jderusse commented Aug 28, 2019 •

edited

Loading

mindplay-dk commented Aug 29, 2019 •

edited

Loading