Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing

Low-impact bugfix changes with added tests.

This tag is for non-invasive patches which solely make coding standards, testing, and docs improvements.

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -187,7 +205,7 @@ public function scan($type, $include_tests = NULL) {
-      $searchdirs[static::ORIGIN_SITE] = $this->sitePath ?: DrupalKernel::findSitePath(Request::createFromGlobals());
+      $searchdirs[static::ORIGIN_SITE] = $this->sitePath ? : DrupalKernel::findSitePath(Request::createFromGlobals());

We IMHO use this codestyle everywhere, and well, its also an out of scope change.

You can't argue against those numbers:

~/w/d/core (8.0.x) $ ag "\? \:" | wc -l
       4
~/w/d/core (8.0.x) $ ag "\?\:" | wc -l
     414

Thank you

There are a lot of out-of-scope docfixes in this patch. Let's move those into a separate issue.

@effulgentsia and I discussed this and agreed that as a straightforward bugfix with no disruption this could go into a patch release. If that's not the case or if there is a strong reason this change needs to go in before RC, please update the summary with that information. Thanks!

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -103,17 +110,27 @@ class ExtensionDiscovery {
   -   *   Whether file cache should be used.
   -   * @param string[] $profile_directories
   -   *   The available profile directories
   -   * @param string $site_path
   -   *   The path to the site.
   +   *   (optional) Whether file cache should be used. Defaults to TRUE.
   +   * @param string[]|null $profile_directories
   +   *   (optional) The available profile directories. Defaults to NULL, which
   +   *   will cause this object to search for profile directories based on
   +   *   settings.
   +   * @param string|null $site_path
   +   *   (optional) The site path within the Drupal installation. Defaults to
   +   *   NULL, which will cause this object to determine the site based on the
   +   *   request information.
       */
   

   Well, xjm asked for removing those changes, if I understood her correctly

2. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
   @@ -0,0 +1,133 @@
   +    $mock_extension_discovery->expects($this->exactly(4))
   +      ->method('scanDirectory')
   +      ->willReturn($expected);
   +    // Use up our 4 calls to scanDirectory().
   ...
   +    // The scanDirectory() method gets called 4 times while traversing the file
   +    // system once, one each for ORIGIN_CORE, ORIGIN_SITE_ALL, etc.
   +    $test1_mock_extension_discovery->expects($this->exactly(4))
   +      ->method('scanDirectory')
   +      ->willReturn($expected);
   +
   +    // Use up our 4 calls to scanDirectory().
   +    $test1_mock_extension_discovery->scan('module', FALSE);
   

   For some reason I hate this super strict tests.

#21.1: You mean the documentation of the code I was working on?

Yeah, well, I just try to write down how I understand #16.

#21.2: What reason do you have for hating the test that proves that a) the code doesn't work before you change it and b) that it works after you change it?

Well, there is this promise that tests make rewriting your code easier. Your super strict tests, like checking that specific code is called exactly N times, maybe even in which
order, doesn't help about it. Just rising a point here, not a reason to block this patch, IMHO.

Sorry for being annoying.

Testbot was a bit borked yesterday

This seems dangerous to me because if you did...

$a = new ExtensionDiscovery('root/a');
$b = new ExtensionDiscovery('root/b');

$a ->scan('module');
$b->scan('module');

very weird things will happen.

It'd be better to create a cache key to use as a top level key in static $files array. Or try to do away with the static.

I agree, the new approach to key by the root itself, is better.

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -165,8 +165,9 @@ public function __construct($root, $use_file_cache = TRUE, $profile_directories
+    if (($this->profileDirectories === NULL) && $type != 'profile') {

This looks out of scope - what breaks if this hunk is reverted?

Thanks. Looks fine otherwise so back to RTBC pending test runs.

Note that it is perfectly possible to unit test this class by resetting the static using reflection which is exactly what is done in the latest tests in #2329453: Ignore front end vendor folders to improve directory search performance.

Yes but this patch only makes it testable if the app root changes - you'd still need to use reflection to clear the static cache if the root doesn't change. Imo making this testable would be better achieved by making the class use StaticStorage once #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() lands.

That said adding the app route to static cache key does actually make sense it is just that the issue title is a tiny bit misleading.

1. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
   @@ -0,0 +1,133 @@
   +    // The scanDirectory() method gets called 4 times while traversing the file
   +    // system once, one each for ORIGIN_CORE, ORIGIN_SITE_ALL, etc.
   +    $mock_extension_discovery->expects($this->exactly(4))
   

   Testing number of calls is always a bit fishy. How about just asserting that is or is not called as expected.

2. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
   @@ -0,0 +1,133 @@
   +    // Now use test2 in order to check that the cache was invalidated.
   

   The cache is not invalidated.

Given the latest comments on #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() I think we should proceed here to add the root directory to key - it makes sense - we just need to address #53 and then I think we're good to go.

I think we can fix this in 8.1.x - it is a bug. And the change is totally BC as it is a very internal change. I don't think this is a major bug though.

Note that it is perfectly possible to unit test this class by resetting the static using reflection

Why not add a static reset method?

  /**
   * Resets the static cache.
   */
  static function staticReset() {
    self::$files = [];
  }

Of course, the additional index [$this->root] is the right thing to do.

I don't really understand what the test is testing, though.
Why can't we, now that the [$this->root] key is added, write a real tests with a bunch of virtual module files and then verify the result of ->scan() ? Maybe a naive question, sorry :)

Why can't we, now that the [$this->root] key is added, write a real tests with a bunch of virtual module files and then verify the result of ->scan() ? Maybe a naive question, sorry :)

I agree totally with this point. This test feels really not flexible.

Not naive... Kind of the point of the exercise. :-)

Ok. Then why start with these "crazy" tests?
Why not something like here?
I used a swappable scan component for this, but I think the equivalent should be possible with vfsStream.

Or this one, which here tests the real existing files, but could be changed to use vfsStream.

Btw, linking this issue: #2724231: Replacement for ExtensionDiscovery, following SOLID principles

I don't see you on IRC..

'Crazy' is what being a Drupal core dev is all about. You hold back the nausea as you to make a patch that will be in a scope the maintainers can review, while solving the specific problem you're trying to solve.

Would you say that a test as proposed in #59 is out of scope?
Imo, if we make the class testable and craft a meaningful test scenario, this does fit within one issue.

what kind of cultural shifts have occurred in Drupal since January

Since January? I don't know what you are talking about, I must have missed it :)
I can assure you the "crazy" is not personal. But I don't want to derail the issue. IRC or PM.

My concern is that we are starting with stuff that depends on reflection and/or advanced mocking functionality (hence "crazy"), when instead we could start with a test for the public methods.

And if you want to test the side effect prior to the added [$this->root] key, what about this:

$discovery_a = new ExtensionDiscovery($root_a);
$extensions_a = $discovery_a->scan('profile');

$discovery_b = new ExtensionDiscovery($root_b);
$extensions_b = $discovery_b->scan('profile');

static::assertNotSame($extensions_a['minimal'], $extensions_b['minimal']);

Just some quick feedback.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -188,14 +188,8 @@ public function scan($type, $include_tests = NULL) {
   -    // at install time. Therefore Kernel service is not always available, but is
   -    // preferred.
   -    if (\Drupal::hasService('kernel')) {
   -      $searchdirs[static::ORIGIN_SITE] = \Drupal::service('site.path');
   -    }
   -    else {
   -      $searchdirs[static::ORIGIN_SITE] = $this->sitePath ?: DrupalKernel::findSitePath(Request::createFromGlobals());
   -    }
   +    // at install time.
   +    $searchdirs[static::ORIGIN_SITE] = $this->sitePath ?: DrupalKernel::findSitePath(Request::createFromGlobals());
   

   Mh, when we change this, we should IMHO inject the site path into the ExtensionDiscovery at least for the common usecases, otherwise it will do more now.

2. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionDiscoveryTest.php
   @@ -0,0 +1,187 @@
   +
   +    // We need to assert the root path of the extension, but it does not have
   +    // an accessor.
   +    $ref_root = new \ReflectionProperty($module, 'root');
   +    $ref_root->setAccessible(TRUE);
   +    $this->assertEquals(vfsStream::url('root'), $ref_root->getValue($module));
   ...
   +  /**
   +   * Helper method to get the cache array from an ExtensionDiscovery object.
   +   *
   +   * @param ExtensionDiscovery $extension_discovery
   +   *   An ExtensionDiscovery object.
   +   *
   +   * @return array[]
   +   *   The files array.
   +   */
   +  protected function getDiscoveredCache(ExtensionDiscovery $extension_discovery) {
   +    $ref_cache = new \ReflectionProperty(ExtensionDiscovery::class, 'files');
   +    $ref_cache->setAccessible(TRUE);
   +    return $ref_cache->getValue($extension_discovery);
   +  }
   

   Can't you use assertAttribute or so?

This illustrated a dependency between scan() and drupal_valid_test_ua(), which meant turning our humble unit test into a KernelTest.

Look at the unit test. Works for me, locally at least.

One thing to fix was to prepend $this->root when doing file_exists(), so we don't depend on include path.

Those failures lead to the discovery that ExtensionDiscovery ignores the $sites_path if there is a kernel. In that case, it uses \Drupal::service('site.path') instead. During testing, 'site.path' is set to simpletest/[test_id], which doesn't help us at all.

I have not thought much about this..
Generally it seems an injected site path should have precedence. But I don't know what else depends on this misbehavior.

Either way, I think the unit test would work either way, because there is no Kernel.

EDIT: @Mile23 I removed your kernel test simply so testbot can focus on my unit test, and because it seemed your test failed last time.
This does not mean that I want your test out. I don't have a clear opinion on it whatsoever atm.

EDIT: "PHP 5.5 & MySQL 5.5 CI error" -> no idea what this means or what to do about it.

EDIT: "PHP 5.5 & MySQL 5.5 CI error" -> no idea what this means or what to do about it.

The last time this happened to me, the unit test didn't have a @group anotation. Looks like this could be the issue here.

EDIT: Thanks @almaudoh!

If you don't have a Kernel present, then yay everything works.

Only since the patch above.
Before, even this did not work, thanks to the dependence on php include path.
The patch in #75 fixes two bugs and introduces a test that is meaningful and was previously not possible.
Imo, this would already justify a commit. After some cleanup, obviously.

But I don't mind if we want to go the next step and also fix the Kernel / site dir issue. I am looking into it as we speak.

I really like this test. Its way less bound to the internal implementation.

Just some cleanup we should do, but I couldn't be bother with posting a review. There is nothing to disagree with this changes, IMHO.

I am still working on the next version of this patch, with more coverage of the weird edge cases.
Let's keep this as "Needs work" until then.

We can still decide to move some of this to a new issue. But give me a few hours or a day and then we talk :)

I made another observation, which we should think about for a bit:
All the search directories are relative to Drupal root. But the test site directory is a stream url, e.g. 'vfs://root/sites/simpletest/739048'.
If we prepend the root path (fake or not) to this site path, we get a broken path.

This applies to this proposed change:

@@ -478,7 +478,7 @@ protected function scanDirectory($dir, $include_tests) {
       else {
         $filename = $name . '.' . $type;
       }
-      if (!file_exists(dirname($pathname) . '/' . $filename)) {
+      if (!file_exists($this->root . '/' . dirname($pathname) . '/' . $filename)) {
         $filename = NULL;
       }

But also to existing code here:

  protected function scanDirectory($dir, $include_tests) {
    [..]
    $absolute_dir = ($dir == '' ? $this->root : $this->root . "/$dir");

    if (!is_dir($absolute_dir)) {
      return $files;
    }

The effect is that in current code, extensions in the test site directory will not be found, because the is_dir() returns FALSE.

I am going to add a kernel test to document this behavior. So that in the follow-up, we have to change this kernel test. This will illustrate how we are changing the behavior.

\Drupal is an anti-pattern.

Absolutely.

True even without the backslash? (running for cover)

In a kernel test, the root path would be '/path/to/drupal'.
The check would be is_dir('/path/to/drupal/vfs://root/sites/simpletest/12345'), so it would fail.

The only thing we could inject to make this work is an empty root path.
But this virtual site directory does usually not contain any modules anyway.

Btw the correct way to fix the site path lookup would be like this:

    if ($this->sitePath) {
      $searchdirs[static::ORIGIN_SITE] = $this->sitePath;
    }
    elseif (\Drupal::hasService('kernel')) {
      $searchdirs[static::ORIGIN_SITE] = \Drupal::service('site.path');
    }
    else {
      $searchdirs[static::ORIGIN_SITE] = DrupalKernel::findSitePath(Request::createFromGlobals());
    }

In your version, you skip the \Drupal::hasService().

I will not fix this part here, but instead provide a test that documents that the existing behavior ignores the injected site path.

-    static::assertFileExists($root . '/core/modules/system/system.module');
-    static::assertFileExists($root . '/core/modules/system/system.info.yml');
+    $this->assertFileExists($root . '/core/modules/system/system.module');
+    $this->assertFileExists($root . '/core/modules/system/system.info.yml');

My IDE always tells me to use static::assert*() here. The methods are indeed static in PhpUnit.
I don't know what is better. Do we have a policy about this?

-  private function populateFilesystemStructure(array &$filesystem_structure) {
-
+  protected function populateFilesystemStructure(array &$filesystem_structure) {

What's the deal with protected everywhere? People (neither core nor contrib) are not supposed to inherit from this, and we don't want to promise that it won't change.

+      $content_by_file[$file] = Yaml::dump($info);

Thank you very much :)

--------------

New patch still contains the static::
But in the end I don't care that much.

New patch contains a lot more test method which cover noticeable behavior of the ExtensionDiscovery class.
We can decide to move some of this to a separate issue. But imo we should simply commit it now.

I forgot to mention, I added this:

@@ -236,6 +236,9 @@ public function scan($type, $include_tests = NULL) {
    */
   public function setProfileDirectoriesFromSettings() {
     $this->profileDirectories = array();
+    if (!function_exists('drupal_get_profile')) {
+      throw new \RuntimeException('Function drupal_get_profile() does not exist.');
+    }

and the respective test:

+  /**
+   * Tests what happens if ->scan('module') is called without prior
+   * ->setProfileDirectories(), outside a functioning Drupal environment.
+   *
+   * If this behavior is to change in the future, this test needs to be updated.
+   *
+   * @covers ::scan
+   *
+   * @expectedException \RuntimeException
+   */
+  public function testMissingSetProfileDirectories() {
+
+    // Create an ExtensionDiscovery with a non-existing root directory.
+    $extension_discovery = new ExtensionDiscovery('FAKE_ROOT', FALSE, NULL, 'sites/default');
+
+    $extension_discovery->scan('module', FALSE);
+  }

I don't know if we want to throw this exception, and if doing so is within scope.
I also don't know if we want this kind of test.
I think it is useful to see in the testing code if we intentionally change behavior.

So let's just go with #79, please, because it satisfies the scope of this issue and is reviewable. And so we can then a) have a baseline with tests, and b) move forward on other ideas in other issues.

I am fine with this.

I disagree with some of your other ideas.
This is only for the sake of discussion, and should not stop the patch going forward.

I think that if you make a change you should cover it with a test.

I think when you introduce new functionality, you should add new tests to cover it, and to detect when the behavior changes.
When you change functionality, there should already be an existing test which expresses the old behavior, and which you then change to reflect the new behavior. Or you say "oops", if you notice that the change is not intended.

The tests I proposed here should have been written when ExtensionDiscovery were first introduced. But they were not. So we either add them here, or in a follow-up.

The proposed tests are still incomplete. There are still cases that are "legitimate use", but that are not covered by tests. So yes, a follow-up is the place.

Having the class explode if it's used without an include file is regression.

The exception is thrown in a case where we previously had a fatal error. So not really a regression.
But it is a change which maybe we don't want. Because if we do this, we would also have to do it in other places. So I can live with not doing it.
The benefit of an exception is that it can be tested. But it is not super-important.

I'm also sure the goal here is to have ExtensionDiscovery do its work without any dependencies, including bootstrap.inc which is where drupal_get_profile() lives.

It is not the goal of this issue, and was apparently not the goal when ExtensionDiscovery was introduced.
The goal here (and/or in a possible follow-up) is to reflect the existing behavior with tests, even if it is awkward.

If someone makes ExtensionDiscovery work for this scenario outside of a functioning Drupal environment, the test will fail and will need to be changed. The commit will then illustrate that this problem was fixed.

Re-posting #79 by @dawehner.

Setting to RTBC via #92 (Mile23).

Committed #79 97d1fae and pushed to 8.1.x and 8.2.x. Thanks!