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.

Sign up for GitHub

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

Jump to bottom

[FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod #54187

Merged
merged 1 commit into from Mar 7, 2024
Merged

[FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod #54187

merged 1 commit into from Mar 7, 2024

Conversation

alexandre-daubois
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Issues Part of #54180
License MIT

This fixes:

16x: Calling ReflectionMethod::__construct() with 1 argument is deprecated, use ReflectionMethod::createFromMethodName() instead
    4x in RouterDebugCommandTest::testDumpAllRoutes from Symfony\Bundle\FrameworkBundle\Tests\Functional
    4x in RouterDebugCommandTest::testSearchMultipleRoutesWithoutInteraction from Symfony\Bundle\FrameworkBundle\Tests\Functional
    4x in RouterDebugCommandTest::testShowAliases from Symfony\Bundle\FrameworkBundle\Tests\Functional
    2x in TextDescriptorTest::testDescribeRouteWithControllerLink from Symfony\Bundle\FrameworkBundle\Tests\Console\Descriptor
    1x in RouterDebugCommandTest::testDumpOneRoute from Symfony\Bundle\FrameworkBundle\Tests\Functional
    ...

@carsonbot carsonbot added this to the 5.4 milestone Mar 7, 2024
@alexandre-daubois alexandre-daubois changed the title [FrameworkBundle] Fix PHP 8.4 deprecation [FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod Mar 7, 2024
@alexandre-daubois alexandre-daubois changed the title [FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod [FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod Mar 7, 2024
xabbuh
xabbuh requested changes Mar 7, 2024
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php Outdated
@@ -559,7 +559,7 @@ private function formatControllerLink($controller, string $anchorText, ?callable
} elseif (!\is_string($controller)) {
return $anchorText;
} elseif (str_contains($controller, '::')) {
$r = new \ReflectionMethod($controller);
$r = \PHP_VERSION_ID < 80300 ? new \ReflectionMethod($controller) : \ReflectionMethod::createFromMethodName($controller);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$r = \PHP_VERSION_ID < 80300 ? new \ReflectionMethod($controller) : \ReflectionMethod::createFromMethodName($controller);
$r = \PHP_VERSION_ID < 80400 ? new \ReflectionMethod($controller) : \ReflectionMethod::createFromMethodName($controller);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the method is available since 8.3. I'd say let's use it with the lowest version possible?

Copy link
Member

@derrabus derrabus Mar 7, 2024 •
edited

Choose a reason for hiding this comment

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

I'd say let's use it with the lowest version possible?

I agree. The earlier we can drop conditional code, the better.

Copy link
Member

Choose a reason for hiding this comment

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

we don't need any condition if we use explode, isn't it?
aren't there more occurences of this pattern in the codebase? It'd be nice to have only one PR to fix them all.

Copy link
Member

Choose a reason for hiding this comment

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

we don't need any condition if we use explode, isn't it?

That is correct, but why wouldn't we want to leverage the native method if there is one? I like the current way because it hints at how the code will look like once we drop PHP 8.2 support (8.0 probably?).

Copy link
Member

Choose a reason for hiding this comment

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

On my side I have a preference for not planning for more future PRs, preserving git blame, and saving a few possible conflicts :)

Copy link
Member

@derrabus derrabus Mar 7, 2024 •
edited

Choose a reason for hiding this comment

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

I predict that someone will make that change on 8.0 anyway. 😁

But yeah, explode or condition, both works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the future-proof opinion of your solution. I updated the PR. Also, I could not find other places to update, ReflectionMethod seems always called with two arguments 👍

derrabus
derrabus approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Nice catch!

@alexandre-daubois alexandre-daubois force-pushed the php8.4-deprec branch 2 times, most recently from 0c908d6 to bb08386 Compare March 7, 2024 12:01
alexandre-daubois
alexandre-daubois commented Mar 7, 2024
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php
@@ -559,7 +559,7 @@ private function formatControllerLink($controller, string $anchorText, ?callable
} elseif (!\is_string($controller)) {
return $anchorText;
} elseif (str_contains($controller, '::')) {
$r = new \ReflectionMethod($controller);
$r = new \ReflectionMethod(...explode('::', $controller, 2));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the limit argument idea comes from ControllerResolver::createController

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit 5bdddc2 into symfony:5.4 Mar 7, 2024
10 of 11 checks passed
This was referenced Apr 2, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@derrabus derrabus derrabus approved these changes

@GromNaN GromNaN GromNaN approved these changes

@stof stof stof approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@alexandre-daubois @nicolas-grekas @GromNaN @stof @derrabus @xabbuh @carsonbot