New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
AdamWill opened this Issue Jul 15, 2014 · 22 comments

Comments

Projects
None yet
9 participants
@AdamWill
Copy link
Contributor

AdamWill commented Jul 15, 2014

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

This comment has been minimized.

Copy link
Member

PVince81 commented Jul 15, 2014

@PVince81 PVince81 added the Bug label Jul 15, 2014

@bantu bantu added Enhancement and removed Bug labels Jul 15, 2014

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 15, 2014

I agree.

@hutchic

This comment has been minimized.

Copy link
Contributor

hutchic commented Jul 15, 2014

👍

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 16, 2014

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

This comment has been minimized.

Copy link
Member

bantu commented Jul 16, 2014

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

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 16, 2014

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

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 16, 2014

@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

This comment has been minimized.

Copy link
Member

bantu commented Jul 16, 2014

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

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 16, 2014

@AdamWill If your patch from #9643 (comment) 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

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 17, 2014

@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

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Member

karlitschek commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jul 17, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 17, 2014

@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

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 17, 2014

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

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 17, 2014

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

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 17, 2014

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
#9714
owncloud/3rdparty#104

@th3fallen

This comment has been minimized.

Copy link
Contributor

th3fallen commented Jul 18, 2014

@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) .

AdamWill added a commit to AdamWill/core that referenced this issue Jul 29, 2014

use Composer autoloader not OC for non-Composer 3rdparty (owncloud#9643)
Composer's autoloader is rather better than the OwnCloud autoloader's
handling of non-OC classes. Plus we can rely on upstream Composer to
maintain it and not worry about it ourselves.

With this change, we drop the bits of OwnCloud's autoloader that
handled non-OC classes, and register the classes that were being
handled by that code with Composer's autoloader instead. As these
dependencies are converted to actually being managed by Composer,
the explicit registrations can be dropped as they won't be needed
any more.

@butonic butonic added technical debt and removed Tech. Debt labels Sep 10, 2014

DeepDiver1975 added a commit that referenced this issue Oct 28, 2014

use Composer autoloader not OC for non-Composer 3rdparty (#9643)
Composer's autoloader is rather better than the OwnCloud autoloader's
handling of non-OC classes. Plus we can rely on upstream Composer to
maintain it and not worry about it ourselves.

With this change, we drop the bits of OwnCloud's autoloader that
handled non-OC classes, and register the classes that were being
handled by that code with Composer's autoloader instead. As these
dependencies are converted to actually being managed by Composer,
the explicit registrations can be dropped as they won't be needed
any more.

Since OwnCloud's autoloader isn't going to handle non-OC classes any
more, we no longer need to test to make sure it does it right.

drop unneeded registerPrefix() and registerClass() from autoloader

Now we're not handling anything but OC's own classes, these are
unnecessary.

error out if composer autoloader is not found (thanks bantu)

We're never going to be able to work without the autoloader, if it's not
there we should just throw our hands up and surrender.

LukasReschke added a commit that referenced this issue Oct 28, 2014

Merge pull request #11805 from owncloud/AdamWill-issue9643
use Composer autoloader not OC for non-Composer 3rdparty (#9643)
@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Mar 2, 2015

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

@MorrisJobke MorrisJobke removed this from the Maybe someday milestone Mar 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment