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

Anyway to get a test case showing the issue added?

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

Thanks!

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)

So RTBC+1 from me.

+++ 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.

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

Believe to be unrelated

Known random test failure, back to RTBC.

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.

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?)

@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 #3391776: InfoParser returns an empty array if passed a non-existing file and #3391778: InfoParserDynamic::parse() looks for strings in a file path without normalising as followups.