[wp-trac] [WordPress Trac] #36335: Next generation: core autoloader proposal

[WordPress Trac]

#36335: Next generation: core autoloader proposal
-----------------------------+-----------------------------
 Reporter:  dnaber-de        |       Owner:
     Type:  feature request  |      Status:  new
 Priority:  normal           |   Milestone:  Future Release
Component:  General          |     Version:
 Severity:  normal           |  Resolution:
 Keywords:                   |     Focuses:
-----------------------------+-----------------------------

Comment (by tfrommen):

 Hey Ryan,

 thanks a lot for your comment, and even more for the time and effort spent
 around it.

 There are several things that I don't quite like, so I'm going to discuss
 them here. I really do try to be as constructive as I can, though. :)

 > I think whether we adopt Composer usage or not isn't hugely relevant to
 this conversation...

 In my opinion, it '''is''' relevant in that we really should not (in fact:
 '''must''' not) introduce anything incompatible with Composer as long as
 there is even the slightest chance we might switch over to Composer one of
 these days.

 > I don't think the current implementation is appropriate for core. It's a
 bit too heavy and general-purpose.

 I don't see anything bad with general purpose software
 (architecture)—as long as serving general purpose doesn't mean
 including lots of stuff that is only relevant for a tiny fraction of use
 cases, of course.
 The current state of our implementation is definitely not set into stone.
 However, I don't see anything really heavy and/or unnecessary there.
 Would you mind indicating what parts you find superfluous (for the start)?

 > I have an alternative implementation that tries to stay as lightweight
 as possible.

 Some comments on your code then...

 '''Consistent return type'''
 Can we please try to not include any more multi-return-type functions
 (without an actual, good reason for doing so)...?
 Making `get_path()` return an empty string `''` can only be understood as
 ''I didn't find a file (path) for the ''thing'' you are trying to load'',
 can it?
 So we should just stick with `string`.

 Even if core WordPress doesn't come with interfaces (or traits), it makes
 sense not to exclude these Structural Elements.
 This would lead to these two changes:

 * `$class` would be either `$fqn` (fully qualified name) or `$fqsen`
 (fully qualified Structural Element name);
 * ''ClassMap'' would be just ''Map''.

 '''More flexible code'''
 I would like to not include the hard-coded `class-` prefix into the
 `WP_Autoload_Prefixed` class, but rather implement the class like so:

 {{{#!php
 <?php

 class WP_Autoload_Prefixed implements WP_Autoload_Handler {

         protected $path;

         protected $path_prefix;

         protected $prefix;

         public function __construct( $path, $path_prefix = '', $prefix =
 '' ) {

                 $this->path = trailingslashit( $path );

                 $this->path_prefix = strtolower( $path_prefix );

                 $this->prefix = strtolower( $prefix );
         }

         public function get_path( $fqn ) {

                 $fqn = strtolower( $fqn );

                 if ( $this->prefix && 0 !== strpos( $fqn, $this->prefix )
 ) {
                         return '';
                 }

                 $filename = $this->path_prefix . str_replace( '_', '-',
 $fqn ) . '.php';

                 return $this->path . $filename;
         }
 }
 }}}

 That way, one could also use the class for things like `class.foo.php` and
 `interface-foo.php`.

 '''Prefix global functions'''
 We should not introduce a new unprefixed global function
 `register_autoloader()`. This will eventually break things here and there,
 so we should call it something like `wp_register_autoloader()`.

 I guess the `file_for_class` if just for your table, right?

 '''Improve `__autoload()`'''
 `$spl_autoloaders` is global, so anyone can put into it whatever they
 like. So we should check for `is_callable` inside `__autoload()`.

 Also, we should really not check for `class_exists` for several reasons.
 First, it's restricted to classes only. Second, it's too heavy, and even
 unnecessary, because the autoload function already returns something we
 can check against.

 This would make `__autoload()` something like this:

 {{{#!php
 <?php

 function __autoload( $fqn ) {
         global $spl_autoloaders;

         foreach ( $spl_autoloaders as $autoloader ) {
                 if ( ! is_callable( $autoloader ) ) {
                         continue;
                 }

                 if ( call_user_func( $autoloader, $fqn ) ) {
                         // If it has been autoloaded, stop processing.
                         return true;
                 }
         }

         return false;
 }
 }}}

 '''Improve `spl_autoload_register()`'''
 You check if the passed callback is actually callable, okay. But you
 continue to just add it to the global in case the function shouldn't throw
 any exceptions.
 In fact, something like this would be better:

 {{{#!php
 <?php

 function spl_autoload_register( $autoload_function, $throw = true,
 $prepend = false ) {

         if ( ! is_callable( $autoload_function ) ) {
                 if ( $throw ) {
                         // String not translated to match PHP core.
                         throw new Exception( 'Function not callable' );
                 }

                 return false;
         }

         global $spl_autoloaders;


         // Don't allow multiple registration.
         if ( in_array( $autoload_function, $spl_autoloaders ) ) {
                 return false;
         }

         if ( $prepend ) {
                 array_unshift( $spl_autoloaders, $autoload_function );
         } else {
                 $spl_autoloaders[] = $autoload_function;
         }

         return true;
 }
 }}}

 '''Improve `spl_autoload_functions()`'''
 Since `$spl_autoloaders` is a global, and since we don't want to argue
 with SPL, we should make sure we're returning an array.

 {{{#!php
 <?php

 function spl_autoload_functions() {
         global $spl_autoloaders;

         return is_array( $spl_autoloaders ) ? $spl_autoloaders : array();
 }
 }}}

 > It implements the bare minimum to support autoloading core itself, which
 is likely also useful for plugins, but isn't necessarily designed to be.

 The main difference between the currently proposed implementations as well
 as the according discussions lies with '''who''' is '''when''' able to use
 the autoloader '''for what'''.
 As long as the end goal is a central autoloader that '''can''' be used for
 core WordPress as well as plugins and themes, I'm totally fine with
 whatever route we take.
 However, we really should think about from what amount (and type) of
 changes we could benefit the most, and what types fo changes do make most
 sense when.

 > let's use `spl_autoload_register` directly rather than implementing our
 own autoloading stack on top of it.

 Well, that would be awesome, wouldn't it. :)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36335#comment:31>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform