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:
- 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. - The third parameter then contains a
vfs://drupal_root/…
prefix: this is the actual app root!
- The first parameter passes
-
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 3076797-26.patch | 24.96 KB | Wim Leers |
#26 | interdiff.txt | 1.41 KB | Wim Leers |
#18 | 3076797-18.patch | 25.2 KB | Wim Leers |
#18 | interdiff.txt | 1.44 KB | Wim Leers |
Comments
Comment #2
Wim LeersThis implements my proposal: to add
assert()
toExtension
'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.Comment #3
Wim LeersAdded a second example to the issue summary and fixed the title.
Comment #5
Wim LeersHah! 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:Let's fix those first.
Comment #6
Wim LeersThis 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.)
Comment #9
Wim LeersThis fixes all tests except the ones that I found to be broken/wrong in #3065545: Deprecate base theme fallback to Stable.
Comment #11
Wim LeersOne more pre-existing bug in
ModuleHandlerTest
… 😨Comment #13
Wim LeersThis 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.Comment #15
Wim LeersAll of the remaining failures look like this:
Which confirms this paragraph that I wrote in the issue summary:
Comment #16
Wim LeersIncreasing priority to #3065545: Deprecate base theme fallback to Stable.
because this blocksComment #17
lauriiiGood 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.
Is there a particular reason for having this exception on this assert? Wouldn't this pathname pass the validation?
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).Comment #18
Wim Leersassert()
s from the exceptional case, but AFAICT this should work too.Comment #19
lauriiiThank you! Changes and the change record both look good to me.
Comment #20
alexpottWe 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.
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.
Comment #21
Wim Leersparse_url()
call is the most expensive one and that's implemented in C. Thefile_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.new Extension(
incore/lib
).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 anExtension
object only if it gets past this code:… this shows that the
is_dir()
check must pass, otherwise noExtension
object is created. So if usingassert(file_exists($root . '/'. $pathname))
truly were a problem, then on Windows installations Drupal would also be unable to discover extensions!Comment #22
andypostComment #23
Wim LeersI explained in #21 why only the first
assert()
is CPU-bound, and how the secondassert()
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.Comment #24
alexpottI think we could change this to
I think the second part of this is covered by the following assert.
Comment #25
andypost@Wim extension objects created mostly every request, so file exists will cause file stat on info files for no reason
Comment #26
Wim Leers#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:
Comment #27
lauriiiI liked the more verbose feedback in #18 but I'm fine with #26 as well. 👍
Comment #28
alexpottCommitted 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.
Comment #30
Wim LeersA backport to Drupal 8.7 is not needed 👍