Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@jderusse
Copy link
Member

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#32844
LicenseMIT
Doc PRNA

The current implementation of proxy-manager triggers a PHP 7.4 deprecationReflectionType::__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 ofProxiedMethodReturnExpression for version prior to 2.5

@nicolas-grekasnicolas-grekas added this to the3.4 milestoneAug 6, 2019
@chalasrchalasr changed the title[ProxyManbagerBridge] Polyfill for unmaintained version[ProxyManagerBridge] Polyfill for unmaintained versionAug 6, 2019
@jderussejderusseforce-pushed thepolyfill-ProxiedMethodReturnExpression branch fromef62893 tob98bbb3CompareAugust 6, 2019 15:37
@jderussejderusseforce-pushed thepolyfill-ProxiedMethodReturnExpression branch 6 times, most recently frombbe865e to51d7c0cCompareAugust 7, 2019 08:16
@nicolas-grekasnicolas-grekasforce-pushed thepolyfill-ProxiedMethodReturnExpression branch from51d7c0c to33f722dCompareAugust 7, 2019 08:26
@nicolas-grekas
Copy link
Member

Thank you@jderusse.

@nicolas-grekasnicolas-grekas merged commit33f722d intosymfony:3.4Aug 7, 2019
nicolas-grekas added a commit that referenced this pull requestAug 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        | 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
@nicolas-grekasnicolas-grekas mentioned this pull requestAug 7, 2019
23 tasks
@jderussejderusse deleted the polyfill-ProxiedMethodReturnExpression branchAugust 8, 2019 11:36
},
"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 ?

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 requestAug 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#32992Commits-------31f9208 [ProxyManager] fix closure binding
@fabpotfabpot mentioned this pull requestAug 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 ?

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

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 commentedAug 28, 2019
edited
Loading

@stof2.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 onecomposer.lock for multiple environments: that's an upfront thought mistake.

@jderusse
Copy link
MemberAuthor

then, please, mergeOcramius/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
MemberAuthor

nothing prevents <2.5 to be installed with 7.4. Running symfony's test suite withprefer-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 considerOcramius/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 below2.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 oncomposer.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

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 commentedAug 28, 2019
edited
Loading

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 for7.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 inroave/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.

soukicz, slash3b, atierant, simPod, Slamdunk, sspat, Taluu, and teohhanhui reacted with thumbs up emojinicolas-grekas reacted with thumbs down emoji

@Ocramius
Copy link
Contributor

Even easier: add"php": "<7.4" tosymfony/symfony on3.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.

atierant, simPod, and teohhanhui reacted with thumbs up emojinicolas-grekas and shyim reacted with thumbs down emoji

@sstok
Copy link
Contributor

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 theocramius/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.

Ocramius, Taluu, mablae, and DavidBadura reacted with thumbs up emoji

@jderusse
Copy link
MemberAuthor

jderusse commentedAug 28, 2019
edited
Loading

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

The root issue isocramius/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.

gmponos and sstok reacted with thumbs up emojigmponos reacted with eyes emoji

@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".

sstok reacted with laugh emoji

@jderusse
Copy link
MemberAuthor

what's about tagging a2.2.4 (or the lowest maintained minor version) with the bug fix updating the composer.json withphp: <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 get2.3.0 and2.4.0, which aren't containing those fixes either ;-)

@stof
Copy link
Member

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 reacted with heart emoji

@jderusse
Copy link
MemberAuthor

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 commentedAug 29, 2019
edited
Loading

@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 resolve2.4.0 even if I tagged2.4.1 with a PHP version restriction.

This is similar toroave/security-advisories and the reason onlydev-master exists there

@nicolas-grekas
Copy link
Member

Fixed in#33382, this is not needed anymore.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof left review comments

+1 more reviewer

@TaluuTaluuTaluu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

9 participants

@jderusse@nicolas-grekas@Ocramius@stof@sstok@mindplay-dk@Taluu@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp