Problem/Motivation

Code was added in #2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems
The code fails on PHP 8.4 as the cause of PHP 8.3 RFC https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

Steps to reproduce

https://3v4l.org/dQVe4/rfc

class my {
    function fu() {}
}
var_dump(new \ReflectionMethod('my::fu'));
var_dump(\ReflectionMethod::createFromMethodName('my::fu'));

Proposed resolution

As deprecation already in action and no other usages left using https://github.com/nette/di/commit/a548b1cfce285ced0f48cd046c10170349a15a3e

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3444264

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: andypost » Unassigned
Issue summary: View changes
Status: Active » Needs review
Issue tags: -PHP 8.4 +PHP 8.3
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

andypost opened merge request !7839

andypost’s picture

andypost
he/him
Primary language Russian
commented
Priority: Normal » Major
andypost’s picture

andypost
he/him
Primary language Russian
commented

Major as the RFC was already fixed for 8.3 compatibility

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: [PHP 8.4] ReflectionMethod costructor deprecated with one argument » [PHP 8.4] ReflectionMethod constructor deprecated with one argument
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward.

Should it be tagged 8.4 though?

andypost’s picture

andypost
he/him
Primary language Russian
commented

No, this change was applied since 8.3 but the pointed issue did not check that

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed aa2830b3b1 to 11.x and 40898b6120 to 10.3.x. Thanks!

  • alexpott committed 40898b61 on 10.3.x 
    Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod constructor...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Major » Normal

  • alexpott committed aa2830b3 on 11.x 
    Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod constructor...

andypost closed merge request !7839

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Needs work

Reverted this, as PHPStan complains in 10.3.x:

  Line   core/lib/Drupal/Component/Utility/ArgumentsResolver.php  
 ------ --------------------------------------------------------- 
  126    Call to an undefined static method                       
         ReflectionMethod::createFromMethodName().

Also in 11.x we don't need the PHP_VERSION_ID check, the minimum is PHP 8.3 there.

  • longwave committed 774dcb1b on 10.3.x 
    Revert "Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod...

  • longwave committed 65e7a1ef on 11.x 
    Revert "Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Patch (to be ported)

Better status I guess; it needs to be built for the branch rather than cherry-picked.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Patch (to be ported) » Needs work

Crossposted with the 11.x revert.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Bah - PHPStan should be able to work out the version check... anyhow +1 for the reverts... typically that something that seems simple turns out to not.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Maybe phpstan is not using 8.3 as minimally allowed, otherwise how it possible?

Ref the failed job https://git.drupalcode.org/project/drupal/-/jobs/1481141

andypost’s picture

andypost
he/him
Primary language Russian
commented

https://www.php.net/manual/en/reflectionmethod.createfrommethodname

since 8.3, and the RFC, probably the issue in phpstan but 11.x surely should not have condition

andypost opened merge request !7864

andypost’s picture

andypost
he/him
Primary language Russian
commented

That's a bug https://phpstan.org/r/9aff1e0d-c4d5-4d48-88fb-b0ae0ec482db

andypost opened merge request !7865

andypost’s picture

andypost
he/him
Primary language Russian
commented

reported upstream https://github.com/phpstan/phpstan/issues/10954

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 10.3.x-dev » 11.x-dev
Status: Needs work » Needs review

added to exclude meantime and it now it needs 10.4.x I bet

andypost’s picture

andypost
he/him
Primary language Russian
commented

Maintainers suggested https://github.com/phpstan/phpstan/issues/10954#issuecomment-2088522792

https://phpstan.org/r/690e2da6-8ad3-4f61-9442-d22971dc836d


$callable = "ReflectionMethod::__construct";

if (method_exists(\ReflectionMethod::class, 'createFromMethodName')) {
    echo "PHP 8.3\n";
    $m = \ReflectionMethod::createFromMethodName($callable);
} else {
    echo "older\n";
    $m = new \ReflectionMethod($callable);
}

  • longwave committed 65e7a1ef on 11.0.x 
    Revert "Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod...

  • alexpott committed aa2830b3 on 11.0.x 
    Issue #3444264 by andypost: [PHP 8.4] ReflectionMethod constructor...
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Should this be marked fixed?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@smustgrave nope - the messages on the issue about reverts and commits from 11.0.x are the wrong way around.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 152b7f0694 to 11.x and a6b7ab7388 to 11.0.x. Thanks!

Committed 9c5eeca and pushed to 10.4.x. Thanks!
Committed 6871694 and pushed to 10.3.x. Thanks!

I don't think we should do the suggestion with method exists as this is called a lot - so doing the ignore in PHPStan and the version check in 10.x seems fine.

  • alexpott committed 6871694d on 10.3.x 
    Issue #3444264 by andypost, alexpott, longwave: [PHP 8.4]...

  • alexpott committed 9c5eecad on 10.4.x 
    Issue #3444264 by andypost, alexpott, longwave: [PHP 8.4]...

  • alexpott committed a6b7ab73 on 11.0.x 
    Issue #3444264 by andypost, alexpott, longwave: [PHP 8.4]...

  • alexpott committed 152b7f06 on 11.x 
    Issue #3444264 by andypost, alexpott, longwave: [PHP 8.4]...

andypost closed merge request !7865

andypost closed merge request !7864

andypost’s picture

andypost
he/him
Primary language Russian
commented

Thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.