Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...)

I will work at this.

Hope it fits.

We need also to adjust PHPStan baseline, this would have failed if #3259355: Always do a full phpstan analysis on DrupalCI were in

I do believe something went wrong with the MR, there are 92 files changed, 1064 changes in code, including some related to static::class but also... coding standards in css files? scaffold deprecation? and documentation of the migration plugin. And many more.
i couldn't review, moving back to needs work, i'm sorry.

Lots of test fails.

The MR needs to be rebased.

When I look to phpstan-baseline.neon, I find 2 other instances:

1. message: "#^\\#lazy_builder value 'string' at key '0' is invalid\\.$#"
   count: 1
   path: modules/filter/src/FilterProcessResult.php
   

   for:

   '#lazy_builder' => [$callback, $args],
   

   I think we need to do explode('::', $callback) first.

2. message: "#^\\#access_callback value ''Drupal\\\\\\\\Tests\\\\\\\\Core…'\\|'Drupal\\\\\\\\Tests\\\\\\\\Core…'\\|'Drupal\\\\\\\\Tests\\\\\\\\Core…'\\|'Drupal\\\\\\\\Tests\\\\\\\\Core…'' at key '0' is invalid\\.$#"
   count: 1
   path: tests/Drupal/Tests/Core/Render/RendererTest.php
   

   for:

   '#access_callback' => 'Drupal\Tests\Core\Render\TestAccessClass::' . $method,
   

All the other changes look good.

@daffie, I'll check this.

Updated the phpstan-baseline.neon file.

I'll try to fix this merge

Merged master

PHPStan is failing.

@longwave, sorry, I'll check this

If I try to change the line in RenderTest.php to

    $build = [
      '#access_callback' => ['Drupal\Tests\Core\Render\TestAccessClass', $method],
    ];

phpstan returs the following error:

Running PHPStan on changed files.
 ------ ------------------------------------------------------------------------------------
  Line   tests/Drupal/Tests/Core/Render/RendererTest.php                                                                                                               
 ------ ------------------------------------------------------------------------------------
  619    #access_callback callback array{'Drupal\\Tests\\Core…', 'accessFalse'|'accessResultAllowed'|'accessResultForbidd…'|'accessTrue'} at key '0' is not callable.  
 ------ ------------------------------------------------------------------------------------ 

So it might be better to keep as it is.

The errors on phpstan at #17 were due to a bad merge, I reverted the merge and did it again.

Updating status.

with the MR, the phpstan-baseline.neon is okay, I couldn't find any more errors for "'non\\-empty\\-string' at key '0' is invalid", and all the calls for methods that created them were fixed besides the one cited in #21.

i found another 9 uses of this concatenation, i will list them here, but I consider them out of the scope of the issue because the focus was phpstan. considering this, moving to RTBC.

core/modules/comment/src/Entity/Comment.php (292)
core/modules/content_translation/src/ContentTranslationHandler.php (179)
core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldWidget/DummyAjaxWidget.php (34)
core/modules/media/src/Entity/Media.php (492)
core/modules/media/src/Plugin/Filter/MediaEmbed.php (250)
core/modules/user/src/EntityOwnerTrait.php (41)
core/tests/Drupal/KernelTests/Core/Field/Entity/BaseFieldOverrideTest.php (77)
core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php (356)
core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php (273)

Is it really right to change all these cases, given the code apparently worked before and we are just doing this to please PHPStan?

This feels like it could be raised as a bug in mglaman/phpstan-drupal instead, as that has special code to deal with #lazy_builder and friends: https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/Ren... - if this is valid Drupal code I think it should be fixed over there or in PHPStan instead?

sorry, the list I send is a suggestion of other possible concatenations.

they were not changed!

I am sorry that I made this confusing.
this list is not included in the MR.

Asked @mglaman in Slack about changing PHPStan but agree that it's probably more trouble than it's worth trying to get it to understand static::class . '::someMethod' properly, so I retract my comment in #24, but I think one of the changes is incorrect - see MR comment.

