Cached ClassMap entries not reset after registering new prefix

[rullzer]

We use the composer autoloader to load our core classes. As well as load classes in enabled apps. For the core we generate a classmap. But the enabled apps are currently added on the fly by calling $autoloader->addPsr4($prefix, $path).

Now we have the following situation

// The prefix Foo is not yet registered
...
// Try to load \Foo\Bar
try {
  $tmp = new \Foo\Bar();
} catch (\Exception $e) {
  //Do something else
}
...
//Install app Foo
$autoloader->addPsr4('Foo', $dir);
...
// Foo\Bar should now be there
$bar = new \Foo\Bar();

This will result in an error because the autoloader has already cached and thus return false (see https://github.com/composer/composer/blob/master/src/Composer/Autoload/ClassLoader.php#L323)

I think it would make sense to clear all cached to false item that match the prefix. Since they might be there now.

[alcohol]

The classmap property you link to does not store this information anymore. Recent versions of the composer ClassLoader have a missingClasses property instead.

However, what you are trying to do is currently not possible with the Composer autoloader. I cannot imagine a valid use-case for adding extra namespaces etc. on the fly. This is probably why the above situation is not supported.

[nickvergessen]

Test case would be:

    public function testAddPsr4ResetClassMap()
    {
        $loader = new ClassLoader();
        $this->assertFalse($loader->findFile('ShinyVendor\ShinyPackage\SubNamespace\Foo'), "->findFile() finds class 'ShinyVendor\\ShinyPackage\\SubNamespace\\Foo'");
        $loader->addPsr4('ShinyVendor\\ShinyPackage\\', __DIR__ . '/Fixtures');
        $this->assertSame(__DIR__ . '/Fixtures', $loader->findFile('ShinyVendor\ShinyPackage\SubNamespace\Foo'));
    }

Fix could be something like:

    protected function resetClassMapAfterAdd($prefix)
    {
        foreach ($this->classMap as $class => $file) {
            if (strpos($class, $prefix) === 0 && $file === false) {
                unset($this->classMap[$class]);
            }
        }
        foreach ($this->missingClasses as $class) {
            if (strpos($class, $prefix) === 0) {
                unset($this->missingClasses[$class]);
            }
        }
    }

I cannot imagine a valid use-case for adding extra namespaces etc. on the fly.

Well in the case of Nextcloud/ownCloud we only add the namespace once the app was loaded and we verified it should be enabled for the user.

[alcohol]

I do not understand your use-case. Since you already know what the user has access to (I presume you do, since you speak of verifying), you can just configure all of this properly up front. No need to do magic on the fly.

[rullzer]

Ah let me explain in a bit more detail then.

We have an app that extends theming capabilities. We load theming capabilities on each request. But if you enable the new app we need to register some stuff.

So it goes like:

1 We request theming
2. Check if 'App\Theming' class is there if not fall back to 'Core\BasicTheming'
3. Enable 'App\Theming'
4. Regsiter PSR4 dir
5. Register 'App\Theming` into settings etc
6. This tries to load 'App\Theming' which fails because in an earlier stage the file was not there

It is an edge case I agree

[alcohol]

Why not just redirect the user after enabling it? A new request should start with a new autoloader and therefor properly pick up the changes.

[rullzer]

But why allow new prefixes to be added if you are not going to invalidate the cache?

I get that from a performance perspective it is less ideal but maybe then a function to do it manually? Like flushMissingClasses?

[alcohol]

You are not supposed to modify the autoloader after you have registered it.

[Seldaek]

@rullzer I'd suggest you create a new ClassLoader instance and register it as well (you can have two just fine). This isn't really a use case we want to support for performance reason IMO, adding a method to flush the cache would be an option but it's really just clutter for an edge case as registering a new classloader works just fine.

[dawehner]

Every system which allows stuff to be registered on the fly will have issues with this. I bet wordpress, typo3, Drupal on top of the example of nextcloud above.

I'm wondering whether this should/could be really considered as an edge case.

Note: The redirect workaround doesn't work in environment like automatic tests, where you simply have nothing like HTTP available.

[Seldaek]

@dawehner see my last comment, this is working just fine for any use case IMO.

[dawehner]

Well, the singleton in getLoader doesn't make that particularly easy :)

[Seldaek]

What's wrong with new ClassLoader?

[dawehner]

Right, don't get me wrong I try to understand that.
At the moment this would require to copy the full setup of ::getLoader though, which is not nothing

[Seldaek]

No what I said is.. keep using the existing loader, and just add a new one with the new classes you want to "activate".

$loader = require 'vendor/autoload.php';

// later after loading a module..
$newLoader = new Composer\Autoload\ClassLoader;
$newLoader->addClassMap([...]);
$newLoader->register();

Now you have two classloaders, one of which will handle the newly loaded module.

[alexpott]

The problem with adding new loaders is that we have services that use the canonical loader to find plugins provided by modules. Having multiple class loaders is not really an option for Drupal.

[Seldaek]

class_exists() or Reflection is how you find classes though :/

[alexpott]

Not in \Doctrine\Common\Reflection\StaticReflectionParser() it isn't :)

[alexpott]

@Seldaek what is the harm of clearing the negative caches in \Composer\Autoload\ClassLoader::$classMap after calling anything that adds to the ability of the class loader to find things?

[stof]

@alexpott the StaticReflectionParser should be initialized with a finder object dealing with all registered autoloaders then, not a single autoloader. And this is the responsibility of the code instantiating it.
Building such a finder is easy, as it could simply be a ChainFinder delegating to many other finders.