"core_version_requirement key must be present" on core modules on Windows

I would say this is caused by ✨ Introduce database driver extensions and autoload database drivers' dependencies Fixed , with the introduction of a new getModuleInfo() function.

There is a DIRECTORY_SEPARATOR used, which is \ on windows, but in the InfoParserDynamic class there is still / hardcoded (see /core/):

      if (!isset($parsed_info['core_version_requirement'])) {
        if (str_starts_with($filename, 'core/') || str_starts_with($filename, $this->root . '/core/')) {
          // Core extensions do not need to specify core compatibility: they are
          // by definition compatible so a sensible default is used. Core
          // modules are allowed to provide these for testing purposes.
          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
        }
      }

So the path D:\htdocs\d10test\core/modules/mysql/mysql.info.yml does not pass on check str_starts_with($filename, $this->root . '/core/').

Let's try a patch which simply changes the hardcoded slash for DIRECTORY_SEPARATOR.

Not sure if we can test this windows-only issue somehow, so will keep this NR for other feedback. Thanks!

Tested the patch also manually and it fixed the error.

Anyway to get a test case showing the issue added?

@shweta__sharma can I ask what that tag is meant for?

Thanks!

I briefly searched the issue queue for Windows issues from the past, but was unable to figure out if/how to test this windows-only issue in our tests (if it is possible, any help will be appreciated and feel free to re-set the Needs tests tag again). Therefore also upon the discussion with @smustgrave on Slack, I am moving this back to Needs review.

I am also changing the priority to major, as this bug prevents Drupal 10.2.x from being installed on Windows.

Thanks for digging @poker10. Going to mark then.

As a Windows user, I can confirm the problem and the patch solving the problem.

(In fact I came up independently with the exact same one to get core installing and/or running a test. Just wasn't sure it was something that would be eligible for an official patch on here, since Windows only)

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -51,7 +51,7 @@ public function parse($filename) {
+        if (str_starts_with($filename, 'core/') || str_starts_with($filename, $this->root . DIRECTORY_SEPARATOR . 'core/')) {

Possibly silly question - why doesn't the trailing slash also need to use directory separator?

We've got an issue open to drop support for Windows on production, but even if that gets agreed, issues like this would still be valid fwiw.

Possibly silly question - why doesn't the trailing slash also need to use directory separator?

I would say that this is caused by the fact, that the paths to extensions are build by ExtensionDiscovery class. This class has scanDirectory() method, which is building the $pathname for each Extension object (this $pathname is used in InfoParserDynamic::parse() calls subsequently).

And the scanDirectory() method is using this code:

    $absolute_dir = ($dir == '' ? $this->root : $this->root . "/$dir");

    // Use Unix paths regardless of platform, skip dot directories, follow
    // symlinks (to allow extensions to be linked from elsewhere), and return
    // the RecursiveDirectoryIterator instance to have access to getSubPath(),
    // since SplFileInfo does not support relative paths.
    $flags = \FilesystemIterator::UNIX_PATHS;
    $flags |= \FilesystemIterator::SKIP_DOTS;
    $flags |= \FilesystemIterator::FOLLOW_SYMLINKS;
    $flags |= \FilesystemIterator::CURRENT_AS_SELF;
    $directory_iterator = new \RecursiveDirectoryIterator($absolute_dir, $flags);

See the hardcoded forward slash in $absolute_dir. Because of that, you will end up with paths like these D:\htdocs\d10test/core/modules/action/action.info.yml from the *SplFileInfo*pathName.

The first part (D:\htdocs\d10test) is taken from $this->root, which is based on \Drupal::root(), which is computed by dirname(), which returns a path with backslashes on Windows.

The second part (core/modules/action/action.info.yml) is taken from RecursiveDirectoryIterator, which has forced unix slashes.

And yes, the ExtensionDiscovery::scanDirectory() is using only this for $pathname:

$pathname = $dir_prefix . $fileinfo->getSubPathname();

So it will be core/modules/mysql/mysql.info.yml.

But the new DatabaseDriver::getModuleInfo() is adding $this->root as well:

  private function getModuleInfo(): void {
    if (!isset($this->info)) {
      $infoParser = new InfoParser($this->root);
      $this->info = $infoParser->parse($this->root . DIRECTORY_SEPARATOR . $this->getModule()->getPathname());
    }
  }

$this->root is D:\htdocs\d10test
DIRECTORY_SEPARATOR is \
$this->getModule()->getPathname() is core/modules/mysql/mysql.info.yml (see the previous comment)

So it seems to me that this is causing the paths with backwards and forwards slashes.

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

Know random test RandomTest failure 🐛 Random test fail in Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness Needs work (and yes, I have been waiting weeks to make that crappy joke), back to RTBC.

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

Believe to be unrelated

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

It would be really nice to get this one in, so I don't have to apply it manually every time I want to install core or run a test locally

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -51,7 +51,7 @@ public function parse($filename) {
-        if (str_starts_with($filename, 'core/') || str_starts_with($filename, $this->root . '/core/')) {
+        if (str_starts_with($filename, 'core/') || str_starts_with($filename, $this->root . DIRECTORY_SEPARATOR . 'core/')) {

The mix of hard coded / and DIRECTORY_SEPARATOR feels really icky here. Really hard to explain. Like If we changed it to
str_starts_with($filename, $this->root . DIRECTORY_SEPARATOR . 'core' . DIRECTORY_SEPARATOR it would break again on windows.

I wonder if instead we should be changing \Drupal\Core\Extension\DatabaseDriver::getModuleInfo() instead to do:

    if (!isset($this->info)) {
      $infoParser = new InfoParser($this->root);
      $this->info = $infoParser->parse($this->getModule()->getPathname());
    }

As that would be inline with other calls to \Drupal\Core\Extension\InfoParserDynamic::parse() for extension listing services.

I wonder why $filename is sometimes an absolute path.

I think it'd be good to see if we could somehow make this look a bit better.

I had exactly the same question as #10, and @poker10 explained the reasoning quite well. Adding credit for @catch for posing a helpful question, and @Spokje for jokes. (It's actually for the manual testing, but let's pretend it's the joke.)

Committed to 11.x, and cherry-picked to 10.1.x as a patch-eligible, major-borderline-critical bugfix.

Thanks everyone!

I think this change might cause more problems though. Because maybe some other custom code that is running on windows as worked around the code as it is Drupal 10.0.x. And now it fails. And the mixture of slashes here is likely to cause more bugs in the future. I think the real issue here is that we should have fixed the new code in \Drupal\Core\Extension\DatabaseDriver::getModuleInfo() as per #20. I thought I'd set this one to needs work. The only reason we accept a full path in parse() is for testing. Which I might have introduced and now regret.

I think we should revert this from 10.1.x and commit the following patch to 11.x for the quick fix. And then open a follow-up to discuss how we should improve \Drupal\Core\Extension\InfoParserDynamic::parse() so that the paths are more filesystem agnostic.

As an example of stuff that would pass on Windows before this change but now will not see core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php where we do
$file_name = DRUPAL_ROOT . '/core/modules/system/tests/themes/' . $name . '/' . $name . '.info.yml';

If #25 works that looks fine as a quick fix. Definitely we should try to minimise the mix of slashes, it's very confusing just reading it, let alone trying to figure out what the practical effects might be.

The last submitted patch, 25: 3383616-25.patch, failed testing. View results →

So #25 is failing because PHPUnit tests don't set the working directory to Drupal root but extension discovery including now database driver discovery assumes that you are in Drupal root. There are other unit tests that set the working directory for this reason so UrlConversionTest can do the same. I think we might consider throwing an exception in \Drupal\Core\Extension\DatabaseDriver::getModuleInfo() if $this->info is an empty array as that means an info file does not exist which is not really valid.

+++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
@@ -27,6 +27,8 @@ class UrlConversionTest extends UnitTestCase {
+    // This unit relies on reading files relative to Drupal root.

This unit test maybe? Otherwise it sounds like a robot's version of first person.

Is #29 intended for both 11.x and 10.1.x? (And wouldn't it have been better to just revert it first anyway?)

Thanks all for your effort with this issue! I have tested the patch #29 and it seems to work on Windows as the patch #2. Given the severity of this (unable to install or update 11.x branch on Windows), I think that it would be good to have one of these quick fixes committed (#2 or this new approach). @alexpott raised some concerns about the patch #2 in #24 🐛 "core_version_requirement key must be present" on core modules on Windows Needs review , so in that case it would probably be safer to commit the later one.

Is #29 intended for both 11.x and 10.1.x? (And wouldn't it have been better to just revert it first anyway?)

The issue which caused this was committed to 11.x branch only: ✨ Introduce database driver extensions and autoload database drivers' dependencies Fixed , so I think it should be sufficient to commit the fix to the 11.x. And the cleanest way probably will be to revert both commits and commit the patch I am uploading now - it is the same as #29, but without the code being reverted (as it will be reverted separately).

This unit test maybe? Otherwise it sounds like a robot's version of first person.

I have also updated this comment in the uploaded patch.

I think we might consider throwing an exception in \Drupal\Core\Extension\DatabaseDriver::getModuleInfo() if $this->info is an empty array as that means an info file does not exist which is not really valid.

@alexpott Do you mean here, or in the follow-up?

Thanks!

@alexpott Do you mean here, or in the follow-up?

Definitely a follow-up.

Yep we can revert #2 from 10.1.x and 11.x and then commit #31. I'll do the reverts and then someone can rtbc this.

Per #32

Committed/pushed to 11.x, thanks!

I think this still needs that followup filed?

Created 🐛 InfoParser returns an empty array if passed a non-existing file Fixed and 🐛 InfoParserDynamic::parse() looks for strings in a file path without normalising Active as followups.

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