Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.8k
[Console] Do not call non-static method via class-name#62822
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
base:7.3
Are you sure you want to change the base?
Conversation
…static method by class-name only
carsonbot commentedDec 19, 2025
Hey! Thanks for your PR. You are targeting branch "8.1" but it seems your PR description refers to branch "7.3". Cheers! Carsonbot |
keradus commentedDec 19, 2025
🙇🏻 |
yceruto commentedDec 19, 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.
For reference, we did this intentionally for this case, where it is not possible to define a callable like
The test changed here was testing that, but it was not actually invoking the method statically. I think if we want to keep testing that feature, we should revert (the static method) and add a comment instead? |
keradus commentedDec 19, 2025
in such case, static analysis has no clue given lambda cannot be static [using if the feature is expected, it should be tested. let me revert and propose some comments |
…all non-static method by class-name only"This reverts commit7815b7e.
keradus commentedDec 19, 2025
i believe this explicit check against static->$this convertion was hidden. i elaborated on it in newly added comment and put an exception. in upcoming commit, i also suggest to not declarae exception for whole file (other lambas shall be static) and handle it differently for those 2 unique lambdas. Please re-review and decide which version you prefer (exception for whole file or workaround with |
| #[Argument(description:'Short argument description')]string$bio ='', | ||
| // Declare callback in static context, even when method is NOT static. | ||
| // PHP doesn't allow to use `$this` here, and callback is later modified | ||
| // on-the-fly to be called on instance instead: https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Console/Attribute/Argument.php#L87 |
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 add links to github in the codebase
| // on-the-fly to be called on instance instead: https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Console/Attribute/Argument.php#L87 | |
| // on-the-fly to be called on instance instead |
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.
ack
let me apply of course,
i wonder if you have a practice how to provide such reference? it was really hidden knowledge, when looking at spec-via-tests
nicolas-grekasDec 19, 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.
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.
you'll find references using eg@see TheClass in phpdoc
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.
tbh, myself, even with@see Argument::tryFrom(), i wouldn't intuitively find this behaviour, as it's not documented in that class/code and condition to apply this transformation is combined of 6 conditions 😅
Perhaps i'm too far away from this, unexpected to me, behaviour.
Perhaps, OK to add some in-code docs for this transofmation? (added last commit, feel free to revert)
…st.phpCo-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
extracted from PR#62821 against 8.1, but targeting 7.3 (as the earliest supported branch with the issue)
Non-static lambda is working right now by "accident", but we still call non-static method
getSuggestedRolesusing class name and not instance -suggestedValues: [self::class, 'getSuggestedRoles']). Yet, imaginegetSuggestedRolesmethod would start using$this...If lambda would be declared as static, calling non-static method from it would fail regardless of not using
$thisinside. This is similar issue as we faced in#62794Let's not call non-static method statically -> solution to convert method into static one.
I'm changing lambdas to static in that case as well. [it's OK they are later re-bind, as those particular lambdas not using
$thiscontext anyway]