ModuleHandlerInterface::getImplementations() is deprecated in 9.4

This is reported in the Travis tests at 9.5. Drupal Rector may report this on #3289475: Automated Drupal 10 compatibility fixes - Scheduler 2.x at some point. Or we could fix it manually before then.

This is a suggestion from DeiterHolvoet on #3289475-11: Automated Drupal 10 compatibility fixes - Scheduler 2.x, to change getHookImplementations() to getHooks(), like this:

public function getHooks(string $hookType, $entity) {
  $entityTypeId = (is_object($entity)) ? $entity->getEntityTypeid() : $entity;
  $hooks = [$hookType, "{$entityTypeId}_{$hookType}"];
  // For backwards compatibility the original node hook is also added.
  if ($entityTypeId == 'node') {
    $legacy_node_hooks = [
      'hide_publish_date' => 'hide_publish_on_field',
      'hide_unpublish_date' => 'hide_unpublish_on_field',
      'list' => 'nid_list',
      'list_alter' => 'nid_list_alter',
      'publish_process' => 'publish_action',
      'unpublish_process' => 'unpublish_action',
      'publishing_allowed' => 'allow_publishing',
      'unpublishing_allowed' => 'allow_unpublishing',
    ];
    $hooks[] = $legacy_node_hooks[$hookType];
  }
  // Append the $hook to the end of the module.
  foreach ($hooks as &$hook) {
    $hook = "scheduler_$hook";
  }
  return $hooks;
}

and invoked like this:

// Allow other modules to add to the list of entities to be published.
$hooks = $this->getHooks('list', $entityTypeId);
foreach ($hooks as $hook) {
  $this->moduleHandler->invokeAllWith($hook, function (callable $hook, string $module) use (&$ids, $process, $entityTypeId) {
    // Cast each hook result as array, to protect from bad implementations.
    $ids = array_merge($ids, (array) $hook($process, $entityTypeId));
  });
}

Thanks chandu9729

Shouldn’t we update parent MR with this fix rather than fixing it here?

This issue is all about fixing the getImplementations hooks, so that stays here.
You can create a new issue for the other warning you found - "Missing explicit access check on entity query"

The parent issue will not have any specific deprecation code fixes. It will have a mr for testing at D10, which will just have the version changes and any thing else necessary to allow testing at D10.

Hi vishalkhode,
Thanks for working on this, it looks good at first read-through. I'll have to make sure that our test coverage fully checks the changes, but I am fairly sure we are OK on that.

To test at D10 you will need to add the version changes from #3271462: [META] Drupal 10 preparation. Some of the optional modules that we use in testing are not compatible with D10 yet (and may not be) so we might need to customise the test script for D9 vs D10. But in the first instance, just add in the changes from that parent issue and see how the D10 test goes.

Thanks for the hint about skipping the tests chandu7929. That would be nice to use, when it becomes available.

#1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped

#3261817: TestRequirementsTrait::checkRequirements() does not work as expected

I think we need some other way around to skip test in D10 in absence of module codebase, while include the same in D9.

Yes that is exactly what I have been thinking about, and I have some ideas. For now, we need to remove your two added commits. I will revert them, then check this actual issue and commit it.

Yes that might work. We'll do that on a separate issue, I want to get this one back on focus to the ModuleHandlerInterface::getImplementations() changes.

it looks like all tests are passing which is related to issue: ModuleHandlerInterface::getImplementations().

Yes they are. With the extra call to moduleHandler()->getImplementations that you found and fixed in 89e7a4c7 I am surprised this was not reported before. Even if there are no impementations of that hook in the test modules, the deprecation should still have been thrown. But the tests against the previous commit did not report it. That is odd, and needs investigation.

Also, the new replacement function ModuleHandler::invokeAllWith() does not exist in Drupal 8 and Scheduler needs to maintain compatibility with Drupal 8.9 to allow sites to convert. Scheduler 2.x does not yet have a release so the first 2.0 should/could remain compatible with 8.9. But then would 2.1 become compatible with D10 and drop 8.9? I don't want to create Scheduler 3.x as that increases the code maintenance burden. More planning needed ...

@vishalkhode that could be a great solution. I'd not realised that the new replacement can be called to get the names and not actually invoke the functions there and then. It did seem like a massive downgrade in functionality, losing that ability. But if you are right, yes please push this and we'll see.

@chandu7929, @vipin.mittal18 as I said in #16 we really do not want to have another major version branch to maintain if at all possible. We will if that is the only solution, but everything else must be tried before we do that.

Excellent work vishalkhode, this is exactly what we needed.

chandu7929, there is no need to change the MR, as I won't be merging it completely, but will just make a direct commit for the required getImplementations() changes. I will fix the few coding standards faults, then test again. I also want to test the _api change locally as that is not covered by any automated tests.

The new function invokeAllWith() only exists from Core 9.4 hence the version check needs to be precise.

Committed just the two changes for getImplementations()
Thanks to @DieterHolvoet for the original code and @vishalkhode for the final version, and to all for their interest.

Now that this is fixed, we can reduced the deprecation count in Travis builds - see this commit