[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 rmccue):

 Added [attachment:classes-real.php] which better demonstrates what the
 final code should look like, without the temporary code I had in there.
 Apologies for not uploading this earlier, I was a bit unclear on what was
 formatting and what was actual proposed code.

 Replying to [comment:31 tfrommen]:
 > 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.
 :)

 Thanks for the reply. We're all working towards the same goal here :)

 > 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 agree, but I don't see there being anything incompatible with the
 approaches suggested here right now. Discussing actually switching to
 Composer is definitely outside the scope of this ticket.

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

 I agree, but I also don't want to be overly generic. If the stuff we put
 into core is usable, there's no reason to not include it, but I'd rather
 we not include things that core doesn't use until there's a need for them.
 We can always iterate on this in future versions.

 > 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 think the FileLoader parts and the Controller class are both a bit too
 over-abstracted for core usage.

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

 Sorry, this was part of the formatting/table code. New attachment doesn't
 include this.

 > '''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:

 This falls under something that we don't need in core, so I'd prefer to
 avoid it. With the exception of a couple of files, core only uses `class-`
 as the prefix; those other files are `class.` and can be handled by the
 class map instead. (Ideally, I'd prefer to move those to fit the pattern.)

 > '''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?

 These were both part of the formatting code, apologies.

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

 Good point.

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

 Autoloaders aren't required to return a value, as far as I'm aware;
 `spl_autoload_register( function ( $class ) { require $class . '.php'; }
 )` is perfectly valid (although not really a good idea).

 >
 > '''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.

 One problem here is if `spl_autoload_register` is called before the file
 containing the function is loaded. e.g. `spl_autoload_register( 'myfunc'
 ); require 'myfunc.php';`

 I believe SPL would allow this normally, so we need to in the polyfill.

 > '''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.

 Good catch.

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

 I'd love to make sure this is usable for plugins/themes as well, but I
 want to keep the scope minimal so we can ship it as soon as possible. (I
 do really want this in my own plugins and themes, because I ''hate''
 having 300 lines of `require`. Every time I work on core, it's like going
 back in time.) As long as we can expand that out in future releases, I
 don't think we need to ship everything right now.

 For example, do we need a PSR-4 autoloader? Core doesn't use it, so I'd
 prefer not to do it for now, and implement it later if lots of plugins and
 themes do want it.

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