Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ProxyManagerBridge] Polyfill for unmaintained version #32992

Merged

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Aug 6, 2019

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

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 6, 2019
@chalasr chalasr changed the title [ProxyManbagerBridge] Polyfill for unmaintained version [ProxyManagerBridge] Polyfill for unmaintained version Aug 6, 2019
@jderusse jderusse force-pushed the polyfill-ProxiedMethodReturnExpression branch from ef62893 to b98bbb3 Compare August 6, 2019 15:37
@jderusse jderusse force-pushed the polyfill-ProxiedMethodReturnExpression branch 6 times, most recently from bbe865e to 51d7c0c Compare August 7, 2019 08:16
@nicolas-grekas nicolas-grekas force-pushed the polyfill-ProxiedMethodReturnExpression branch from 51d7c0c to 33f722d Compare August 7, 2019 08:26
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 33f722d into symfony:3.4 Aug 7, 2019
nicolas-grekas added a commit that referenced this pull request Aug 7, 2019
…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
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 7, 2019
23 tasks
@jderusse jderusse deleted the polyfill-ProxiedMethodReturnExpression branch August 8, 2019 11:36
@@ -25,6 +25,7 @@
},
"autoload": {
"psr-4": { "Symfony\\Bridge\\ProxyManager\\": "" },
"classmap": [ "Legacy/ProxiedMethodReturnExpression.php" ],
Copy link
Contributor

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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. 👍

nicolas-grekas added a commit that referenced this pull request Aug 15, 2019
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
@fabpot fabpot mentioned this pull request Aug 26, 2019

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

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 ?

Copy link
Member

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
Copy link
Contributor

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

@stof
Copy link
Member

stof commented Aug 28, 2019

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
Copy link
Contributor

Ocramius commented Aug 28, 2019

@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
Copy link
Contributor

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

@jderusse
Copy link
Member Author

then, please, merge Ocramius/ProxyManager#484

@Ocramius
Copy link
Contributor

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
Copy link
Member Author

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

@nicolas-grekas
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

stof commented Aug 28, 2019

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
Copy link
Contributor

Ocramius commented Aug 28, 2019

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

sstok commented Aug 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 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
Copy link
Member Author

jderusse commented Aug 28, 2019

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

@stof
Copy link
Member

stof commented Aug 28, 2019

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

@Ocramius
Copy link
Contributor

I can most certainly fix ranges that way 👍

@jderusse
Copy link
Member Author

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
Copy link
Contributor

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

@mindplay-dk
Copy link

mindplay-dk commented Aug 29, 2019

@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
Copy link
Contributor

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
Copy link
Member

Fixed in #33382, this is not needed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants