Windows vs POSIX

[Boegie]

When creating a new baseline on a Windows OS machine using vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist --generate-baseline ./core/phpstan-baseline.neon on drupal/core with (at least) 1.1.16 I get this suppression:

		-
			message: "#^File core/modules/system/tests/modules/entity_test\\\\entity_test\\.inc could not be loaded from Drupal\\\\Core\\\\Extension\\\\ModuleHandlerInterface\\:\\:loadInclude$#"
			count: 2
			path: modules/system/tests/modules/entity_test/entity_test.install

on a POSIX machine I get:

		-
			message: "#^File core/modules/system/tests/modules/entity_test/entity_test\\.inc could not be loaded from Drupal\\\\Core\\\\Extension\\\\ModuleHandlerInterface\\:\\:loadInclude$#"
			count: 2
			path: modules/system/tests/modules/entity_test/entity_test.install

This leads to sad TestBots and sad users.

[mglaman]

Hm. I'm not sure what to do here: https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/ModuleLoadInclude.php#L40

        try {
            // Try to invoke it similarly as the module handler itself.
            [$moduleName, $filename] = $this->parseLoadIncludeArgs($args[1], $args[0], $args[2] ?? null, $scope);
            $module = $this->extensionMap->getModule($moduleName);
            if ($module === null) {

Which is from

    protected function parseLoadIncludeArgs(Node\Arg $module, Node\Arg $type, ?Node\Arg $name, Scope $scope): array
    {
        $moduleName = $this->getStringArgValue($module->value, $scope);
        if ($moduleName === null) {
            return [];
        }
        $fileType = $this->getStringArgValue($type->value, $scope);
        if ($fileType === null) {
            return [];
        }
        $baseName = null;
        if ($name !== null) {
            $baseName = $this->getStringArgValue($name->value, $scope);
        }
        if ($baseName === null) {
            $baseName = $moduleName;
        }

        return [$moduleName, "$baseName.$fileType"];
    }

[Boegie]

@mglaman I think for this case we're looking at https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/LoadIncludes.php#L73, but the principle seems to be the same.

For reasons, I as a mere mortal don't understand, on Windows OS, the dirname() in in $module->getPath() (See

phpstan-drupal/src/Drupal/Extension.php

Line 121 in 2453a2e

return dirname($this->pathname);

) returns a path with / as directory separator (hence the core/modules/system/tests/modules/entity_test-bit in the message), but the actual DIRECTORY_SEPARATOR in line https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/LoadIncludes.php#L73 always returns a \ (which seems to gets double-escaped and ends up as a \\\\ in the message).
That leaves us with the worst of both worlds.

I would replace the DIRECTORY_SEPARATOR with an actual /, and I think that's a pattern that's used more throughout the code (and should be replaced more if my theory is correct).

In fact I see that you're already doing so in lines like this

phpstan-drupal/src/Drupal/Extension.php

Line 157 in 2453a2e

return $this->getPath() . '/' . $this->filename;

and

phpstan-drupal/src/Drupal/Extension.php

Line 182 in 2453a2e

include_once $this->root . '/' . $this->getPath() . '/' . $this->filename;

for example.

Does that make any sense?

[mglaman]

@Boegie makes sense! How does #417 look? PHP does some fancy fixes of mixed / or \ and DIRECTORY_SEPARATOR. For the fix, I just made sure we do not use DIRECTORY_SEPARATOR in the error message.

[Boegie]

@mglaman Works for me :)