Skip to content
Snippets Groups Projects

Resolve #3519561 Introduce implementationList class for hooks

Open Resolve #3519561 Introduce implementationList class for hooks
issue/drupal-3519561:3519561-hook-ImplementationList into 11.x
1 unresolved thread
Open Andreas Hennings requested to merge issue/drupal-3519561:3519561-hook-ImplementationList into 11.x
1 unresolved thread

Closes #3519561

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #506889 failed

Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal-3519561

#506891

    Merge request pipeline failed for fc32af41

    Code Quality is loading
    Test summary results are being parsed
    Full report
    Metrics reports are loading
    Ready to merge by members who can write to the target branch.
    Loading

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
    398 397 ->willReturn([
    399 398 [$c, 'some_method'],
    400 399 ]);
    401 $this->assertNotEmpty($module_handler->hasImplementations('some_hook'));
    402
    403 $listeners = $r->getProperty('invokeMap')->getValue($module_handler)['some_hook']['some_module'];
    404 // Anonymous class doesn't work with assertSame() so assert it piecemeal.
    405 $this->assertSame(count($listeners), 1);
    406 foreach ($listeners as $listener) {
    407 $this->assertSame(get_class($c), get_class($listener[0]));
    408 $this->assertSame('some_method', $listener[1]);
    409 }
    400 $this->assertTrue($module_handler->hasImplementations('some_hook'));
    • Andreas Hennings
      Andreas Hennings @donquixote
      Author

      The assertions I removed assert internal implementation details, which I don't know why we want to test them.
      The test method name implies that it is about ->hasImplementations(), not about which specific implementations get stored internally.
      If we really want to test something useful, we could test ->hasImplementations() with a hook that has no implementations.

      But tbh, we should probably do this in a kernel test, where we don't have to mock the event dispatcher.

    • nicxvan
      nicxvan @nicxvan
      Developer

      Did this start failing or are you removing it just because you think it doesn't have value?

      If the former we need to find out the bit that changed (I haven't looked closely enough yet at the main change)

      If the latter, then we need to open a separate issue for that cleanup.

    • Andreas Hennings
      Andreas Hennings @donquixote
      Author

      This part of the test is testing an internal property, which we are removing in this MR.

      I pushed a new version where we are instead testing the new property.

    • Please register or sign in to reply
  • Andreas Hennings added 4 commits

    added 4 commits

    • 9476814a - Introduce ImplementationList.
    • 134ddf9f - Drop usage of ->getHookListeners() in ModuleHandler methods.
    • cc2d066f - Fix ModuleHandlerTest::testHasImplementations().
    • 57dfb380 - Reorder imports in ModuleHandlerTest.php.

    Compare with previous version

    Toggle commit list
  • Andreas Hennings added 7 commits

    added 7 commits

    • 293edeac - Add docs on ModuleHandlerTest::getModuleHandler().
    • 9f932fce - Reorder imports in ModuleHandlerTest.php.
    • cf2b7caf - Introduce ImplementationList.
    • 1fb77b2a - Drop usage of ->getHookListeners() in ModuleHandler methods.
    • f0c583ab - Remove ModuleHandler::getHookListeners() and ImplementationList::groupByModule().
    • d3fd1acd - Include files before getting the listeners, in getHookImplementationList().
    • 9bc0342e - Assert that listeners in ImplementationList are callable, and update test.

    Compare with previous version

    Toggle commit list
  • Andreas Hennings added 141 commits

    added 141 commits

    • 9bc0342e...a2db325e - 134 commits from branch project:11.x
    • 0822209f - Add docs on ModuleHandlerTest::getModuleHandler().
    • 18a69c65 - Reorder imports in ModuleHandlerTest.php.
    • 95bc1dc5 - Introduce ImplementationList.
    • cacdee73 - Drop usage of ->getHookListeners() in ModuleHandler methods.
    • 445156e3 - Remove ModuleHandler::getHookListeners() and ImplementationList::groupByModule().
    • de38fedf - Include files before getting the listeners, in getHookImplementationList().
    • fc32af41 - Assert that listeners in ImplementationList are callable, and update test.

    Compare with previous version

    Toggle commit list
    • Please register or sign in to reply