Changes to Drupal core have started to cause the Drupal 10 tests for Rules to start failing.

Most of the failures seem to be due to the change in All hook invocation delegated to Module Handler service

Specifically, this change removed a method that Rules was using in test cases - namely \Drupal\Core\Extension\ModuleHandlerInterface::getImplementations()

This leads to about 100 test errors in Rules. And because we use this method in RulesEntityIntegrationTestBase, this also leads to D10 testing errors on many other contributed modules which provide Rules integrations.

The errors looks something like this:

Prophecy\Exception\Doubler\MethodNotFoundException: Method `Double\ModuleHandlerInterface\P1::getImplementations()` is not defined.

Rules uses the method like this:

    $this->moduleHandler->getImplementations('entity_type_build')
      ->willReturn([]);

I have done research, but there is no issue or discussion or code comments about what RulesEntityIntegrationTestBase is doing when it uses this method. I have two theories. First is that the test is just using this method to turn off hook_enitity_type_build(), since that's what it is literally doing. Why? I don't know. Second is that the test just needs to mock a return value for the internal getImplementations() method so that the Unit testing doesn't fail when trying to call a non-existent function. This could also be true.

Regardless, the quickest way to find out is to remove this line of code and see if it causes problems. If it does, we can re-invent the wheel and solve them in a different way than with this now-removed method.

Here is a patch that removes that line of code in RulesEntityIntegrationTestBase. We currently have 149 test errors - let's see how this patch changes that number.

CommentFileSizeAuthor
remove-module-handler-get-implementations.patch793 bytestr

Comments

TR created an issue. See original summary.

tr’s picture

Status: Active » Needs review
tr’s picture

Status: Needs review » Postponed

Well, the tests got farther before they failed this time, but now we're hitting another problem. I've opened #3346846: New 'Drupal\Component\DependencyInjection\ReverseContainer' service to replace DrupalKernel::getServiceIdMapping() to handle that other problem and will wait until that is fixed before progressing on this current issue.

tr’s picture

Status: Postponed » Needs review

I decided to test against 10.0 instead of 10.1, to isolate where the problems were. As you can see in https://www.drupal.org/pift-ci-job/2556946, the last time we tested against 10.0 we had 95 errors and this patch has reduced it to 3. I think that's pretty conclusive that this patch fixes the things it was supposed to fix, so I'm going to commit it. If there are side effects they should be minor and we can handle them in other issues.

I will also test other contributed modules once this has been committed to see if this also fixes their errors when testing their Rules integrations.

  • TR committed e552e2bb on 8.x-3.x
    Issue #3346842 by TR: [10.0] ModuleHandlerInterface::getImplementations...
tr’s picture

Status: Needs review » Fixed

Note, this only affected the TESTS and not the actual Rules code that would execute on a website.

Note, I did go and test some contributed modules, for example Honeypot, and their Rules integration tests WERE fixed by this.

Status: Fixed » Closed (fixed)

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