Problem/Motivation
2x: ModuleHandlerInterface::getImplementations() is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Instead you should use ModuleHandlerInterface::invokeAllWith() for hook invocations, or you should use ModuleHandlerInterface::hasImplementations() to determine if hooks implementations exist. See https://www.drupal.org/node/3000490
Needs a conditional like in the field_group D10 issue. Alternatively, we could possibly call it like a regular hook and check if any module returned TRUE. slightly less efficient, as this is currently optimized to return TRUE on the first module returning TRUE.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | pathauto-3293221-12-interdiff.txt | 2.05 KB | berdir |
| #12 | pathauto-3293221-12.patch | 3.4 KB | berdir |
| #10 | pathauto-3293221-10.patch | 3.44 KB | berdir |
| #9 | pathauto-3293221-9.patch | 749 bytes | benjifisher |
| #8 | pathauto-3293221-8.patch | 1021 bytes | benjifisher |
Comments
Comment #2
ada hernandez commentedAdding a patch to remove the ModuleHandler::getImplementations() deprecation.
Comment #3
berdirThanks, there are two issues with it:
a), we need to keep both versions, to support 9.3 still. See the patch in #3278537: D10 compatibility | Declaration of Drupal\field_group\Routing\RouteSubscriber::getSubscribedEvents() must be compatible with Drupal\Core\Routing\RouteSubscriberBase::getSubscribedEvents(), check core version, then do one or the other.
b) I don't think this is working. You are now inside a nested function, that return TRUE/FALSE is not going anywhere. seems like we don't have test coverage of this? We need to have a by reference variable that we set to TRUE/FALSE and then return that. Also no need to make $results an array. And to keep the optimization, we need to check if $result is already TRUE and skip the call then, so:
Comment #4
s_bhandari commentedHi,
Patch added for the same. Please review.
Thanks.
Comment #5
s_bhandari commentedComment #6
berdirthis stores the return in a results array but then uses it as a boolean. you need to assign to $results directly.
Also, you can't return anymore, you're inside a function, it goes nowhere. you need to use (&$result) so that the result if it can be returned.
Comment #7
benjifisherI think you are referring to #3278537: D10 compatibility | Declaration of Drupal\field_group\Routing\RouteSubscriber::getSubscribedEvents() must be compatible with Drupal\Core\Routing\RouteSubscriberBase::getSubscribedEvents(). I am adding that as a related issue.
I plan to work on this today. If I forget to un-assign myself, feel free to reassign after that.
Comment #8
benjifisherHere is a patch that implements the solution proposed in the issue summary.
I am not providing an interdiff, since this is a different approach from the earlier patches.
Comment #9
benjifisherHere is a patch that implements yet another approach. This should work the same way with both Drupal 10 (tested) and 9.3 (untested).
Comment #10
berdirInteresting. I guess that works, I do wonder what the performance impact is, getImplementations() is cached and this is likely going to call hasImplementations() 100+ times on sites with many modules. And most sites won't actually have an implementation of this hook.
So I'll go with #8, we can eventually clean this up and do the check directly in the callback.
Also sneaking in D10 test fixes that didn't fail before.
Comment #12
berdirAh, need to pass in the Url object, forgot that you can do that.
Comment #14
berdirCommitted.