Having said that, I then looked into PHPStan internals and think I can fix it, so I opened https://github.com/mglaman/phpstan-drupal/pull/436 which should allow it to parse static::class . '::someMethod' correctly.

I tried to address https://git.drupalcode.org/project/drupal/-/merge_requests/1709#note_102077

Updated IS steps with links to PHP 8.2 changes so it may need more work (I did not check current MR)

- https://wiki.php.net/rfc/deprecate_partially_supported_callables
- https://php.watch/versions/8.2/partially-supported-callable-deprecation

re-parented

There's still one left: \Drupal\filter\FilterProcessResult::createPlaceholder()

    // Add the placeholder attachment.
    $this->addAttachments([
      'placeholders' => [
        $placeholder_markup => [
          '#lazy_builder' => [$callback, $args],
        ],
      ],
    ]);

Suppression in the PHPStan baseline:

		-
			message: "#^\\#lazy_builder value 'non\\-empty\\-string' at key '0' is invalid\\.$#"
			count: 1
			path: modules/node/src/NodeViewBuilder.php

A lot of people (including me) when to a lot of different approaches to get it fixed (see the various commits on the MR), but we seem to all haven't found the correct way to do it.

https://github.com/mglaman/phpstan-drupal/pull/436 is about to be merged, which makes phpstan-drupal more intelligent for resolving static::class . '::callback'.

As for that last example, seems like something phpstan-drupal needs a test case for. We're not resolving the value of $callback

https://github.com/mglaman/phpstan-drupal/releases/tag/1.1.22 was released. This removes the warning about not being able to parse static::class concatenation with method names.

Tagging for IS update:

Coming from #3190585-14: [META] Run PHPStan on Drupal core but as 10.0 core should be compatible with PHP 8.2 this now required

(emphasis mine)

vs

It's just a PHPStan thing where static:class is handled a specify way for generics

vs

https://github.com/mglaman/phpstan-drupal/releases/tag/1.1.22 was released. This removes the warning about not being able to parse static::class concatenation with method names.

So either this _was_ a PHPStan(-drupal) thing, that isn't a PHPStan(-drupal) thing any more, making this issue obsolete, or we need this anyway to become PHP 8.2 compatible.

Or something completely different?

@Spokje This was a phpstan-drupal thing, where it did not understand static::class . '::methodName' was equivalent to [static::class, 'methodName'] for callables. It now does understand that those are equivalent and so postponing this until we upgrade to mglaman/phpstan-drupal 1.1.22, which should solve the false positives in the baseline that relate to this issue.

Thanks @longwave,

So if I understand this correctly: As soon as we require mglaman/phpstan-drupal 1.1.22 this false positives will disappear, making this issue obsolete and there's no need for doing this anyway to become (more) PHP 8.2 compatible?

FWIW: I locally did a rebuild of the PHPStan baseline with mglaman/phpstan-drupal 1.1.22 and all the failures we're fixing in the MR here are indeed gone.

So this issue will be obsolete when Core requires that version or higher.

The next release of phpstan-drupal will include a stub for FilterProcessResult to define $callable as callable-string, fixing the one reported error here. See https://github.com/mglaman/phpstan-drupal/pull/453

https://wiki.php.net/rfc/partially-supported-callables-expand-deprecatio... accepted

Filed bug for PHP 8.2 #3299327-3: Replace deprecated static::method() calls for PHP 8.2

https://wiki.php.net/rfc/deprecate_partially_supported_callables
btw the recommended variant of Callable is

If compatibility with PHP < 8.1 is not desired, use of the first-class callable syntax self::method(...) is also possible.

There's even Rector rule https://github.com/rectorphp/rector-src/pull/2544 and follow-up so rectorphp >=0.13.8 is required

As far as I can tell, this issue is now outdated, since all the (false) positives in PHPStan are gone since #3299606: Update mglaman/phpstan-drupal to 1.1.25 to unblock testing on PHP 8.2 landed.

@Spokje please check #42 - I'm sure we must normalize callables as 10+ requires PHP 8.1

I did not update IS because not clear how to apply rector rule to convert'em all

Anyway it should be postponed on #3299327: Replace deprecated static::method() calls for PHP 8.2

I looked at #37 when I closed this one. I'll leave it as it is now, because I'm lost at this point.

static::class . "::method" is the format used here, and this is fine; this is suggested in the RFC as a replacement for static::method. I don't think we need to do anything here, even for future PHP versions - only #3299327: Replace deprecated static::method() calls for PHP 8.2.

@longwave the suggested method is to use first-class callable since 8.1 as it's more optimized (require no conversion from array to callable so improve performance)

re-title to make it clear and updated IS

There's rector for that https://github.com/rectorphp/rector-src/commit/fe88fbb6306cec3728b9158fd...

So I filed #3300804: Add rector for PHP 8.2 callables

Parent issue commited and there's related follow-up #3309750: Fix callable syntax for PHP 8.2 in Views.php

last blocker for PHP 8.2 commited, so this can be commited as improvement only to 10.1

Here's a start patch

And here conversion of "static::" and "self::" I found (surely needs rector)

Meantime discovered bug, so callables help #3318581: FIx \Drupal\Core\Config\ConfigImporter::doSyncStep() to accept callable

patch #54 for 10.1.x (some offset)

Fix failed test

Use callable in module handler instead of call_user_func_array

Should this form be changed also?

 $class = static::class;

      '#element_validate' => [
        [$class, 'validateDatetime'],
      ]

into something like

    // Note that since this information is cached, the #date_timezone property
    // is not set here, as this needs to vary potentially by-user.
    return [
      '#input' => TRUE,
      '#element_validate' => [
        static::validateDatetime(...),
      ]

Not sure I get all the implications. In any case the patch from #58 looks good

This doesn't look like it's related to PHPStan anymore.

@nod_ it's great question but closures are not serializable so it should break render cache, I see a lot of usage in render element classes so they should remain serializable for form-api purposes.

$ php -r '$f=strlen(...);echo serialize($f);'
PHP Fatal error:  Uncaught Exception: Serialization of 'Closure' is not allowed in Command line code:1
Stack trace:
#0 Command line code(1): serialize()
#1 {main}
  thrown in Command line code on line 1

Thanks a lot for the explanations :)

• Maybe \Drupal\user\Entity\Role::postLoad could be update from uasort($entities, [static::class, 'sort']); uasort($entities, static::sort(...));?
• \Drupal\migrate_drupal_ui\Batch\MigrateUpgradeImportBatch::run might have some, it's for batch related things so it might not be possible because of a serialization thing
• in \Drupal\Core\Mail\MailFormatHelper::htmlToText the call to $string = preg_replace_callback($pattern, [static::class, 'htmlToMailUrls'], $string); could be updated?
• in \Drupal\views\Plugin\views\filter\Date::hasValidGroupedValue the call to $actual = count(array_filter($group['value'], [static::class, 'arrayFilterZero'])); could be updated?

All the other static::class i could see were used in render arrays or in tests so per #61 they should be left alone.

Saw a couple of array_map([static::class, __FUNCTION__], $merged) in json API module, or 'callback' => [static::class, 'doesNotExist'] in tests but I'm not sure those are relevant for this issue.

Here's patch #58 for Drupal 9.5.x

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll after #3299327: Replace deprecated static::method() calls for PHP 8.2

And 2 more places found

And a bit more for git grep "'strtolower'" and git grep "'array_merge'"

Tested manually it works on Drupal core 9.5.6

@ahmadhalah patch #67 applied cleanly to D10 and patch #68 seems to be missing bulk of changes.

Applied patch #67

Searched for [static::class,
There are 82 matches still.

Could it be added to the issue summary specifically what is being replaced and what isn't?

Replacing a few more instances of static::class

I don’t know if replacing more is the answer. Should be documented in IS what is being replaced though

@smustgrave patch #68 works with Drupal 9.6.5. Did you test it on that version?

Fixed CCF

Hiding files up till #67

@andypost could you update what is being replaced in the IS please.

new candidates from #3351004: Deprecations for PHP 8.1 get_class() and get_called_class() without argument

@smustgrave it looks like it needs split as growing, basically all places that are not supposed to be serializable can benefit from that

Looks like we're addressing array-callable, not just string callable. And \Closure::fromCallable, others?

Issue summary needs update, title could lose the mention of string.

Marked #3354146: Role::postLoad() warning on every load as duplicate.

We should be careful with this.

Replace usages of [static::class, '::methodName'] to static::methodName(...) which is more readable and performant since PHP 8.1

Readable, yes.
Also, closures are _verified_. Once we have a closure object, we know it can be called. The 'Call to undefined function' error happens when the closure is created, not when it is called - which makes for nicer error detection.

But:
More performant - yes, but it depends.
Also it is not serializable - but this is only relevant for caching between requests.

I created this script to compare different ways of calling different types of callbacks:
https://gist.github.com/donquixote/85efcca90056111e967dd14cb1f9de9c
It can be a good idea to copy the result into a spreadsheet app and sort by different columns.
The results could be different depending on php configuration etc. Also I found some unexpected side effects from changing the total number of repetitions and the order of comparison candidates.

Conclusions:

• For any callable value, If we want to call it only one time, it is faster to call it directly than to first convert it to a closure object.
• If we have a closure, the cost of calling is almost the same no matter if it is a function or a class (static) method. (if they have the same parameter signature).
• Calling a closure is generally faster than calling a callable-string or callable-array, if we don't include the time for converting it to a closure.
• Time can be lost for the extra step of converting to closure. So it is only worthwhile if we call it repeatedly, or if the alternative would cost us the same.

E.g. the way we currently do it in ModuleHandler actually makes things slower, not faster, because we do not reuse the $hookInvoker.

  public function invokeAllWith(string $hook, callable $callback): void {
    foreach (array_keys($this->getImplementationInfo($hook)) as $module) {
      $hookInvoker = \Closure::fromCallable($module . '_' . $hook);
      $callback($hookInvoker, $module);
    }
  }

It would be faster to pass the callable-string directly:

  public function invokeAllWith(string $hook, callable $callback): void {
    foreach (array_keys($this->getImplementationInfo($hook)) as $module) {
      $callback($module . '_' . $hook, $module);
    }
  }

The performance argument does not need to stop us, there can be sufficient other reasons to use first-class callables.
However, if we want to use performance as an argument _for_ first-class callables, we need to look more closely.

-    $map = array_map('strtolower', $connection->schema()->getFieldTypeMap());
+    $map = array_map(strtolower(...), $connection->schema()->getFieldTypeMap());

We can do this, but from what I found it will be slower, not faster. Imo in a case like this, a literal string is totally fine, and looks even simpler than the first-class callable. It is even less ambiguous because it is automatically in the root namespace.
Also, the IDE (or a static analysis tool) sees that the parameter is callable, so it can know that the literal string refers to a function.

-    $element['#element_validate'][] = [static::class, 'validateElement'];
+    $element['#element_validate'][] = static::validateElement(...);

Should the form and render array stuff not be serializable?
Maybe in some cases it doesn't need to be.
I think a raw original form is cached, but a processed form is not. So maybe it's fine.
This being said, the first-class callables do look nicer than the callable arrays. So for these I totally see the point, whenever it works.

-        '#element_validate' => [[static::class, 'elementValidateFilter']],
+        '#element_validate' => static::elementValidateFilter(...),

This is missing brackets.