Skip to content
This repository

Support directory structure for autoloading classes (Trac #3706) #3706

Closed
elgg-gitbot opened this Issue February 16, 2013 · 16 comments

1 participant

elgg-gitbot
elgg-gitbot
Owner

Original ticket http://trac.elgg.org/ticket/3706 on 41564424-09-24 by trac user kevinjardine, assigned to trac user mrclay.

Elgg version: 1.8.3

It's great that Elgg now supports autoloading classes, but not so great that these are loaded from a flat directory structure. Many PHP class libraries have subdirectories.

I recently had to code my own class autoloader to load a PHP library from a "myclasses" directory in my plugin directory to get around this limitation.

It was only about 10 lines of code but it felt a bit silly writing it given that Elgg is supposed to handle this automatically.

I'd be happy to submit a patch for core Elgg if the core team would accept such an enhancement.

elgg-gitbot
Owner

brettp wrote on 41566485-12-27

I'm not exactly sure what you're asking. You want mod/my_plugin/classes/some/vendor/SpecialClass.php to be auto loaded as SpecialClass?

elgg-gitbot
Owner

trac user kevinjardine wrote on 41567338-03-01

That would work in the case of the specific library I'm using as each class file uses the PHP 5.3 namespace command to assign the correct name.

In the longer term, it might make sense to convert a class file in

classes/dir1/dir2/MyClass.php

to the namespace-qualified class

dir1\dir2\MyClass

but so far as I know namespaces only work properly for PHP 5.3.

elgg-gitbot
Owner

trac user kevinjardine wrote on 41575114-12-18

Cash, I'm not proposing that Elgg be made "Elgg 5.3 only". In fact the opposite.
This enhancement request has nothing to do with that.

All I'm asking for is that classes be loaded from subdirectories in the plugin classes directories.

elgg-gitbot
Owner

cash wrote on 41574335-03-02

We're not ready to make Elgg 5.3 only.

Classes can be manually registered for autoloading through elgg_register_class().

elgg-gitbot
Owner

trac user kevinjardine wrote on 41575118-01-02

Replying to kevinjardine:

Cash, I'm not proposing that Elgg be made "Elgg 5.3 only".

Sorry that should have been "PHP 5.3 only".

elgg-gitbot
Owner

trac user mrclay wrote on 41666872-03-04

FYI, the mapping of class names to folders is becoming [http://groups.google.com/group/php-standards/web/psr-0-final-proposal standardized]. In short, take the class name, remove leading "\" if present, replace all "\" and "_" with DIRECTORY_SEPARATOR, append ".php":

\Foo\Bar_Bing => look for Foo/Bar/Bing.php

I believe both ZF2 and Symfony2 have adaptable code for generating classmaps by searching the filesystem, but the bigger problem is Elgg has no classmap cache, which gives it all its performance benefit. Of course we don't have to take the scan-everything-at-once approach. As long as it can be cached/refreshed, we can perform a search whenever it can't find an unknown class. E.g.

// in _elgg_autoload
$class = ltrim($class, '\'); // for normalization
if (isset($CONFIG->classes[$class])) {
    if ($CONFIG->classes[$class] === '') { // not found on last search
        return false;
    } else {
        if (is_readable($CONFIG->classes[$class])) {
            return (bool) (require $CONFIG->classes[$class]);
        } else {
            $CONFIG->classes[$class] = '';
            elgg_classmap_save();
            return false;
        }
    }
} else {
    // search all the classes dirs for The/Class/Name.php
    $CONFIG->classes[$class] = elgg_find_class($class);
    return _elgg_autoload($class);
}

In summary:
cache the locations of all "classes" folders.
cache the classmap (storing "" if search failed).
clear both caches on upgrade/plugin (de)activate
allow classmap to update itself (sacrificing a bit of performance for not having to include ALL files in classmap. e.g. we shouldn't put all of ZF in the classmap just because a plugin author includes the framework in "classes").

Another approach: provide a simple mechanism for plugin authors to add PSR-0-compatible autoloading to their classes dir. Only complex plugins that need it get the autoloading.

I'll try to work on this, but wanted to dump some thoughts here.

elgg-gitbot
Owner

cash wrote on 41686308-11-27

Agreed on caching being needed.

As an aside: A class name like "\Foo\Bar_Bing" makes me start looking for a new language to learn.

elgg-gitbot
Owner

trac user mrclay wrote on 42329952-02-25

PR: #204

elgg-gitbot
Owner

Milestone changed to Elgg 1.9.0 by ewinslow on 42360957-07-10

elgg-gitbot
Owner

trac user mrclay wrote on 42329957-06-11

Description of changes: http://groups.google.com/group/elgg-development/msg/710c641aab5be2eb

elgg-gitbot
Owner

trac user mrclay wrote on 42375870-12-21

1.9 PR: #223

elgg-gitbot
Owner

trac user mrclay wrote on 42423547-06-22

Rebased PR for 1.9 #262

elgg-gitbot
Owner

ewinslow wrote on 42423697-10-08

This was reported against 1.8.3, so let's keep that as the version. Milestone is at 1.9, which is effectively master right now.

elgg-gitbot
Owner

trac user Steve Clay wrote on 42534012-12-15

Fixes #3706: Overhauls autoloading to cached class map system
Changeset: ec533ef

elgg-gitbot
Owner

trac user Cash Costello wrote on 42534012-12-26

Merge pull request #262 from mrclay/3706_2

Fixes #3706: Overhauls autoloading to cached class map system
Changeset: fd04f4d

elgg-gitbot elgg-gitbot closed this February 16, 2013
Jeff Tilson jrtilson referenced this issue from a commit in THINKGlobalSchool/Elgg May 16, 2012
Steve Clay Fixes #3706: Overhauls autoloading to cached class map system ec533ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.