-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
[FrameworkBundle] Fix PHP 8.4 deprecation on ReflectionMethod
#54187
Conversation
ReflectionMethod
@@ -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); |
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.
$r = \PHP_VERSION_ID < 80300 ? new \ReflectionMethod($controller) : \ReflectionMethod::createFromMethodName($controller); | |
$r = \PHP_VERSION_ID < 80400 ? new \ReflectionMethod($controller) : \ReflectionMethod::createFromMethodName($controller); |
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.
Actually the method is available since 8.3. I'd say let's use it with the lowest version possible?
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'd say let's use it with the lowest version possible?
I agree. The earlier we can drop conditional code, the better.
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 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.
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 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?).
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.
On my side I have a preference for not planning for more future PRs, preserving git blame, and saving a few possible conflicts :)
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 predict that someone will make that change on 8.0 anyway. 😁
But yeah, explode or condition, both works for me.
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 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 👍
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.
Nice catch!
0c908d6
to
bb08386
Compare
bb08386
to
8746765
Compare
@@ -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)); |
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.
Using the limit argument idea comes from ControllerResolver::createController
Thank you @alexandre-daubois. |
This fixes: