Problem/Motivation

Over at #3065545: Deprecate base theme fallback to Stable, we need to keep update path test coverage for functionality that is being deprecated in that issue.

Because extension discovery happens very early in the installer, the deprecations are triggered during extension discovery. The only way to avoid this was by using $settings['file_scan_ignore_directories'], which was not acceptable. So we set out to update the existing test coverage to not use a physical on-disk file (to avoid extension discovery discovering it and then triggering a deprecation error), but one that is stored in vfs.

In doing that, I stumbled upon some remarkable bugs in the existing test coverage: even though some of it uses vfs already, it apparently is mostly working by accident, because the test coverage just happens to be not exercising certain code paths.

It turns out that even though \Drupal\Core\Extension\Extension is aware of the app root it operates in, it does not respect this for file system operations. Which means that as soon as you call Extension::getMTime() for example (which Drupal core does), it always assumes PHP's current working directory instead of the specified app root. This works fine outside of tests (since PHP's working directory matches the app root), but it fails in case of unit tests.

  • For example, \Drupal\Tests\Core\Extension\ExtensionListTest::setupTestExtensionList() does:
    $extension_scan_result[$extension_name] = new Extension($this->root, 'test_extension', "vfs://drupal_root/example/$extension_name/$extension_name.info.yml");
    

    This contains multiple bugs:

    1. The first parameter passes $this->root. This is provided by \Drupal\Tests\UnitTestCase::setUp() and set to $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)))); — so something like /Users/wim.leers/Work/d8. This is obviously not a virtual file system app root.
    2. The third parameter then contains a vfs://drupal_root/… prefix: this is the actual app root!
  • Analogously, \Drupal\Tests\Core\Extension\ThemeExtensionListTest::testRebuildThemeDataWithThemeParents() does:
    'test_subtheme'  => new Extension($this->root, 'theme', $this->root . '/core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml', 'test_subtheme.info.yml'),
    

    While less obviously broken, this is also still wrong, because it is saying that it lives at /Users/wim.leers/Work/d8/Users/wim.leers/Work/d8/core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml: the app root is repeated.

This combination has happened to work okay so far because all extensions that were ever actually installed were not stored in vfs://. #3065545: Deprecate base theme fallback to Stable is the first to instal an extension from the virtual file system and is hence running into this bug.

Proposed resolution

I first thought we could not fix this separately from #3065545 because the bug only exists in test coverage. And writing test coverage for a test makes no sense.

However, I see an easier path forward now: adding development time-only validation logic by adding assert() statements to the \Drupal\Core\Extension\Extension constructor.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.04 KB

This implements my proposal: to add assert() to Extension's constructor.

This should trigger failures in at least \Drupal\Tests\Core\Extension\ExtensionListTest and \Drupal\Tests\Core\Extension\ThemeExtensionListTest. It will also surface bugs elsewhere — I see \Drupal\Tests\Core\Extension\ThemeHandlerTest::testThemeLibrariesEmpty() is wrong too for example.

Wim Leers’s picture

Title: \Drupal\Core\Extension\Extension » \Drupal\Core\Extension\Extension's absence of validation has allowed multiple incorrect tests to be added
Issue summary: View changes

Added a second example to the issue summary and fixed the title.

Status: Needs review » Needs work

The last submitted patch, 2: 3076797-2.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
1.19 KB

Hah! Apparently there are even more invalid Extension objects being created than I thought! 😲

Many of those failures are caused by \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName() which does:

    if (empty($themes) || !$theme_name || !isset($themes[$theme_name])) {
      $theme_name = 'core';
      // /core/core.info.yml does not actually exist, but is required because
      // Extension expects a pathname.
      $active_theme = $this->getActiveTheme(new Extension($this->root, 'theme', 'core/core.info.yml'));

      // Early-return and do not set state, because the initialized $theme_name
      // differs from the original $theme_name.
      return $active_theme;
    }

Let's fix those first.

Wim Leers’s picture

