Problem/Motivation

There's some deprecations in upcoming PHP 8.3

- https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

Proposed resolution

https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#...
- Replace deprecated get_class() call without argument 

$ git grep 'get_class()'
core/lib/Drupal/Component/Plugin/PluginManagerBase.php:128:    throw new \BadMethodCallException(get_class() . '::getFallbackPluginId() not implemented.');

https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#...
- ReflectionMethod::__construct() with 1 argument

core/lib/Drupal/Component/Utility/ArgumentsResolver.php:126:      return new \ReflectionMethod($callable);

https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#...

$ git grep -A 5 ReflectionProperty|grep setValue
core/tests/Drupal/Tests/Component/Utility/HtmlTest.php-31-    $property->setValue(NULL);
CommentFileSizeAuthor
#19 3374223-19.patch1.26 KBandypost
#19 interdiff.txt673 bytesandypost
#12 3374223-12.patch1.92 KBandypost
#12 interdiff.txt1.4 KBandypost
#2 3374223-2.patch1.28 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3374223-2.patch1.28 KB

It should wait for the first beta of 8.3 (in 1 week) but suppose following patch

andypost’s picture

andypost
he/him
Primary language Russian
commented

The ReflectionMethod deprecations should be delayed until 8.4 so version check could be added. Before that it makes no sense to slowdown the method

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
@@ -125,7 +125,7 @@ protected function handlePluginNotFound($plugin_id, array $configuration) {
-    throw new \BadMethodCallException(get_class() . '::getFallbackPluginId() not implemented.');
+    throw new \BadMethodCallException(self::class . '::getFallbackPluginId() not implemented.');

probably it could use separate issue to improve the message like self::class ... is not implemented in ... static::class

neclimdul’s picture

neclimdul
He/Him
commented

#2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems added the ReflectionMethod call to 11.x so its not even in php's audit. Neat!

> it makes no sense to slowdown the method
Since we bypassed the audit, should we raise that slowdown concern with the PHP mailing list? I expect with symfony, laminas, and nette in their list some form of "if you expect reflection to be fast you're doing it wrong" response which is probably accurate but maybe.

Sample implementation post deprecation for reference:

    // Optimized for arrays over strings.
    if (is_array($callable)) {
      return new \ReflectionMethod($callable[0], $callable[1]);
    }
    if (is_string($callable) && str_contains($callable, '::')) {
      $callable = explode('::', $callable, 2);
      return new \ReflectionMethod($callable[0], $callable[1]);
    }
    return new \ReflectionFunction($callable);

+++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
@@ -125,7 +125,7 @@ protected function handlePluginNotFound($plugin_id, array $configuration) {
-    throw new \BadMethodCallException(get_class() . '::getFallbackPluginId() not implemented.');
+    throw new \BadMethodCallException(self::class . '::getFallbackPluginId() not implemented.');

Don't we need static:: to get that sweet LSB to make it actually useful? Interesting that this passed testing...

andypost’s picture

andypost
he/him
Primary language Russian
commented

Don't we need static::

Using Google's Bard the suggested change is

protected function getFallbackPluginId($plugin_id, array $configuration = []) {
throw new \BadMethodCallException(static::class . '::getFallbackPluginId() not implemented. This method must be implemented in order to use the fallback plugin.');
}
andypost’s picture

andypost
he/him
Primary language Russian
commented

Interesting that this passed testing...

Because there's no coverage for the method( Sounds like needs separate issue

andypost’s picture

andypost
he/him
Primary language Russian
commented

One more RFC landed https://wiki.php.net/rfc/saner-inc-dec-operators

smustgrave’s picture

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

Seems like a simple enough change.

Feel this conversation was just had about sections that never had testing, if they belong in follow ups or not.

Think with a change this small moving the testing to a follow up wouldn't be a problem.

Maybe we open a Meta for extended test coverage for items that must be included in D11. So pushing testing to follow ups don't just hang there.
May be a thread on slack conversation though.

andypost’s picture

andypost
he/him
Primary language Russian
commented

This week the beta 1 will out and I bet we'll need to split assertion deprecation and new deprecations

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

The issue summary appears to be talking about more changes than the patch. The changes in the patch look fine, but I don't understand the mismatch.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt1.4 KB
new 3374223-12.patch1.92 KB

Filed separate issue for assertions #3375693: Fix deprecated assert_options() function usage for PHP 8.3 as they are blocking linker now

Checking on beta1 and found that 

/var/www/html/web $ php -v
PHP 8.3.0beta1 (cli) (built: Jul 18 2023 23:53:10) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.3.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.3.0beta1, Copyright (c), by Zend Technologies
    with Xdebug v3.3.0-dev, Copyright (c) 2002-2022, by Derick Rethans

Reflection method is not deprecated yet but I added a fix (#5 suggested a bit different from the RFC)
the new createFromMethodName() method is added

/var/www/html/web $ php -r 'class aa { function b() {}}; var_dump(new ReflectionMethod(...explode("::", "aa::b")));'
object(ReflectionMethod)#1 (2) {
  ["name"]=>
  string(1) "b"
  ["class"]=>
  string(2) "aa"
}
/var/www/html/web $ php -r 'class aa { function b() {}}; var_dump(new ReflectionMethod("aa::b"));'
object(ReflectionMethod)#1 (2) {
  ["name"]=>
  string(1) "b"
  ["class"]=>
  string(2) "aa"
}
/var/www/html/web $ php -r 'class aa { function b() {}}; var_dump( ReflectionMethod::createFromMethodName("aa::b"));'
object(ReflectionMethod)#1 (2) {
  ["name"]=>
  string(1) "b"
  ["class"]=>
  string(2) "aa"
}

setValue() now throws

/var/www/html/web $ php -r 'class aa { public static $a; }; var_dump((new ReflectionProperty("aa", "a"))->setValue(null, null));'
NULL
/var/www/html/web $ php -r 'class aa { public static $a; }; var_dump((new ReflectionProperty("aa", "a"))->setValue(null));'
PHP Deprecated:  Calling ReflectionProperty::setValue() with a single argument is deprecated in Command line code on line 1

Deprecated: Calling ReflectionProperty::setValue() with a single argument is deprecated in Command line code on line 1
NULL

get_class() throws, so replaced with static:: as implementation should live in called class

/var/www/html/web $ php -r 'class aa { static function b() { var_dump(get_class()); } } aa::b();'
PHP Deprecated:  Calling get_class() without arguments is deprecated in Command line code on line 1

Deprecated: Calling get_class() without arguments is deprecated in Command line code on line 1
string(2) "aa"
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/lib/Drupal/Component/Utility/ArgumentsResolver.php
@@ -123,7 +123,7 @@ protected function getReflector(callable $callable) {
-      return new \ReflectionMethod($callable);
+      return new \ReflectionMethod(...explode('::', $callable));

asked about it https://github.com/php/php-src/pull/11703#discussion_r1268709319

Status: Needs review » Needs work

The last submitted patch, 12: 3374223-12.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Related issues: +#3375584: [random test failure] Random failure in PathWorkspacesTest

Random failure

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
@@ -125,7 +125,7 @@ protected function handlePluginNotFound($plugin_id, array $configuration) {
-    throw new \BadMethodCallException(get_class() . '::getFallbackPluginId() not implemented.');
+    throw new \BadMethodCallException(static::class . '::getFallbackPluginId() not implemented.');

this message is not covered with test as no test failed in #3375693-5: Fix deprecated assert_options() function usage for PHP 8.3

smustgrave’s picture

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

IS seems to be updated so remarking.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt673 bytes
new 3374223-19.patch1.26 KB

Removed hunk for ArgumentsResolver because misread RFC https://github.com/php/php-src/pull/11703#discussion_r1269355095

This RFC proposes to add the following new factory method in PHP 8.3 and deprecate the second constructor signature in PHP 8.4. Finally, the deprecated signature would become unsupported in either PHP 9.0 or 10.0

andypost’s picture

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

updated IS a bit

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3374223-19.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Reviewed & tested by the community

PathWorkspacesTest again

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3374223-19.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Back to RTBC

  • longwave committed d12b95d2 on 10.1.x 
    Issue #3374223 by andypost, smustgrave, neclimdul: Fix deprecated...

  • longwave committed 4912be31 on 11.x 
    Issue #3374223 by andypost, smustgrave, neclimdul: Fix deprecated...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Backported to 10.1.x as a low risk bug fix to help keep the branches in sync.

Committed and pushed 4912be319e to 11.x and d12b95d2c8 to 10.1.x. Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.1.x-dev
andypost’s picture

andypost
he/him
Primary language Russian
commented

Tests revealed 2 more #3375693-16: Fix deprecated assert_options() function usage for PHP 8.3

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Filed follow-up #3391281: Fix deprecated ReflectionProperty::setValue() usage for PHP 8.3

kopeboy’s picture

kopeboy
Primary language Italian
Location Milan
commented

There's a get_class deprecation in Core/Render as well, see #3421556: Calling get_class() without arguments is deprecated in Drupal\Core\Render\ElementInfoManager