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.
- Status changed to Needs review
about 2 years ago 6:19pm 24 March 2023 - 🇫🇷France andypost
- 🇫🇷France andypost
And a bit more for
git grep "'strtolower'"andgit grep "'array_merge'" - 🇺🇸United States ahmadhalah
Tested manually it works on Drupal core 9.5.6
- Status changed to Needs work
about 2 years ago 1:27am 26 March 2023 - 🇺🇸United States smustgrave
@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?
- Status changed to Needs review
about 2 years ago 6:15am 26 March 2023 - Status changed to Needs work
about 2 years ago 6:23am 26 March 2023 - 🇺🇸United States smustgrave
I don’t know if replacing more is the answer. Should be documented in IS what is being replaced though
- 🇺🇸United States ahmadhalah
@smustgrave patch #68 works with Drupal 9.6.5. Did you test it on that version?
- 🇫🇷France andypost
new candidates from 📌 Deprecations for PHP 8.1 get_class() and get_called_class() without argument Fixed
@smustgrave it looks like it needs split as growing, basically all places that are not supposed to be serializable can benefit from that
- 🇦🇺Australia dpi Perth, Australia
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.
- 🇦🇺Australia dpi Perth, Australia
Marked 🐛 Role::postLoad() warning on every load Closed: duplicate as duplicate.
- 🇩🇪Germany donquixote
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 .
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. - 🇩🇪Germany donquixote
- $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.
