Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix supertype condition in LoadIncludes. #587

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

donquixote
Copy link

Direction is wrong.

As a test case you can use something like this:

class XClass {
  public function foo(ModuleHandler $a, ModuleHandlerInterface $b, object $c, mixed $d) {
    $a->loadInclude('non_existing_module', 'inc', 'something');
    $b->loadInclude('non_existing_module', 'inc', 'something');
    $c->loadInclude('non_existing_module', 'inc', 'something');
    $d->loadInclude('non_existing_module', 'inc', 'something');
  }
}

I have not looked deeply into your package so I just post this code here instead of actually writing a test :)

@mglaman
Copy link
Owner

mglaman commented Jul 13, 2023

  public function testLoadInclude() {
    $module_handler = $this->getModuleHandler();
    // Include exists.
    $this->assertEquals(__DIR__ . '/modules/module_handler_test/hook_include.inc', $module_handler->loadInclude('module_handler_test', 'inc', 'hook_include'));
    $this->assertTrue(function_exists('module_handler_test_hook_include'));

The module is in core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/hook_include.inc. I'm guessing modules in this location aren't discovered by the test extension discovery.

That's because it's a mocked module handler:

  protected function getModuleHandler() {
    $module_handler = new ModuleHandler($this->root, [
      'module_handler_test' => [
        'type' => 'module',
        'pathname' => 'core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.info.yml',
        'filename' => 'module_handler_test.module',
      ],
    ], $this->cacheBackend);
    return $module_handler;
  }

So I guess this PR does fix a bug in its logic :)

@mglaman
Copy link
Owner

mglaman commented Jul 13, 2023

    // Register entity_test text field.
    $this->container->get('module_handler')->loadInclude('entity_test', 'install');

In core/tests/Drupal/KernelTests/Core/Field/FieldAccessTest.php seems like a weird failure:

 ------ -------------------------------------------------------------- 
  Line   core/tests/Drupal/KernelTests/Core/Field/FieldAccessTest.php  
 ------ -------------------------------------------------------------- 
  60     A file could not be loaded from                               
         Drupal\Core\Extension\ModuleHandlerInterface::loadInclude     
 ------ -------------------------------------------------------------- 

@donquixote
Copy link
Author

That's because it's a mocked module handler:

The list of "known modules" for this check has nothing to do with the module handler instance used in that test.
There is a registry of modules somewhere..
And it seems none of the test modules is included.

@mglaman
Copy link
Owner

mglaman commented Jul 25, 2023

I didn't forget about this. I just am waiting for time to figure out the "regression" for that unit test. The module inside of core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test (source) isn't discoverable even with test discovery enabled.

@donquixote
Copy link
Author

Maybe you have to disable the check for tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants