Problem/Motivation

While working on #3259716-53: Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...) I faced that conversion causes error

Steps to reproduce

Fix \Drupal\KernelTests\Core\Config\ConfigImporterTest::testCustomStep() to use https://php.watch/versions/8.1/first-class-callable-syntax

TypeError : method_exists(): Argument #2 ($method) must be of type string, Closure given
 /var/www/html/web/core/lib/Drupal/Core/Config/ConfigImporter.php:510
 /var/www/html/web/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php:825
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Proposed resolution

reorder condition to check for callable first

Remaining tasks

- view
- commit

User interface changes

no

API changes

no

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#7 3318581-7-9.x.patch1.61 KBandypost
#7 3318581-7-10.x.patch1.88 KBandypost
#2 3318581-2.patch1.96 KBandypost
#2 3318581-2-fail.patch948 bytesandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3318581-2-fail.patch948 bytes
new 3318581-2.patch1.96 KB

Fail and fix patches

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: other » configuration system
andypost’s picture

andypost
he/him
Primary language Russian
commented
Parent issue: #3259716: Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...) »

The last submitted patch, 2: 3318581-2-fail.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

I think you can change way less logic by doing...

diff --git a/core/lib/Drupal/Core/Config/ConfigImporter.php b/core/lib/Drupal/Core/Config/ConfigImporter.php
index 4846d9f554d..3d822d875ee 100644
--- a/core/lib/Drupal/Core/Config/ConfigImporter.php
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -507,7 +507,7 @@ public function import() {
    *   Exception thrown if the $sync_step can not be called.
    */
   public function doSyncStep($sync_step, &$context) {
-    if (!is_array($sync_step) && method_exists($this, $sync_step)) {
+    if (is_string($sync_step) && method_exists($this, $sync_step)) {
       \Drupal::service('config.installer')->setSyncing(TRUE);
       $this->$sync_step($context);
     }

Plus we should update the @param to say it accepts string|callable rather then string|array

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 3318581-7-10.x.patch1.88 KB
new 3318581-7-9.x.patch1.61 KB

Its already written in phpdoc @param string|callable $sync_step

Here's 2 patches:
- 10.x could use faster approach for callables (since PHP 8.1 first-class callables are recommended for that)
- 9.x needs to use Closure in test and just change condition

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

I like the change to positive logic and we've got test coverage so +1

  • catch committed 1dbc412 on 10.0.x 
    Issue #3318581 by andypost, alexpott: FIx \Drupal\Core\Config\...
  • catch committed fcf0932 on 10.1.x
    Issue #3318581 by andypost, alexpott: FIx \Drupal\Core\Config\...

  • catch committed 8da49ba on 9.5.x 
    Issue #3318581 by andypost, alexpott: FIx \Drupal\Core\Config\...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/cherry-picked the respective patches to 10.1.x, 10.0.x 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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