[BUG] FilteredComposerIterator doesn't find classes in PSR4 fallback dirs

[josefsabl]

Hello, I am (as it seems) end user of this library as a dependency of The Coding Machines GraphQLite library and encountered this bug: thecodingmachine/graphqlite#696.

I narrowed the problem down to this class-finder. As it seems the \Kcs\ClassFinder\Iterator\FilteredComposerIterator::searchInPsrMap method doesn't look for classes in \Composer\Autoload\ClassLoader::getFallbackDirsPsr4.

And that turned to be my case exactly.

I, indeed, have something like this in my composer.json:

  "autoload": {
    "psr-4": {
      "": "src/",
      "SomeNamespace\\": "packages/"
    }
  },

And the classes the GraphQLite looks for are, indeed, in the src directory :-)

Thank you for your library, have a nice day.

PS: If you consider this to be a feature and not a bug, I apologize about the [BUG] in a title.

[alekitto]

Hi @josefsabl,
Yes, this is clearly a bug. I've just merged a patch to fix this.

Can you please test the dev-master version on your project before I release it? Just to be sure everything is working correctly.

[josefsabl]

Wow, you are fast 😅❤️. I will check on monday.

[josefsabl]

Unfortunately it is still not working. The problem is that $this->validNamespace('') here \Kcs\ClassFinder\Iterator\FilteredComposerIterator::146 is evaluated as false and the fallback code never gets executed. My namespaces property contains exactly one namespace and it is not 'empty'.

Another problem is that when I comment out the aforementioned check your library includes all php files, that are in that directory, even those that do not contain classes but are php scripts that do something which leads to very wild results.

In all humbleness, this is pretty important side effect that should be at least highly stressed out in the documentation. I consider it to be very dangerous.

But I am sure you are aware of this 😄.

I did see this #12 and this composer/composer#6987.

[alekitto]

I see, but maybe I did not understand your problem correctly.

Could you please open a PR with a minimal reproducer?
Place it in a new folder inside data/, so I can check out what is going wrong. Thanks

[josefsabl]

I am now looking into your tests in the fixing commit. Indeed it is a misunderstanding of the problem or maybe even the fallback directory feature (?). The fallback doesn't work like that those classes don't have a namespace. It practically means that looking for namespaces start in a particular directory (src) and the next level of directories determines the first level of namespaces.

Imagine this directory structure:

src
  Namespace1
    Logger.php
  Namespace2
    Logger.php
composer.json

And this entry in composer.json:

  "autoload": {
    "psr-4": {
      "": "src/",
    }
  },

This means that the classes will be autoloaded as:

Namespace1\Logger
Namespace2\Logger

Attached class-finder-poc.zip is a minimalistic proof of concept with the demonstration of a bug.

The poc.php file contains additional explanation but it is rather simple:

<?php

declare(strict_types=1);

require_once __DIR__ . '/vendor/autoload.php';

$finder = new \Kcs\ClassFinder\Finder\ComposerFinder();

$finder->inNamespace('ExplicitNamespace'); // <= This works as expected
$finder->inNamespace('FallbackNamespace'); // <= This causes nothing to be found in fallback directory

// When not filtered by namespaces (i.e. remove the two lines above)
// It works as \Kcs\ClassFinder\Iterator\FilteredComposerIterator::validNamespace
// always returns true

foreach ($finder as $className => $reflector) {
    echo $className . PHP_EOL;
}

\Kcs\ClassFinder\Iterator\FilteredComposerIterator:146 is to blame.

It tests '' against valid namespaces, i.e. looks if it starts with any whitelisted namespace. It is empty, so it can never start with any real namespace. I.e. fallback directory can contain ANY namespace.

In fact this check is redundant and merely removing it works for me at the moment.

I also noticed another negative consequence of the side effect I mentioned earlier. Because you include the files to check if the class is there, you also include files with classes in path that doesn't correspond with their namespace.

Imagine you have a class in src/MyNamespace/MyClass.php file. Now you decide to deprecate the class or maybe write a new version and move it to src/Deprecated/MyNamespace/MyClass.php file. You also create new version in the original place. As the PSR-4 autoloading loads files by the required classes namespace, it never touches the file in Deprecated directory. So the code will work with new version. But once the finder is run, suddenly there is "Class cannot be redeclared" fatal error because it loads both files.

[alekitto]

Yes, you're right.
Now it should work, but I'm thinking on a way to not include all the files in fallback dirs.

The Class cannot be redeclared should not be be triggered if using autoloading (enabled by default), but only when using the include_once mechanism. Anyway, I don't think this a problem this library can resolve.

[alekitto]

A fix for this has been released in 0.5.4

At the moment, if a file is causing fatal error, it can be ignored via pathFilter.
If a library is known to cause such issue, we can add the files to the BogonFilesFilter regex.

I'll leave this issue open, while thinking about a way to not include all the files in the fallback dirs, but at least the fix is released.