Drop use of OwnCloud's autoloader (in favour of Composer's) for non-OwnCloud classes

[AdamWill]

By my reading the OC autoloader is now needed only for OC-specific classes that are not PSR-0 or PSR-4 compatible. The OC autoloader's handling of non-OC classes is very basic:

    foreach ($this->prefixPaths as $prefix => $dir) {
        if (0 === strpos($class, $prefix)) {
        $path = str_replace('\\', '/', $class) . '.php';
        $path = str_replace('_', '/', $path);
        $paths[] = $dir . '/' . $path;
    }

this is a not-quite-PSR-0 compliant implementation. The Composer autoloader is fully PSR-0 compliant and also PSR-4 compliant, and does a bunch of other tricks too; it's comfortably superior to the code above. I believe the bits of autoloader.php that relate to non-OC classes should be dropped, and the prefixes for any bundled libraries that aren't directly handled by Composer should be registered with Composer's autoloader (instead of the OC autoloader) in base.php .

[PVince81]

@bantu @DeepDiver1975

[bantu]

I agree.

[hutchic]

:+1:

[AdamWill]

completely untested (and incomplete, it doesn't drop the generic class handling bits of OC's autoloader yet) patch which I think is roughly what'd be needed:

diff --git a/lib/base.php b/lib/base.php
index 840d904..58c89ee 100644
--- a/lib/base.php
+++ b/lib/base.php
@@ -431,12 +431,6 @@ class OC {
        // register autoloader
        require_once __DIR__ . '/autoloader.php';
        self::$loader = new \OC\Autoloader();
-       self::$loader->registerPrefix('Doctrine\\Common', 'doctrine/common/lib');
-       self::$loader->registerPrefix('Doctrine\\DBAL', 'doctrine/dbal/lib');
-       self::$loader->registerPrefix('Symfony\\Component\\Routing', 'symfony/routing');
-       self::$loader->registerPrefix('Symfony\\Component\\Console', 'symfony/console');
-       self::$loader->registerPrefix('Patchwork', '3rdparty');
-       self::$loader->registerPrefix('Pimple', '3rdparty/Pimple');
        spl_autoload_register(array(self::$loader, 'load'));

        // make a dummy session available as early as possible since error pages need it
@@ -509,7 +503,14 @@ class OC {
        // setup 3rdparty autoloader
        $vendorAutoLoad = OC::$THIRDPARTYROOT . '/3rdparty/autoload.php';
        if (file_exists($vendorAutoLoad)) {
-           require_once $vendorAutoLoad;
+           $loader = require_once $vendorAutoLoad;
+           $loader->add('Pimple',OC::$THIRDPARTYROOT . '/3rdparty/Pimple');
+           $loader->add('Doctrine\\Common',OC::$THIRDPARTYROOT . '/3rdparty/doctrine/common/lib');
+           $loader->add('Doctrine\\DBAL',OC::$THIRDPARTYROOT . '/3rdparty/doctrine/dbal/lib');
+           $loader->add('Symfony\\Component\\Routing',OC::$THIRDPARTYROOT . '/3rdparty/symfony/routing');
+           $loader->add('Symfony\\Component\\Console',OC::$THIRDPARTYROOT . '/3rdparty/symfony/console');
+           $loader->add('Patchwork',OC::$THIRDPARTYROOT);
+           $loader->add('Pimple',OC::$THIRDPARTYROOT . '/3rdparty/Pimple');
        }

        // set debug mode if an xdebug session is active

[bantu]

@AdamWill I don't think these $loader->add() calls are necessary at all. Composer should already handle all of these paths.

[bantu]

More than require_once $vendorAutoLoad; should not be necessary. The OC::$THIRDPARTYROOT "feature" is kept this way too.

[AdamWill]

@bantu I don't see how it would handle all those paths. The components in question are not registered with Composer - check 3rdparty/composer/installed.json , neither Doctrine, nor those two Symfony components, nor Patchwork, nor Pimple, is listed.

The Composer autoloader will only autoload components that have actually been pulled in via Composer, by default. If you set $loader->setUseIncludePath(true) then if it can't find a class from the libraries it's actually managing it'll fall back to trying to find them via PSR-0 and PSR-4 conventions relative to the PHP include path, but even that would only help Patchwork (which is PSR-0 compliant relative to 3rdparty/ ). None of the other libraries listed is PSR-0 or PSR-4 compliant relative to 3rdparty/ , so they're not going to be found unless their location is taught to the autoloader.

[bantu]

@AdamWill You are correct. I assumed all of these are loaded via composer, but they are not.

[bantu]

@AdamWill If your patch from https://github.com/owncloud/core/issues/9643#issuecomment-49125536 works without any further changes to the owncloud autoloader, I think I'd like to see that as a pull request already. Everything else can be done as new PRs then.

[AdamWill]

@bantu I didn't actually test it (which is why I didn't send it as a PR), just wanted to give a starting point. As I work on distro packaging, I'm mainly working with Fedora's packaged OC, where all of this is rather different (i.e. we unbundle all that stuff and load the system wide copies). I have to set up a different test environment to work on the stock upstream autoloading. Got a lot of stuff on my todo list, but if no-one else gets to it first I will try and get to this over the next week or two.

Obviously, the alternative is to look at pulling those bits in via composer, but I presume someone already looked at that and there are some complications or something.

[th3fallen]

I'm working on this conversion at the moment but it is MUCH deeper than i had hoped their namespaces are off base at best. so its requiring quite a bit of tweaking. I'll put a diff up before i do a pr when i get done.

Edit: i'm attempting to drop OC autoloader completely.

[AdamWill]

th3fallen: the OC classes are hopelessly out of spec with any kind of classloading conventions (PEAR, PSR-0, PSR-4, whatever), so yeah, that's an ambitious goal. You'd either have to make OC's class layout less idiosyncratic, basically port the OC autoloader's logic into Composer's autoloader, do a whole bunch of hand-definitions, or possibly use the Composer auto-loader's classmap facility. But I wasn't proposing dropping the OC autoloader for OC's own classes, at least not yet, that did seem to be a bit too ambitious; I was only thinking of dropping its use for 3rdparty bundled stuff.

[karlitschek]

before we change the classloader here I would like to understand the performance implications. So how many IOs and filesystem and filesystem operations are triggered by ownCloud before this refactoring and after the refactoring

[th3fallen]

I totally agree the brunt of the work is getting a testable version to compare. I may stash those changes and get composer working on 3rd party libs.and see how that compares.

[DeepDiver1975]

@th3fallen please do not move all libs to composer just for the sake of "doing it right" - we will step by step move over to composer as soon as we need a new library or we need to update a library - THX

[AdamWill]

@DeepDiver1975 the idea is to register the libraries with the composer autoloader, not to actually move to pulling them in from composer. (though doing that would be another way to solve the autoloading issue, so I'm not sure why it's not a good idea for at least Patchwork and Pimple, which are pretty straightforward by the looks of it.)

[th3fallen]

I think his concerns are more performance based. But yes pimple and patchwork and even doctrine are dead simple to load in

[AdamWill]

OC's current doctrine is fairly heavily patched compared to upstream 2.3, IIRC, so including it from composer would be non-trivial. I don't think that applies to pimple or patchwork (OC is using fairly old versions of both, but they have lots of old versions available in packagist). I'm not sure about the symfony components.

Composer's autoloader should be more efficient than OC's, I believe, but it'd be good to get numbers to check.

[th3fallen]

In my trivial test all the stable symfony packages worked and the older version of pimple is available. I'll look more into it tomorrow.

[th3fallen]

I've got a working version that passes all defined unit tests on my end (waiting on scrutinizer) please do more tesiting as needed here are the relevent urls https://github.com/owncloud/core/pull/9714 https://github.com/owncloud/3rdparty/pull/104

[th3fallen]

@karlitschek As for the performance implications of composer autoloader in a xdebug profiler run on the index of the files app. OC\Autoload->load method's cost was 19.80 your calls to spl_autoload_call cost was 19.87. Where composers loaders cost was 2.66 and its spl_autoload_call's cost is 5.75.

Bear in mind that is in the copy that is using composer to load all non OC libs aside from the patched Doctrine one.

Note all costs referenced above are the total inclusive cost (self cost x invocation count) .

[DeepDiver1975]

Any 3rdparty code is already loaded now using composer's autoloader - more to come with #13241 -> close