Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form][PhpUnitBridge] Remove usage of noopReflectionProperty::setAccessible()#61207
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ReflectionProperty::setAccessible()ReflectionProperty::setAccessible()ReflectionProperty::setAccessible()ReflectionProperty::setAccessible()
|
alexandre-daubois commentedJul 23, 2025 • 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.
Refactors like this one generally target the feature branch (7.4 at this time). Only bug fixes are pushed to 7.3 (and below), so given And indeed, you can safely ignore fabbot recommendations and Windows failures, which are not related to your pull request. |
Hey Alexandre, thanks for the input! Before I change the target branch, here my rational why I believe it should be in 7.3. ThisRFC (vote pending) seeks to deprecate the usage in PHP 8.5. If the vote goes through, what I believe based on the internals list discussion, it would unneccessarily spam depcrecation warnings. I agree, it's not a bug, but I think it's a worthy "fix" to have in all supported versions. Wdyt? |
alexandre-daubois commentedJul 23, 2025 • 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.
Ah yes, I missed this one. Makes sense then 👍 If these lines are also present in earlier versions, it might be a good idea to even target 6.4, so it is also compatible with upcoming 8.5 and doesn't trigger deprecations on release. |
NickSdot commentedJul 23, 2025 • 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.
Done. 6.4 had a few more. Updated and target branch changed! Edit: I am a bit confused about the test suite here. 😅 Again not related? If there is anything I must do, please let me know. |
e3fdd2b to5f69f0fCompareThank you@NickSdot. |
4af9890 intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
| privatefunctionaddCoversForClassToAnnotationCache(Test$test,array$covers):void | ||
| { | ||
| $r =new \ReflectionProperty(TestUtil::class,'annotationCache'); | ||
| $r->setAccessible(true); |
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.
I think we need to revert the changes made to the PHPUnit bridge as it supports PHP 7.
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.
Why does/would the PhpUnit bridge in Symfony 6.4, which requires PHP 8.1, need to support PHP 7?
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.
We have to indeed, my bad for missing that!
The bridge on 7.4 is the one that's bumped to PHP >= 8.1
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.
So back to where we been yesterday? I target 7.4 for the full changes, and 6.4 for everything but bridge?
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.
… PHP < 8.1 (W0rma)This PR was merged into the 6.4 branch.Discussion----------[PhpUnitBridge] Call Reflection*::setAccessible() only for PHP < 8.1| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | no| New feature? | no| Deprecations? | no| Issues || License | MITFollow-up of#61207There are plans to deprecate `Reflection*::setAccessible()` in PHP 8.5:https://wiki.php.net/rfc/deprecations_php_8_5#extreflection_deprecationsThus, it might be beneficial to only call it if PHP < 8.1 is used.Commits-------9e89dd3 Reflection*::setAccessible() has no effect as of PHP 8.1
Uh oh!
There was an error while loading.Please reload this page.
As of PHP 8.1.0, calling this method has no effect; all properties are accessible by default. Symfony 7.x requires PHP 8.2+.
https://www.php.net/manual/en/reflectionproperty.setaccessible.php
Notes:
Component/ErrorHandler/Internal/TentativeTypes.phpis auto-generated, didn't touch it.targeted 7.3 because support for 7.2 ends in a few days.