This fixes the tests that contain new Extension(… and resulted in test failures when I ran them locally.

(This should fix many of the other failures, but I'm keeping those from #3065545: Deprecate base theme fallback to Stable for last.)

The last submitted patch, 5: 3076797-5.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 3076797-6.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
15.16 KB

This fixes all tests except the ones that I found to be broken/wrong in #3065545: Deprecate base theme fallback to Stable.

Status: Needs review » Needs work

The last submitted patch, 9: 3076797-9.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
15.95 KB

One more pre-existing bug in ModuleHandlerTest… 😨

Status: Needs review » Needs work

The last submitted patch, 11: 3076797-11.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.98 KB
24.87 KB

This then finally fixes the wrong uses of \Drupal\Core\Extension\Extension that were discovered in #3065545: Deprecate base theme fallback to Stable and which caused this issue to be opened.

Status: Needs review » Needs work

The last submitted patch, 13: 3076797-13.patch, failed testing. View results

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
715 bytes
25.24 KB

All of the remaining failures look like this:

RuntimeException: SplFileInfo::getMTime(): stat failed for core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml

Which confirms this paragraph that I wrote in the issue summary:

It turns out that even though \Drupal\Core\Extension\Extension is aware of the app root it operates in, it does not respect this for file system operations. Which means that as soon as you call Extension::getMTime() for example (which Drupal core does), it always assumes PHP's current working directory instead of the specified app root. This works fine outside of tests (since PHP's working directory matches the app root), but it fails in case of unit tests.

Wim Leers’s picture

Priority: Normal » Major

Increasing priority to Major because this blocks #3065545: Deprecate base theme fallback to Stable.

lauriii’s picture

Good job on finding a way to test this. 👏 I think it was the right decision to make this a separate issue since both of the issues are much easier to review after separation.

  1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
    @@ -63,6 +63,9 @@ class Extension {
    +    assert($pathname === 'core/core.info.yml' || $pathname[0] !== '/' && array_key_exists('path', parse_url($pathname)) && count(array_keys(parse_url($pathname))) === 1, 'The relative path and filename is invalid. A path relative to the Drupal root is expected, for example: "core/modules/node/node.info.yml".');
    

    Is there a particular reason for having this exception on this assert? Wouldn't this pathname pass the validation?

  2. +++ b/core/lib/Drupal/Core/Extension/Extension.php
    @@ -63,6 +63,9 @@ class Extension {
    +    assert($pathname === 'core/core.info.yml' || file_exists($root . '/' . $pathname), sprintf('The file specified by the given app root, relative path and file name (%s) do not exist.', $root . '/' . $pathname));
    

    Looking at the changes needed in core (Drupal\Tests\Core\Layout\LayoutPluginManagerTest in particular), I'm a bit concerned that we might break some tests outside of core as well. I'm wondering if we should have at least a change record saying that we are now validating this (in development environments).

Wim Leers’s picture

  1. ✅ I think you're right! I just exempted both assert()s from the exceptional case, but AFAICT this should work too.
  2. ✅ Created change record: https://www.drupal.org/node/3077623
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Changes and the change record both look good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
    @@ -63,6 +63,9 @@ class Extension {
    +    assert($pathname[0] !== '/' && array_key_exists('path', parse_url($pathname)) && count(array_keys(parse_url($pathname))) === 1, 'The relative path and filename is invalid. A path relative to the Drupal root is expected, for example: "core/modules/node/node.info.yml".');
    +    // @see \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName()
    +    assert($pathname === 'core/core.info.yml' || file_exists($root . '/' . $pathname), sprintf('The file specified by the given app root, relative path and file name (%s) do not exist.', $root . '/' . $pathname));
    

    We construct a lot of these objects. In a lot places. I'm concerned that this might affect test performance. Also I'm concerned about Windows and its different directory separators.

  2. +++ b/core/lib/Drupal/Core/Extension/Extension.php
    @@ -155,7 +158,7 @@ public function load() {
    -      $this->splFileInfo = new \SplFileInfo($this->pathname);
    +      $this->splFileInfo = new \SplFileInfo($this->root . '/' . $this->pathname);
    

    Is this change necessary? Ah I see #15 - yeah this makes sense and I can see how this has not had test coverage or any actually bad effects so far. I think the use of '/' is fine here. PHP will convert must things correctly to Windows paths at a lower level. The reason I'm concerned with the asserts above is that I'm not sure where all the paths come from and on Windows these might have Windows directory separators and not nix.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
  1. Interesting observations!
    1. I am not at all concerned about test performance. The parse_url() call is the most expensive one and that's implemented in C. The file_exists() one is the only one that touches the file system. I'm assuming this is the one you're most concerned about. But this is happening at times when the file system is being touched a lot anyway (to discover extensions in the first place, and then parse them), which means either the operating system-level file system cache either has already been primed or this primes it.
    2. I agree we construct a lot these objects: one per extension. But the same I/O argument as above applies: that's far more expensive than this additional check.
    3. I disagree we construct it in a lot of places. Unless you're talking about tests. Yes, we construct it in a lot of kernel and unit tests. But other than that, we only construct it in 7 places (search for new Extension( in core/lib).
    4. The Windows concern definitely is the most interesting remark! To answer this, we need to look at the non-test Extension instantiations versus the test instantiations. For the test ones, most of them use $this->root, which is set to $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));, which means that Windows-style directory separators have already been present in there all this time and they've worked fine (for example: \Drupal\Tests\ComposerIntegrationTest::testComposerLockHash() appends /composer.json to $this->root and the reads the file contents — many more examples can be found). For the non-test ones, let's look at \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory(). This creates an Extension object only if it gets past this code:
          $absolute_dir = ($dir == '' ? $this->root : $this->root . "/$dir");
      
          if (!is_dir($absolute_dir)) {
            return $files;
          }
      …
          $extension = new Extension($this->root, $type, $pathname, $filename);
      

      … this shows that the is_dir() check must pass, otherwise no Extension object is created. So if using assert(file_exists($root . '/'. $pathname)) truly were a problem, then on Windows installations Drupal would also be unable to discover extensions!

    5. The last RTBC patch test run for #18 took 55 minutes, the last HEAD test run before it took 58 minutes.
  2. 👍
andypost’s picture

Wim Leers’s picture

Issue tags: -needs profiling

I explained in #21 why only the first assert() is CPU-bound, and how the second assert() is either priming a operating-level file system cache or hitting a primed cache.

So, profiled the first assert(): https://3v4l.org/F8tfF/perf#output — 0.02 seconds for 1000 extensions on PHP 7.1, 0.015 seconds for 1000 extensions on PHP 7.4.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -63,6 +63,9 @@ class Extension {
+    assert($pathname[0] !== '/' && array_key_exists('path', parse_url($pathname)) && count(array_keys(parse_url($pathname))) === 1, 'The relative path and filename is invalid. A path relative to the Drupal root is expected, for example: "core/modules/node/node.info.yml".');

I think we could change this to

assert($pathname[0] !== '/', 'The path is invalid. A path relative to the Drupal root is expected, for example: "core/modules/node/node.info.yml".');

I think the second part of this is covered by the following assert.

assert($pathname === 'core/core.info.yml' || file_exists($root . '/' . $pathname), sprintf('The file specified by the given app root, relative path and file name (%s) do not exist.', $root . '/' . $pathname));
andypost’s picture

@Wim extension objects created mostly every request, so file exists will cause file stat on info files for no reason

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.41 KB
24.96 KB

#24: as discussed in Slack: 👍

#25: But assertions are turned off on production sites, meaning there is zero impact for production sites :) That's why I wrote this in the issue summary: […] adding development time-only validation logic by adding assert() […]

lauriii’s picture

I liked the more verbose feedback in #18 but I'm fine with #26 as well. 👍

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed afcdeee and pushed to 8.8.x. Thanks!

I don't think we need to backport this to 8.7.x as we'll get the benefit there without having to backport there as everything has to go to 8.8.x first and given how other changes to Extension objects have panned out in the past I'm slightly burned by making changes to this.

  • alexpott committed afcdeee on 8.8.x
    Issue #3076797 by Wim Leers, lauriii, alexpott: \Drupal\Core\Extension\...
Wim Leers’s picture

A backport to Drupal 8.7 is not needed 👍

Status: Fixed » Closed (fixed)

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