WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 4 months ago

#36335 new feature request

Next generation: core autoloader proposal

Reported by: dnaber-de Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description (last modified by ocean90)

Hello WordPress community. With this ticket I would like to get the debate of the last days about how to attract WordPress to developers (or the other way around?) to a concrete discussion on how a class autoloader could look like in WordPress core.

So when we start to think about some major improvements like in #36292, we should also take some time and talking about autoloading.

Abstract

A class autoloader is a basic tool to separate code writing form code organization. It takes care about providing class declaration at the point they are needed. The fact that WordPress lacks of a core autoloader was one point mentioned in the debate on what developers missing most with WordPress.

Why we need an autoloader

Plugin authors using autoloaders these days. They even use composer for dependency management and ship their plugins with this dependencies. This practice leads to trouble right now. I'm convinced that in a long-range plan we even have to talk about how to deal with proper dependency management to overcome collisions. Having an autoloader in core is a precondition for this.

How an implementation could look like

The following proposal follows a concept of separating the file locating and file loading process to avoid a violation of the single responsibility principle and to gain flexibility. All classes and interfaces are prefixed with WP_Autoload_ to apply a pseudo namespace.

The main instance of this concept is the interface WP_Autoload_Rule. An autoload rule is against the client responsible for locating and loading a given class. The class is provided by its full qualified name. This leads to this interface signature:

<?php
interface WP_Autoload_Rule {

    /**
     * @param string $class (A full qualified class name)
     *
     * @return bool
     */
    public function load_class( $class );
}

Implementations could be WP_Autoload_Psr4Rule, WP_Autoload_Psr0Rule or WP_Autoload_ClassMapRule or what ever plugin and theme authors want to implement for their requirements. Here's a quick example:

<?php
class WP_Autoload_Psr4Rule implements WP_Autoload_Rule {

    /**
     * @var string
     */
    private $base_namespace;

    /**
     * @var string
     */
    private $base_directory;

    /**
     * @var WP_Autoload_FileLoader
     */
    private $file_loader;

    /**
     * @param $base_namespace
     * @param $base_directory
     */
    public function __construct( $base_namespace, $base_directory, $file_loader = NULL ) {

        $this->base_directory = (string) $base_directory;
        $this->base_namespace = (string) $base_namespace;
        $this->file_loader    = $file_loader && is_a( $file_loader, 'WP_Autoload_Fileloader' )
            ? $file_loader
            : new WP_Autoload_IsReadableFileLoader;
    }

    /**
     * @param string $class (A full qualified class name)
     *
     * @return bool
     */
    public function load_class( $class ) {

        // performing the psr4 mapping here to get a $file
        $file = '/whatever';

        return $this->file_loader->load_file( $file );
    }

}

Autoloading rules should depend on a file loader. The file loader receives a file name and loads it, if it is present.

<?php
interface WP_Autoload_FileLoader {

    /**
     * @param string $file
     *
     * @return bool
     */
    public function load_file( $file );
}

A very simple implementation can just ask for is_readable(). Looking on performance, another implementation could glob()-ing a given directory once to read in all PHP-files and matching the given file against this list. Even persistent caches are thinkable.

Last but not least WP_Autoload_Autoload acts as a repository for all possible rules:

<?php
interface WP_Autoload_Autoload {

    /**
     * @param WP_Autoload_Rule $autoload_rule
     *
     * @return void
     */
    public function add_rule( $autoload_rule );
}

The main implementation should be WP_Autoload_SplAutoload which connects to PHP standard library autoload:

<?php
class WP_Autoload_SplAutoload implements WP_Autoload_Autoload {

    /**
     * @var WP_Autoload_Rule[]
     */
    private $rules = array();

    /**
     * @param WP_Autoload_Rule $autoload_rule
     *
     * @return void
     */
    public function add_rule( $autoload_rule ) {

        if ( ! in_array( $autoload_rule, $this->rules, TRUE ) )
            $this->rules[] = $autoload_rule;
    }

    /**
     * @param string $fqcn (full qualified class name)
     */
    public function load_class( $fqcn ) {

        foreach ( $this->rules as $rule ) {
            if ( $rule->load_class( $fqcn ) )
                break;
        }
    }
}

Instantiate and propagate the autoloader

<?php
function wp_setup_autoloader() {

    $autoloader = new WP_Autoload_SplAutoload;
    spl_autoload_register( array( $autoloader, 'load_class' ) );

    /**
     * Use the WordPress core autoloader to
     * bootstrap your plugin and theme
     *
     * @param WP_Autoload_Autoload $autoloader
     */
    do_action( 'wp_autoload', $autoloader );
}

wp_setup_autoloader() should then be called early in the WP loading process.

<?php
add_action( 'wp_autoload', function( $autoloader ) {

    $autoloader->addRule(
        new WP_Autoload_Psr4Rule(
            'MyPlugin\\'
            __DIR__ . '/src'
        )
    );
} );

Things to discuss

  • The proposal uses sensible interface names which I prefer over naming interfaces like WhateverInterfaces. As WordPress does not provide interfaces right now, this is just a suggestion.
  • PHP class identifiers are case insensitive. That means new MySql and new MySQL will both work, if a class mysql is declared. The autoloader should respect this. Now, Psr4 is very wide-spread and encourage developers to use case sensitive autoloaders and this is a problem. How can we deal with this in a performant way?
  • How should the WordPress core files be organized to work with the autoloader? Is it realistic to rearrange them, if not how could a corresponding WP_Autoload_Rule look like?
  • What about compatibility with PHP 5.2.? The proposal uses spl_autoload_register. But before PHP 5.3.0 it is theoretically possible to deactivate the spl extension. In this case another implementation of WP_Autolad_Autoload would be necessary and maybe some adaptions to the other interfaces. But is this really the intention?

Finally

Thanks for reading. Feel free to add your concerns, your opinions or even if I'm on a completely wrong train. In fact
I'm really interested in critic. To be clear, I don't want to push this proposal but I would like to see a proper
autoloader in core some day :)

Attachments (28)

core-classes.txt (12.7 KB) - added by ocean90 12 months ago.
classes.php (9.9 KB) - added by rmccue 12 months ago.
Alternative implementation of autoloading concept
autoload-compat.php (1.8 KB) - added by rmccue 12 months ago.
SPL autoload compatibility shim/polyfill
classes-real.php (8.7 KB) - added by rmccue 12 months ago.
Proposed actual code for the autoloaders
36335.diff (67.6 KB) - added by wonderboymusic 8 months ago.
36335.2.diff (71.9 KB) - added by wonderboymusic 8 months ago.
36335.3.diff (72.3 KB) - added by wonderboymusic 8 months ago.
36335.4.diff (72.1 KB) - added by netweb 8 months ago.
36335.5.diff (198.3 KB) - added by wonderboymusic 8 months ago.
36335.6.diff (244.0 KB) - added by wonderboymusic 8 months ago.
36335.7.diff (192.6 KB) - added by wonderboymusic 8 months ago.
36335.8.diff (137.3 KB) - added by wonderboymusic 8 months ago.
36335.9.diff (141.8 KB) - added by jorbin 8 months ago.
36335.10.diff (89.3 KB) - added by wonderboymusic 8 months ago.
36335.11.diff (61.0 KB) - added by wonderboymusic 8 months ago.
36335.12.diff (60.3 KB) - added by wonderboymusic 8 months ago.
36335.13.diff (46.2 KB) - added by wonderboymusic 8 months ago.
36335.14.diff (45.7 KB) - added by wonderboymusic 8 months ago.
36335.15.diff (925 bytes) - added by JohnPBloch 8 months ago.
36335.16.diff (758 bytes) - added by kraftbj 8 months ago.
Fixes WP_Screen notice
http-tests.diff (548 bytes) - added by swissspidy 8 months ago.
36335-custom-autoloader-plugin.diff (39.0 KB) - added by schlessera 8 months ago.
Using custom WordPress-specific autoloader plugin.
36335-revert.diff (29.4 KB) - added by wonderboymusic 8 months ago.
36335-revert.2.diff (47.3 KB) - added by wonderboymusic 8 months ago.
36335-revert.3.diff (47.3 KB) - added by wonderboymusic 8 months ago.
36335-classmap.diff (33.8 KB) - added by MikeSchinkel 8 months ago.
wp-classmapgen.php and example generated wp-classmap.php
36335-classmap.2.diff (28.5 KB) - added by MikeSchinkel 8 months ago.
wp-classmapgen.php and example generated wp-classmap.php
36335-autoloader.diff (6.0 MB) - added by MikeSchinkel 7 months ago.
Refactoring of WordPress core to enable autoloading of classes + a simple classmap generator and classmap autoloader.

Change History (267)

#1 @ocean90
13 months ago

  • Description modified (diff)

Previously: #21300, #30203

#2 @dnaber-de
13 months ago

One more thing: I read the discussion about #30203 here and I do not accord that autoloading is all about dependency management. However, what if we think about a solution that gives a complete optimized autoload file for the core that can be shipped with WordPress which doesn't hurts performance at all? Beside that having all benefits of a dynamic autoloader for development. Thats what we should strive for.

I'll provide complete implementation examples for the proposal the next days.

Last edited 13 months ago by ocean90 (previous) (diff)

#3 @tfrommen
13 months ago

For testing's sake, the autoloader methods (i.e., the interface as well as the implementing classes) should return a status. This would lead to the following:

<?php
interface WP_Autoload_Autoload {

    /**
     * @param WP_Autoload_Rule $autoload_rule
     *
     * @return bool
     */
    public function add_rule( WP_Autoload_Rule $autoload_rule );
}

and

<?php
class WP_Autoload_SplAutoload implements WP_Autoload_Autoload {

    /**
     * @var WP_Autoload_Rule[]
     */
    private $rules = array();

    /**
     * @param WP_Autoload_Rule $autoload_rule
     *
     * @return bool
     */
    public function add_rule( WP_Autoload_Rule $autoload_rule ) {

        if ( in_array( $autoload_rule, $this->rules, true ) ) {
            return false;
        }

        $this->rules[] = $autoload_rule;

        return true;
    }

    /**
     * @param string $fqcn (fully qualified class name)
     *
     * @return bool
     */
    public function load_class( $fqcn ) {

        foreach ( $this->rules as $rule ) {
            if ( $rule->load_class( $fqcn ) )
                return true;
        }

        return false;
    }
}

I also added a type-hint for the autoloader rule parameter.

#4 @jdgrimes
13 months ago

As far as support for PHP 5.2 goes, what I am doing in one of my plugins is this: when the autoloader is being registered, if spl_autoload_register() isn't available, it just goes ahead and pre-includes all of the class files instead. Obviously that's not ideal, but I don't think that there's anything else that you can do.

#5 @ChriCo
13 months ago

@jdgrimes this could be implemented as WP_Autoload_ClassMap as fallback if no spl_autoload_register() is available.

#6 @dnaber-de
13 months ago

@tfrommen Exactly, thanks. About that type-hinting: I messed up with 5.2. compatible syntax, ignoring that it is available since 5.0.

@jdgrimes Yes, a possible solution will probably result in something like this. But I would like to have a clear, unified interface for that and ideally an automatic process to collect all files and build the includes. Something like composers classmap.

#7 @dnaber-de
13 months ago

We decided to follow the example of the Fields API proposal and created a public repository on github to collect ideas and work on a concrete concept and later on an implementation.

https://github.com/inpsyde/wordpress-core-autoloader

Everyone's opinion, critic and contribution is appreciate either as comment here or as issue over there at Github.

#8 follow-up: @rmccue
13 months ago

  • Milestone changed from Awaiting Review to Future Release

FYI, we can actually polyfill autoloader registration. The way autoloaders work in PHP is that PHP runs the __autoload() function, or rather, it looks the __autoload symbol up and executes it. When SPL is enabled, it actually reaches into PHP's internals and swaps the existing __autoload out for its own implementation, keeping a reference to the previous function.

We can replicate this:

if ( ! function_exists( 'spl_autoload_register' ) ):
function __autoload( $class ) {
    global $wp_registered_autoloaders;
    foreach ( $wp_registered_autoloaders as $autoloader ) {
        call_user_func( $autoloader, $class );
        if ( class_exists( $class, false ) ) {
            break;
        }
    }
}

function spl_autoload_register( $autoloader ) {
    global $wp_registered_autoloaders;
    $wp_registered_autoloaders[] = $autoloader;
}
endif;

(It requires that we're the first to declare __autoload, but that shouldn't be an issue 99% of the time.)


As to the issue of whether we want an autoloader or not, profiling has been done in the past as to whether it'd be useful or not, and the finding then was that including the files we're definitely going to need turned out better. I don't know the exact results, since it's important to remember that autoloading is a time/memory tradeoff; loading all files might take less time, but significantly more memory.

I think a staged approach to this is the best way. Firstly, let's make every class that's not always loaded (wp-admin, etc) loadable via autoloading. This is probably a large task in itself, as we need to switch to one-class-per-file with predictable file naming.

Once that's done, stage 2 should be working out which classes are part of the critical path (i.e. required for every page load). Those that aren't can be switched to autoloading.

I strongly support this idea; thanks for proposing it :)

#9 @dnaber-de
13 months ago

Hi @rmccue
thanks for your feedback. Using __autoload() as fallback is a good idea, I already forgot about that function but of course, it's the most obvious way to polyfill SPL autoloading.

Currently, we're improving the concept behind the proposal.

Firstly, let's make every class that's not always loaded (wp-admin, etc) loadable via autoloading. This is probably a large task in itself, as we need to switch to one-class-per-file with predictable file naming.

Sure thing. I think this should go in a separate ticket as this is somewhat a separate task. What do you think?

#10 @giuseppe.mazzapica
13 months ago

Hi all,

I have a major concerns here.

The first one is that most of core files containing classes, does not just contain classes.

In fact, many of them also contain function definitions, function calls, constants definitions... and so on. All things that actually force those file to be required eagerly (no autoload).

The other concern is that some files contain more than one class. It means those files does not follow WordPress coding standard (https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions):

class file names should be based on the class name with class- prepended and the underscores in the class name replaced with hyphens, for example WP_Error becomes: class-wp-error.php

An example (among literally dozens) the class WP_MatchesMapRegex is located in a file named class-wp.php that is so named because also contains the class WP.

Not to talk about some classes that are placed pretty much arbitrarily: wpdb class is in wp-db.php and WP_Query class is in query.php.

A "sane" autoload will need:

  • to be able to easily match a class to a file name. This can be achieved by having one class per file, following coding standards for naming
  • don't be forced to hard-require files because of function / constant definitions or client code. This can be achieved by putting only class definitions in class files.

Until those things are done, we can have only a deficient autoloader.

To be fair, the first point could be achieved implementing a map from class to file name:

function wp_autoload_map() {
  global $wp_autoload_map;
  if ( ! $wp_autoload_map ) {
    $wp_autoload_map = array(
      'WP'                 => 'wp.php',
      'WP_MatchesMapRegex' => 'wp.php',
      'WP_Query'           => 'query.php',
      'wpdb'               => 'wp-db.php',
      // and other 257 entries here...
    );
  }
  return $wp_autoload_map;
}

but this not the best solution ever and does not solve the issue of function / constants definition and client code in files that contain class definitions...

#11 @dnaber-de
13 months ago

Heyo Giuseppe,

thanks for your response.

You're absolutely right, the core is at the moment not able to switch to a common autoloading system. I am aware of that. Moving the core to a one-class-per-file structure is worth another ticket, I think. (It is at least a sub-task of the big complex «autoloader». And by the way probably the most complicated part.)

The first focus from my perspective is to provide a working example of a possible implementation that comply with these two points:

  • being a proof of concept. That means to show people that is it generally possible. Even more, having a existing example make it easier to understand and discuss the concept.
  • being flexible, so that it can be used in any way we want.

I could also imagine a sequential introduction. In the first release the autoloader is introduced. In a next release everything in wp-admin/ is moved to the autoloader and so on.

Anyway, the major benefit of having an autoloader would be having a consistent way for plugin authors and later talking about dependency/package management. Because we're runing into a big issue with inconsistent packages, when plugin authors keep on using composer and ship their dependencies with their plugins. And they will.

#12 @giuseppe.mazzapica
13 months ago

Anyway, the major benefit of having an autoloader would be having a consistent way for plugin authors and later talking about dependency/package management. Because we're runing into a big issue with inconsistent packages, when plugin authors keep on using composer and ship their dependencies with their plugins. And they will.

Autoloader does not resolve the dependency/package management issue. Actually, if autoloader is shared there are chances it causes even more issues.

To give you an example, let's assume I have a plugin which has a dependency to AwesomeLibrary in version 1. And another plugin that uses same library, but in version 2.

If we have a shared autoloader, both plugins will somehow "register" a rule in the shared autoloader to load the AwesomeLibrary class.

Assuming that the class is named in the same way in both versions (a very reasonable assumption) which version of the file the autoloader have to load when new AwesomeLibrary() is encountered in code?

This just to demonstrate that autoloader and dependencies are related issues, but solving one does not necessarily solve (nor necessarily help in solving) the other.

To bring you a real world example, Composer autoloader works very well as a shared autoloader when Composer is used to manage the entire website, because it has control on all the dependencies and so there's the warranty only one version of each package is installed. In fact, it fails miserably when different plugins ship different version of same library.

Moreover, a dependency/package management tool does not require an autoloader: it's a convenience not a requirement.

For these reasons I think that having an autoloader would not facilitate the dependency/package management discussion that much.

That said, I think an autoloader for core is a nice thing and pretty easy to realize (surely easier than dependency/package management by far) and if we focus on core autoloader there might be chance it will be done, as some point.

However, I think to make core actually "autoloadable" is a priority in that direction.

For example, consider the issue of files containing more classes. There are two ways to solve it: 1) split the files; 2) write a complex implementation capable to work in those cases.

If you go with the latter, and at some later point the file are split, you wasted time and effort on create an unnecessary complex proof of concept implementation. Wouldn't been better to start working on a proof of concept after files were split or after the decision to not split the file was taken?

#13 @dnaber-de
13 months ago

Hm, I think my expressions where a bit fuzzy. It was not my intention to say, that autoloading solves dependency management. Of course it does not. But in my opinion it is a first step in a long way to get there. If you name this more a convenience than a requirement, I'm totally fine with this :) But I'm a lazy developer, who likes to work convenient.

However, I think to make core actually "autoloadable" is a priority in that direction.

Maybe you're right. But at the moment it's not clear that the core will ever be completely handled by an autoloader. At least this is what I read out of Ryan McCue's and Nacin's contributions to this discussion.

Basically I fully agree with all your points. But non of them is a reason for me to stop thinking and working on a possible solution for that autoload part.

#14 follow-up: @schlessera
12 months ago

Although I would really love to have an autoloader be centrally available within WordPress, I fear that the current proposal will cause more issues than it solves.

  • Autoloading large-scale projects (that depend on a lot of external resources like libraries/plugins/themes) goes hand-in-hand with namespaces. Without namespaces, you are guaranteed to have conflicts.
  • Usage of an autoloader will only start to provide tangible benefits if it is combined with a clear OOP architecture that makes use of dependency injection and lazy-loading. Without these, you will probably end up still including everything (to avoid breaking stuff), just with a lot of overhead.
  • Assuming that one of the long-term goals for WordPress is to be a prominent citizen within the PHP world, building our own autoloader instead of using the one from Composer (that pretty much every other recent PHP project uses) will just add another roadblock for developer on-boarding, and doesn't allow us to reuse the huge amounts of work that have gone into Composer's implementation as well as optimization (which contains a lot of code to make the autoloader efficient & scalable for production).
  • When talking about eventually adding dependency/package management in the future, it should be a given that interoperability is critical. This makes building our own autoloader problematic again.

I would recommend just adding a composer.json for now and using this to work on making WP's components autoloadable in the first place. Composer can generate the optimized class maps, and it can even provide a PHP5.2 compatible autoloader. This avoids "reinventing the wheel from the past". Composer itself would only be a development requirement to make changes in Core, so the fact that it requires PHP5.3.2 to rebuild the classmaps should not be an issue.

Making everything properly autoloadable is probably enough work for one or the other release as it stands.

Once this work is done, chances are that we can make the switch to PHP5.3, and make the central (Composer) autoloader available to plugins & themes. In the mean time, we won't introduce new issues through an autoloader that won't provide the benefits that we hope for.

This would also provide the additional benefit that, once the composer.json has the correct autoloading settings and the WP classes are properly autoloadable, you can very easily efficiently reuse this data in a composerized WP stack.

#15 follow-up: @greg1anderson
12 months ago

The comments by @schlessera, above, are extremely sensible. I wanted to expand slightly on these thoughts, lest individual plugin authors become tempted to provide their own autoloading prior to its availability in WP core. This is technically possible, but runs the risk of running into some pretty serious "dependency hell" when people mix-and-match their extensions.

It is clear that if plugin "A" requires some library version 1.x, and another library requires the same library at version 2.x, that can never work. However, if you implement plugin dependencies by giving each plugin its own autoloader, then you can run into some pretty extreme and hard-to-debug scenarios if you mix the same minor versions (e.g. 1.0.1 and 1.0.2) of the same dependency. The problem with including different versions of Composer twice has already been mentioned above, but this same problem can extend to any common library used in any two plugins. For a more complete explanation, see The Trouble with Two Autoloaders.

This problem cannot be solved by implementing your own common autoloader. It can only be solved by having a single autoloader for the entire application -- e.g. a single composer.json (EDIT: misspoke here, see below) at the root of the project, where plugins are added via composer require.

Last edited 12 months ago by greg1anderson (previous) (diff)

#16 in reply to: ↑ 15 ; follow-ups: @schlessera
12 months ago

I did not want to delve too deep into the dependency/package management topic, as I find that this is a completely separate problem to solve from autoloading. Having Composer provide both adds to the general confusion, as they often get thrown into the same bucket.

However, what @greg1anderson says is completely true. When you try to load two conflicting libraries, you will always have an issue, no matter how many autoloaders you throw in-between. They will only obfuscate the real issue, and the sooner you get an error message about the conflict, the better. PHP can only ever know about 1 version of a given class, whether that was included or autoloaded...

However, I want to clarify one technical nuance from the comment above:

This problem cannot be solved by implementing your own common autoloader. It can only be solved by having a single autoloader for the entire application -- e.g. a single composer.json at the root of the project, where plugins are added via composer require.

Yes, there needs to be only 1 single autoloader to get rid of this issue. However, there can be as many composer.json files as you want (1 for each dependency). Composer will recursively go through each of them, and build up that one true autoloader. So, if you composer require a plugin, that plugin's composer.json gets parsed and its class definitions added to the autoloader automatically.

(Just want to make sure people are not afraid to have a composer.json inside of each plugin or theme.)

Last edited 12 months ago by schlessera (previous) (diff)

#17 in reply to: ↑ 16 ; follow-up: @dnaber-de
12 months ago

having a single autoloader for the entire application

That's what this ticket is all about. :)

#18 in reply to: ↑ 16 @greg1anderson
12 months ago

Replying to schlessera:

Yes, there needs to be only 1 single autoloader to get rid of this issue. However, there can be as many composer.json files as you want (1 for each dependency). Composer will recursively go through each of them, and build up that one true autoloader. So, if you composer require a plugin, that plugin's composer.json gets parsed and its class definitions added to the autoloader automatically.

Yes, you are absolutely right; thank you for making that correction. Each requirement does have its own composer.json file, and there are a number of ways these can be brought together, including wikimedia/composer-merge-plugin, or something custom such as the Composer Manager module that the Drupal project ended up with. I think the best solution, though, is to use Composer to build the autoload.php files, exactly as you propose in 14.

#19 in reply to: ↑ 17 @greg1anderson
12 months ago

Replying to dnaber-de:

having a single autoloader for the entire application

That's what this ticket is all about. :)

Yes, absolutely. What I really intended to say here was that there should be a single composer-generated autoloader for the entire application. When you start calling spl_autoload_register directly, though, and you provide a way for plugins to provide classes of their own, you quickly move into a space where plugin authors need or desire to introduce dependencies of their own. During development, it is common to overlook the problems that can be caused when plugins provide dependencies that are not coordinated by the top-level application. Such solutions will often appear to work well for quite a long time, because problems are not evident until subtle differences in dependencies cause a conflict.

@schlessera already made the most important points in 14. The best way forward is to bring in all dependencies via composer require from the project root.

#20 @dnaber-de
12 months ago

When you start calling spl_autoload_register directly, though, and you provide a way for plugins to provide classes of their own, you quickly move into a space where plugin authors need or desire to introduce dependencies of their own.

We are already there. And yes, I totally agree, just having an autoloader doesn't solve this problems. But I wouldn't have started this ticket if I see a realistic chance to get composer support in the next view years. Having an autoloader in core would allow developers at least to use a shared instance that can be optimized somehow.

The proposal is designed flexible and extensible, so in the future it wouldn't be a problem to adapt the composer autoloader or the other way around.

#21 @schlessera
12 months ago

I just wrote a longer post about using Composer as a central autoloader for WordPress: https://www.alainschlesser.com/adding-central-autoloader-wordpress/

Summary of the experiment, for those who don't want to read through the entire thing:

  • Adding a PHP 5.2 compatible autoloader to WordPress is a matter of changing just a few lines.
  • Having a "well-behaving" class be loaded through that autoloader "just works" (as demonstrated with _WP_Editors class).

If there's interest in delving deeper into that approach, I'm more than willing to contribute.

Last edited 12 months ago by schlessera (previous) (diff)

#22 @tfrommen
12 months ago

To be honest, I would've expected at least some response to the last comment(s).
But maybe other people have the same problems I have (or had)...?

I really do like the idea to use Composer's autoloader. And at the same time I don't like it (for now?).

Just so this has been said: we did not propose a custom autoloader (no matter if the one over at the repository at Inpsyde's, or another one) because we would like to write an autoloader. The one and only reason is to use a single central autoloader.

As mentioned numerous times, the current state of Core WordPress's codebase does not play nicely with a modern autoloader. So there's (a lot of) work to do, and we all know that.
When David created this ticket, the plan was not to only work on a functional autoloader alone. In parallel, one could also start with some of the changes that need to be done with respect to WordPress itself.
Our idea was (and hopefully, it still is) to implement a functional autoloader (which we started in the GitHub repository linked above), and then integrate it in some WordPress fork. To demonstrate its usage.

The main, and in my opinion, very important difference between this approach and the one proposed by @schlessera is: who can make use of the autoloader when.
With our approach, plugins and themes could start using the autoloader with WordPress 4.6 already, for instance. Changes at Core WordPress could then be made step by step, when the time is right for each of these steps.

The one problem with these changes is ... that there are a lot of individual problems. ;)
Changes only can be made in a granular fashion. One would need to agree on a consistent naming scheme for classes and interfaces (okay, okay, there are none—yet!). Moving parts from one file to another makes it hard for anyone working on patches for that file. Then, some day, we maybe have namespaces. All this makes preparing (and, later on, adapting) WordPress a tedious task.

If the long-term goal is to eventually ship an autoloader that works like the one of Composer (and maybe at some point in time we actually use Composer in general), we shouldn't allow for registration of custom autoload rules (as we called the classes implementing the individual autoloading logic). Instead, we should only offer a small set of autoloading ways, so each plugin or theme author can choose what works (best) for them. PSR-0, PSR-4, a classmap (because the plugin/theme doesn't follow PSR-whatsoever), ...
What we would gain here is the ability to dynamically register with the autoloader—opposed to only have WordPress (partially!) use the autoloader.

I'm really interested in your opinions here.

Last edited 12 months ago by tfrommen (previous) (diff)

This ticket was mentioned in Slack in #core by tfrommen. View the logs.


12 months ago

#24 @charliespider
12 months ago

re:

I'm really interested in your opinions here.

The biggest problem with using Composer as a solution for WP is the sheer number of sites and servers that will not have it installed. Therefore the only viable option IMHO is to bake an autoloader into core.

PSR-4 plus a class map for nonconformists would be good (PSR-4 only would be ideal IMHO) and we shouldn't even consider PSR-0 as it is deprecated (because isn't WP the target of enough jokes in the PHP community already? I mean seriously? "Hey everyone... we just took a giant step forward into the future... and adopted a deprecated system for autoloading!!!" ya no one is going to mock that!).

we shouldn't allow for registration of custom autoload rules

Totally agree. Allowing a free for all will just make it harder to control everything (especially if this ever ventures into the realm of dependency management). I would go a step further and not even allow a class map, so themes and plugins have the option of converting to PSR or continuing to do things on their own.

#25 in reply to: ↑ 8 @F J Kaiser
12 months ago

Replying to giuseppe.mazzapica:

An example (among literally dozens) the class WP_MatchesMapRegex is located in a file named class-wp.php that is so named because also contains the class WP.

Not to talk about some classes that are placed pretty much arbitrarily: wpdb class is in wp-db.php and WP_Query class is in query.php.

Problem numero uno.

Replying to rmccue:

I think a staged approach to this is the best way. Firstly, let's make every class that's not always loaded (wp-admin, etc) loadable via autoloading. This is probably a large task in itself, as we need to switch to one-class-per-file with predictable file naming.

I strongly agree with that. Whatever solution there could be, the list of class and file names imo is priority 1. From there, further steps can be taken – to make the one class per file rule stick. I want to suggest to break this task in multiple tickets and proceed from there on:

  1. Build a list of classes
  2. Map list of classes to file names
  3. Fix file names + fix includes that break in this step
  4. Break out classes when there is more than one class in a file + fix includes … again
  5. Think about autoloading … again (but now with a strong base)

As I have done an throughout analysis (not by hand) of all classes, etc. in core (4.4), I can provide the base for step one. (Yes, I know, those are not the classes in core, but the used classes – hence base for step one).

	    Class                                  Matches REF        EXT min/Max PHP min/Max PHP all
	    -----------------------------------------------------------------------------------------
	    AMFReader                              1       user                   5.0.0                
	    AMFStream                              1       user                   5.0.0                
	    AVCSequenceParameterSetReader          1       user                   5.0.0                
	    AtomEntry                              1       user                   4.0.0                
	    AtomFeed                               1       user                   4.0.0                
	    AtomParser                                     user                   5.0.0                
	    Automatic_Upgrader_Skin                5       user                   5.0.0                
	    Bulk_Plugin_Upgrader_Skin              1       user                   5.0.0                
	    Bulk_Theme_Upgrader_Skin               1       user                   5.0.0                
	    Bulk_Upgrader_Skin                     2       user                   5.0.0                
	 CU COM                                    3       user                   4.0.0                
	    Core_Upgrader                          4       user                   5.0.0                
	    Custom_Background                      1       user                   5.0.0                
	    Custom_Image_Header                    1       user                   5.0.0                
	 C  DOMDocument                            6       dom        5.0.0       5.0.0                
	    DOMText                                2       dom        5.0.0       5.0.0                
	    DOMXPath                               6       dom        5.0.0       5.0.0                
	    DateTimeZone                           5       date       5.2.0       5.2.0                
	 C  Error                                  8       user                   4.0.0       5.1.0    
	    Exception                              41      Core       5.1.0       5.1.0                
	 C  Featured_Content                       1       user                   4.0.0       5.1.0    
	    File_Upload_Upgrader                   2       user                   5.0.0                
	    Gettext_Translations                   2       user                   5.0.0                
	    IXR_Base64                             1       user                   5.0.0                
	    IXR_Client                             3       user                   5.0.0                
	    IXR_ClientMulticall                            user                   5.0.0                
	    IXR_Date                               5       user                   5.0.0                
	    IXR_Error                              214     user                   5.0.0                
	    IXR_IntrospectionServer                        user                   5.0.0                
	    IXR_Message                            3       user                   5.0.0                
	    IXR_Request                            2       user                   5.0.0                
	    IXR_Server                             2       user                   5.0.0                
	    IXR_Value                              4       user                   5.0.0                
	 C  Imagick                                2       imagick    2.0.0a1     5.1.3                
	 C  ImagickPixel                           1       imagick    2.0.0a1     5.1.3                
	    Language_Pack_Upgrader                 5       user                   5.0.0                
	    Language_Pack_Upgrader_Skin            2       user                   5.0.0                
	 C  MO                                     2       user                   5.0.0                
	    MagpieRSS                              1       user                   5.0.0                
	    Memcache                               1       memcache   0.2         4.3.3                
	 C  NOOP_Translations                      1       user                   5.0.0                
	    PDO                                    1       PDO        5.1.0       5.1.0                
	  U PEAR                                   3       user                   4.0.0                
	 CU PEAR_Error                             1       user                   4.0.0                
	    PHPMailer                              1       user                   4.0.0       5.2.0RC1 
	 C  PO                                     24      user                   5.0.0                
	 C  POMO_CachedFileReader                  1       user                   5.0.0                
	 C  POMO_CachedIntFileReader                       user                   5.0.0                
	 C  POMO_FileReader                        1       user                   5.0.0                
	 C  POMO_Reader                            3       user                   5.0.0                
	 C  POMO_StringReader                      1       user                   5.0.0                
	    POP3                                   1       user                   5.0.0                
	    PasswordHash                           7       user                   5.0.0                
	    PclZip                                 165     user                   5.0.0                
	    Plugin_Installer_Skin                  2       user                   5.0.0                
	    Plugin_Upgrader                        7       user                   5.0.0                
	    Plugin_Upgrader_Skin                   1       user                   5.0.0                
	    RSSCache                               1       user                   5.0.0                
	    ReflectionClass                        1       Reflection 5.0.0       5.0.0                
	    SMTP                                   1       user                   4.0.0       5.1.0    
	 C  Services_JSON                          6       user                   5.0.0                
	    Services_JSON_Error                    2       user                   5.0.0                
	 C  SimplePie                              3       user                   4.0.0       5.3.0    
	    SimplePie_Author                               user                   5.0.0                
	    SimplePie_Cache                        3       user                   5.0.0                
	    SimplePie_Cache_DB                     1       user                   5.0.0                
	    SimplePie_Cache_File                   1       user                   5.0.0                
	    SimplePie_Cache_Memcache                       user                   5.0.0                
	    SimplePie_Cache_MySQL                          user                   4.0.0       5.1.0    
	    SimplePie_Caption                              user                   5.0.0                
	    SimplePie_Category                             user                   5.0.0                
	    SimplePie_Content_Type_Sniffer                 user                   5.0.0                
	    SimplePie_Copyright                            user                   5.0.0                
	    SimplePie_Core                                 user                   4.0.0                
	    SimplePie_Credit                               user                   5.0.0                
	    SimplePie_Decode_HTML_Entities         1       user                   5.0.0                
	    SimplePie_Enclosure                            user                   5.0.0                
	    SimplePie_Exception                    3       user                   4.0.0       5.1.0    
	    SimplePie_File                         2       user                   5.0.0                
	    SimplePie_HTTP_Parser                  2       user                   5.0.0                
	    SimplePie_IRI                          8       user                   5.0.0                
	    SimplePie_Item                                 user                   4.0.0       5.3.0    
	    SimplePie_Locator                              user                   5.0.0                
	    SimplePie_Misc                         28      user                   4.0.0       5.1.0    
	    SimplePie_Net_IPv6                     2       user                   5.0.0                
	    SimplePie_Parse_Date                   2       user                   5.0.0                
	    SimplePie_Parser                               user                   4.0.0       5.1.0    
	    SimplePie_Rating                               user                   5.0.0                
	    SimplePie_Registry                     6       user                   5.0.0                
	    SimplePie_Restriction                  1       user                   5.0.0                
	    SimplePie_Sanitize                     2       user                   5.0.0                
	    SimplePie_Source                               user                   5.0.0                
	    SimplePie_XML_Declaration_Parser               user                   5.0.0                
	    SimplePie_gzdecode                     1       user                   5.0.0                
	 C  SimpleXMLElement                       1       SimpleXML  5.0.1       5.0.1                
	 C  Snoopy                                         user                   5.0.0                
	 CU Sodium                                 2       user                   4.0.0                
	 C  Text_Diff                              6       user                   5.0.0                
	    Text_Diff_Engine_native                        user                   4.0.0                
	    Text_Diff_Engine_shell                         user                   4.0.0       4.0.4    
	    Text_Diff_Engine_string                        user                   4.0.0                
	    Text_Diff_Engine_xdiff                         user                   4.0.0                
	    Text_Diff_Op                           4       user                   5.0.0                
	    Text_Diff_Op_add                       7       user                   5.0.0                
	    Text_Diff_Op_change                    5       user                   5.0.0                
	    Text_Diff_Op_copy                      10      user                   5.0.0                
	    Text_Diff_Op_delete                    7       user                   5.0.0                
	    Text_Diff_Renderer                     2       user                   5.0.0                
	    Text_Diff_Renderer_inline              2       user                   4.0.0                
	    Text_MappedDiff                                user                   5.0.0                
	    Theme_Installer_Skin                   2       user                   5.0.0                
	    Theme_Upgrader                         6       user                   5.0.0                
	    Theme_Upgrader_Skin                    1       user                   5.0.0                
	 C  Translation_Entry                      6       user                   5.0.0                
	 C  Translations                           1       user                   5.0.0                
	    Twenty_Eleven_Ephemera_Widget                  user                   4.0.0       4.3.0    
	    Twenty_Fourteen_Ephemera_Widget                user                   5.0.0                
	 C  TypeError                              17      user                   4.0.0       5.1.0    
	    WP                                     1       user                   4.0.0       5.1.2    
	    WP_Admin_Bar                                   user                   5.0.0                
	    WP_Ajax_Response                       15      user                   5.0.0                
	    WP_Automatic_Updater                   5       user                   5.0.0                
	    WP_Comment                             4       user                   4.0.0       5.1.0    
	    WP_Comment_Query                       5       user                   5.0.0                
	    WP_Comments_List_Table                 1       user                   5.0.0                
	    WP_Customize_Background_Image_Control  1       user                   5.0.0                
	    WP_Customize_Background_Image_Setting  1       user                   5.0.0                
	    WP_Customize_Color_Control             5       user                   5.0.0                
	    WP_Customize_Control                   12      user                   5.0.0                
	    WP_Customize_Cropped_Image_Control     1       user                   5.0.0                
	    WP_Customize_Filter_Setting            2       user                   5.0.0                
	    WP_Customize_Header_Image_Control      1       user                   5.0.0                
	    WP_Customize_Header_Image_Setting      1       user                   5.0.0                
	    WP_Customize_Image_Control             3       user                   5.0.0                
	    WP_Customize_Manager                   3       user                   4.0.0       5.1.2    
	    WP_Customize_Media_Control             1       user                   5.0.0                
	    WP_Customize_Nav_Menu_Auto_Add_Control         user                   5.0.0                
	    WP_Customize_Nav_Menu_Control                  user                   5.0.0                
	    WP_Customize_Nav_Menu_Item_Control     1       user                   5.0.0                
	    WP_Customize_Nav_Menu_Item_Setting     2       user                   4.0.0       5.1.0    
	    WP_Customize_Nav_Menu_Location_Control 1       user                   5.0.0                
	    WP_Customize_Nav_Menu_Name_Control             user                   5.0.0                
	    WP_Customize_Nav_Menu_Section          1       user                   5.0.0                
	    WP_Customize_Nav_Menu_Setting          2       user                   4.0.0       5.1.0    
	    WP_Customize_Nav_Menus                 1       user                   5.0.0                
	    WP_Customize_Nav_Menus_Panel           1       user                   5.0.0                
	    WP_Customize_New_Menu_Control          1       user                   5.0.0                
	    WP_Customize_New_Menu_Section          1       user                   5.0.0                
	    WP_Customize_Panel                     2       user                   5.0.0                
	    WP_Customize_Section                   5       user                   5.0.0                
	    WP_Customize_Setting                   5       user                   5.0.0                
	    WP_Customize_Sidebar_Section           1       user                   5.0.0                
	    WP_Customize_Site_Icon_Control         1       user                   5.0.0                
	    WP_Customize_Theme_Control             2       user                   5.0.0                
	    WP_Customize_Themes_Section            1       user                   5.0.0                
	    WP_Customize_Upload_Control            1       user                   5.0.0                
	    WP_Customize_Widgets                   1       user                   4.0.0       5.2.0    
	    WP_Date_Query                          4       user                   5.0.0                
	    WP_Dependencies                        2       user                   5.0.0                
	    WP_Embed                               1       user                   5.0.0                
	 C  WP_Error                               319     user                   5.0.0                
	    WP_Feed_Cache                                  user                   5.0.0                
	    WP_Feed_Cache_Transient                1       user                   5.0.0                
	    WP_Filesystem_Base                     4       user                   5.0.0                
	    WP_Filesystem_Direct                           user                   5.0.0                
	    WP_Filesystem_FTPext                           user                   5.0.0                
	    WP_Filesystem_SSH2                             user                   5.0.0                
	    WP_Filesystem_ftpsockets                       user                   5.0.0                
	  U WP_HTTP                                1       user                   4.0.0                
	    WP_HTTP_Fsockopen                              user                   5.0.0                
	    WP_HTTP_IXR_Client                     2       user                   5.0.0                
	    WP_HTTP_Proxy                          2       user                   5.0.0                
	    WP_HTTP_Response                       1       user                   5.0.0                
	  U WP_HTTP_Streams                        1       user                   4.0.0                
	    WP_Http                                12      user                   5.0.0                
	    WP_Http_Cookie                         2       user                   5.0.0                
	    WP_Http_Curl                                   user                   5.0.0                
	    WP_Http_Encoding                       5       user                   5.0.0                
	    WP_Http_Streams                                user                   5.0.0                
	    WP_Image_Editor                        2       user                   5.0.0                
	    WP_Image_Editor_GD                             user                   5.0.0                
	    WP_Image_Editor_Imagick                        user                   5.0.0                
	    WP_Importer                                    user                   5.0.0                
	    WP_Internal_Pointers                           user                   5.0.0                
	    WP_Links_List_Table                            user                   5.0.0                
	    WP_List_Table                          14      user                   5.0.0                
	    WP_Locale                              7       user                   5.0.0                
	    WP_MS_Sites_List_Table                         user                   5.0.0                
	    WP_MS_Themes_List_Table                        user                   4.0.0       5.0.2    
	    WP_MS_Users_List_Table                         user                   5.0.0                
	    WP_MatchesMapRegex                     3       user                   5.0.0                
	    WP_Media_List_Table                            user                   5.0.0                
	    WP_Meta_Query                          6       user                   5.0.0                
	    WP_Nav_Menu_Widget                             user                   5.0.0                
	    WP_Network                             12      user                   5.0.0                
	    WP_Object_Cache                        1       user                   4.0.0       5.2.0    
	    WP_Plugin_Install_List_Table                   user                   5.0.0                
	    WP_Plugins_List_Table                          user                   5.0.0                
	    WP_Post                                8       user                   5.0.0                
	    WP_Post_Comments_List_Table                    user                   5.0.0                
	    WP_Posts_List_Table                            user                   5.0.0                
	    WP_Press_This                          1       user                   5.0.0                
	    WP_Query                               15      user                   5.0.0                
	    WP_REST_Request                        3       user                   4.0.0       5.3.0    
	    WP_REST_Response                       4       user                   5.0.0                
	    WP_REST_Server                                 user                   4.0.0       5.1.0    
	    WP_Rewrite                             1       user                   4.0.0       5.1.0    
	    WP_Role                                3       user                   5.0.0                
	    WP_Roles                               3       user                   5.0.0                
	 C  WP_Screen                              5       user                   5.0.0                
	    WP_Scripts                             3       user                   5.0.0                
	    WP_Session_Tokens                      10      user                   5.0.0                
	    WP_SimplePie_File                              user                   5.0.0                
	    WP_SimplePie_Sanitize_KSES             1       user                   5.0.0                
	    WP_Site_Icon                           1       user                   5.0.0                
	    WP_Styles                              2       user                   5.0.0                
	    WP_Tax_Query                           2       user                   5.0.0                
	    WP_Term                                4       user                   5.0.0                
	    WP_Terms_List_Table                            user                   5.0.0                
	 C  WP_Text_Diff_Renderer_Table            1       user                   5.0.0                
	    WP_Text_Diff_Renderer_inline                   user                   5.0.0                
	    WP_Theme                               17      user                   4.0.0       5.4.0    
	    WP_Theme_Install_List_Table                    user                   5.0.0                
	    WP_Themes_List_Table                   1       user                   4.0.0       5.0.2    
	    WP_Upgrader                            4       user                   5.0.0                
	    WP_Upgrader_Skin                       8       user                   5.0.0                
	    WP_User                                17      user                   5.0.0                
	    WP_User_Meta_Session_Tokens                    user                   5.0.0                
	    WP_User_Query                          3       user                   5.0.0                
	 C  WP_User_Search                                 user                   5.0.0                
	    WP_Users_List_Table                            user                   5.0.0                
	    WP_Widget                              16      user                   5.0.0                
	    WP_Widget_Archives                             user                   5.0.0                
	    WP_Widget_Area_Customize_Control       1       user                   5.0.0                
	    WP_Widget_Calendar                             user                   5.0.0                
	    WP_Widget_Categories                           user                   5.0.0                
	    WP_Widget_Factory                      1       user                   5.0.0                
	    WP_Widget_Form_Customize_Control       1       user                   5.0.0                
	    WP_Widget_Links                                user                   5.0.0                
	    WP_Widget_Meta                                 user                   5.0.0                
	    WP_Widget_Pages                                user                   5.0.0                
	    WP_Widget_RSS                                  user                   5.0.0                
	    WP_Widget_Recent_Comments                      user                   5.0.0                
	    WP_Widget_Recent_Posts                         user                   5.0.0                
	    WP_Widget_Search                               user                   5.0.0                
	    WP_Widget_Tag_Cloud                            user                   5.0.0                
	    WP_Widget_Text                                 user                   5.0.0                
	    WP_oEmbed                              3       user                   4.0.0       5.1.0    
	    WP_oEmbed_Controller                   1       user                   5.0.0                
	    Walker                                 7       user                   5.0.0                
	    Walker_Category                        1       user                   5.0.0                
	    Walker_CategoryDropdown                1       user                   5.0.0                
	    Walker_Category_Checklist              1       user                   5.0.0                
	    Walker_Comment                         2       user                   5.0.0                
	    Walker_Nav_Menu                        3       user                   5.0.0                
	    Walker_Nav_Menu_Checklist              3       user                   5.0.0                
	    Walker_Nav_Menu_Edit                           user                   5.0.0                
	    Walker_Page                            1       user                   5.0.0                
	    Walker_PageDropdown                    1       user                   5.0.0                
	    XMLReader                              1       xmlreader  5.0.0       5.0.0                
	 C  ZipArchive                             1       zip        1.6.0       5.2.0                
	    _WP_Dependency                         1       user                   5.0.0                
	 C  _WP_Editors                            4       user                   4.0.0       5.5.0    
	    _WP_List_Table_Compat                  2       user                   5.0.0                
	    ftp                                    1       user                   4.0.0       4.3.0    
	    ftp_base                               2       user                   4.0.0       4.0.4    
	 C  getID3                                 18      user                   5.0.0                
	    getid3_ac3                             2       user                   5.0.0                
	    getid3_apetag                          1       user                   5.0.0                
	    getid3_asf                                     user                   4.0.0       5.3.0    
	    getid3_dts                             1       user                   5.0.0                
	    getid3_exception                       8       user                   4.0.0       5.1.0    
	    getid3_flac                            2       user                   5.0.0                
	    getid3_flv                                     user                   5.0.0                
	    getid3_handler                         14      user                   5.0.0       5.1.0    
	    getid3_id3v1                           2       user                   5.0.0                
	    getid3_id3v2                           3       user                   5.0.0                
	    getid3_lib                             901     user                   4.0.0       5.1.0    
	    getid3_lyrics3                                 user                   5.0.0                
	    getid3_matroska                                user                   4.0.0       5.1.0    
	    getid3_mp3                             6       user                   5.0.0                
	  U getid3_mpeg                            1       user                   4.0.0                
	    getid3_ogg                             2       user                   5.0.0                
	    getid3_quicktime                               user                   5.0.0                
	    getid3_riff                            8       user                   4.0.0       5.1.0    
	 CU idna_convert                           2       user                   4.0.0                
	    parent                                 102     Core       5.0.0       5.0.0                
	 CU pear                                           user                   4.0.0                
	    phpmailerException                     25      user                   4.0.0       5.1.0    
	    self                                   263     Core       5.0.0       5.0.0                
	    stdClass                               34      Core       4.0.0       4.0.0                
	 C  wp_atom_server                                 user                   5.0.0                
	    wp_xmlrpc_server                               user                   4.0.0       5.1.0    
	    wpdb                                   1       user                   5.0.0              
	    -----------------------------------------------------------------------------------------  
	    Total [294]                                                           5.2.0       5.4.0 

When you read until here: you can close your mouth now.

Last edited 12 months ago by F J Kaiser (previous) (diff)

#26 follow-up: @schlessera
12 months ago

I want to add some clarifications to some of what's been written before.

First of all, I want to make it clear that I am not against the goal of having a public API for letting plugins & themes register autoloader mappings at runtime. However, I see this as the last step in a long term effort, not the first step.

I think that, starting out with the public API spells disaster while we still have the following issues:

  1. we are still bound to PHP 5.2 compatibility (no namespaces);
  2. we have no clear naming rules for some of the elements we are trying to write the API for;
  3. Core is in a state where it does not know what is needed from this API, and couldn't fulfil its requirements.

The "just use Composer" approach does not preclude adding a public API at some point that lets plugins & themes add autoloader mappings at runtime. However, it postpones deciding on the final structure of this API until a point in time where the above three issues have been solved. Composer is just a fast way to be able to attack these systematically.

Anther thing to consider is that the one true benefit that an autoloader brings to the table that you can't easily have without is the way you can "just use" libraries. You don't have to call any bootstrapping code that will include lots of stuff, you don't have to include yourself, you don't have to check if an external class exists first. If you now start with a custom autoloader specific to WordPress Core, you might be able to autoload a plugin's classes without includes (you saved a few lines of typing), but you blocked access to that biggest benefit of an autoloader. Let's be honest: all the great modern PHP libraries can saves us hours and days of custom development when working on a larger project will not be rewritten to make them work with a WP-specific autoloader! So each of them will need special forks/bridges/adapters to use within WordPress. That's not really progress, if you ask me...

With our approach, plugins and themes could start using the autoloader with WordPress 4.6 already, for instance. Changes at Core WordPress could then be made step by step, when the time is right for each of these steps.

Yes, they could start using the autoloader. My personal belief is that this will lead to loads of breaking code further down the line, as we will have lots of plugins & themes that don't use namespaces and will produce all sorts of conflicts. We will then end up with thousands of problematic plugins, and no way to deal with the conflicts they produced. What's worse, with the backwards compatibility policy of WordPress, we would not even be able to make drastic changes to the autoloader itself to remedy that situation.

Presence of an autoloader makes naming conflicts worse, as they may not be immediately obvious. That's why I think namespaces are an absolute must when using an autoloader.

we shouldn't allow for registration of custom autoload rules

Agree, PSR-4/WP-? & classmaps should be more than enough.

The biggest problem with using Composer as a solution for WP is the sheer number of sites and servers that will not have it installed. Therefore the only viable option IMHO is to bake an autoloader into core.

That's not an issue. For the approach I recommended above, Composer would only be used at development time for as long as WP stays at PHP 5.2. After that, you can just as well include Composer with WordPress, it does not need to be installed globally. So, if you want to bake in an autoloader into WordPress Core, why not just use the one that the rest of the PHP world uses?

"Hey everyone... we just took a giant step forward into the future... and adopted a deprecated system for autoloading!!!"

I might be wrong, but I assume that, when talking about an autoloader, you'll get a similar reaction for "not using namespaces", as well as for "not using Composer". As far as I am concerned, the simple fact of starting to build a custom autoloader is "reinventing the wheel from the past" as I wrote further above.

  1. Build a list of classes
  2. Map list of classes to file names
  3. Fix file names + fix includes that break in this step
  4. Break out classes when there is more than one class in a file + fix includes … again
  5. Think about autoloading … again (but now with a strong base)

Yes, I also recommend an approach similar to this. In addition to that, if we're using a composer.json for the autoloading from the beginning, we have an easy way to test and immediately use the changes, and "fixing includes" would be replaced by "add to composer.json".

#27 in reply to: ↑ 26 @dnaber-de
12 months ago

Replying to schlessera:

Anther thing to consider is that the one true benefit that an autoloader brings to the table that you can't easily have without is the way you can "just use" libraries. You don't have to call any bootstrapping code that will include lots of stuff, you don't have to include yourself, you don't have to check if an external class exists first. If you now start with a custom autoloader specific to WordPress Core, you might be able to autoload a plugin's classes without includes (you saved a few lines of typing), but you blocked access to that biggest benefit of an autoloader. Let's be honest: all the great modern PHP libraries can saves us hours and days of custom development when working on a larger project will not be rewritten to make them work with a WP-specific autoloader! So each of them will need special forks/bridges/adapters to use within WordPress. That's not really progress, if you ask me...

I don't get it. Why they would have to be rewritten? Most of them are PSR-4 compatible, so you would just have to register their directories/namespaces to the autoloader.

we shouldn't allow for registration of custom autoload rules

Agree, PSR-4/WP-? & classmaps should be more than enough.

Disagree. In my eyes, that would be a violation of the open/closed principle. Psr-4 is sure used by everyone out there but in fact it's not an optimal solution as it is case-sensitive (I mentioned this earlier). I wouldn't like to restrict developers in this way.

So, if you want to bake in an autoloader into WordPress Core, why not just use the one that the rest of the PHP world uses?

Because the implementation of that autoloader is not SOLID.

In my opinion, an ideal solution would be to provide a flexible core autoloader and afterwards integrating composer for dependency management by pluggin in the core autoloader to composer via a composer plugin.

However, the other way around would also be fine for me.

#28 @schlessera
12 months ago

I don't get it. Why they would have to be rewritten? Most of them are PSR-4 compatible, so you would just have to register their directories/namespaces to the autoloader.

No, unfortunately not. The nice thing about a package manager (which pretty much all of these libraries now use), is that it does not only take care of the direct dependencies, but also of the dependencies' dependencies, recursively. This is was encourages developers to reuse existing code by just pulling it in, instead of copying it somewhere.

So, if you don't use a system like Composer, you will not only have to register that library's namespace, but also the namespace of all of its dependencies at each level and sublevel. Composer's main work is done recursively, which is a HUGE benefit.

Disagree. In my eyes, that would be a violation of the open/closed principle. Psr-4 is sure used by everyone out there but in fact it's not an optimal solution as it is case-sensitive (I mentioned this earlier). I wouldn't like to restrict developers in this way.

I was not talking about restricting the code (implementations are details to be discussed once the general direction has been set), but rather enforcing a limited set of standard approaches, instead of allowing whatever creatives (ab)uses developers can think of, only to find out months and years down the road that they created a huge mess. Think of this as the namespace version of "decisions, not options".

Because the implementation of that autoloader is not SOLID.

I'm not sure what you mean. Composer is certainly not 100% SOLID (I'd love to see any bigger project that is, to be honest), but I am pretty sure they come closer to that goal than the WordPress codebase. Is a specific "design pattern usage" of Composer the issue?

In my opinion, an ideal solution would be to provide a flexible core autoloader and afterwards integrating composer for dependency management by pluggin in the core autoloader to composer via a composer plugin.

I'm not saying that we should reduce ourselves to a build-time only Composer autoloader. Having something that can be managed at runtime would be great! But I would recommend not building a custom autoloader until some preconditions have been met (PHP 5.3 with namespaces, WP autoloadable). We'll be in a better position to build the autoloader correctly from the start, and put in sane requirements (namespaces!) that avoid creating a huge conflict-ridden pool of buggy plugins. Composer would allow us to quickly have a working playground to already use and improve autoloading in Core until the preconditions have been met, while probably improving memory usage in the process.

@rmccue
12 months ago

Alternative implementation of autoloading concept

@rmccue
12 months ago

SPL autoload compatibility shim/polyfill

#29 follow-up: @rmccue
12 months ago

I think whether we adopt Composer usage or not isn't hugely relevant to this conversation; while it has the ability to solve some of the problems here, it's also a huge architectural change to get it to the point where it would make sense. Let's discuss this on a new ticket rather than here.


I do love the concept of autoloading, and I think the work here has been productive, but I don't think the current implementation is appropriate for core. It's a bit too heavy and general-purpose.

I have an alternative implementation that tries to stay as lightweight as possible. It implements the bare minimum to support autoloading core itself, which is likely also useful for plugins, but isn't necessarily designed to be. We can look at expanding this in the future, but this is just a Minimal Viable Patch. I think we can all agree that some form of autoloading is better than what we have right now. :)

The attached file contains ~70 lines of (undocumented) code that includes PSR-0, WP-style (class-...php) and classmap style autoloaders, plus ~170 lines of configuration for core itself. (The rest of the code is to generate a table.) Note that this is just the lookup at the moment so we can get the mapping sorted first (adapting this to the actual autoloader will be only a line or two more). The $wp_autoloaders bit is temporary; in the final implementation, I'd rather we avoid treating the autoloaders any different to any other normal PHP autoloader. That is, let's use spl_autoload_register directly rather than implementing our own autoloading stack on top of it.

Here's what that mapping looks like. This covers the vast majority of classes in core, with some exceptions. The ones I haven't tackled here are mostly things we should split out, or are never going to be autoloaded.

We can look at expanding and changing this in the future, but this takes us from no autoloading right now to having the key pieces we need for it. This is kind of like building demolition: we're assembling the scaffolding first to allow us to take the building down slowly. Once this is in place, we can start removing some of the require statements safely and evaluating how it affects core as we go.

(I've also attached the autoloader compatibility file; you can check the consistency using 3v4l.org. This should polyfill the SPL autoloader nicely.)

I'd love to hear your thoughts on this alternative approach.

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


12 months ago

#31 in reply to: ↑ 29 ; follow-up: @tfrommen
12 months ago

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)&mdash;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

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

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

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

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

@rmccue
12 months ago

Proposed actual code for the autoloaders

#32 in reply to: ↑ 31 @rmccue
12 months ago

Added 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 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)&mdash;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.

#33 follow-up: @joehoyle
12 months ago

I'm liking the simplicity of @rmccue's patch, the core of which I see as just being the SPL polyfill. A bunch of utility autoloaders is handy I guess, however given the simple callable nature of spl_autoload_register then I see those as handy utilities rather than integral to this. I'd be most interested in getting the SPL polyfill committed and work from there, it _seems_ like that would be a good first step.

For now I'm +1 on only shipping autoloaders that are used within Core.

#34 in reply to: ↑ 33 @rmccue
12 months ago

Replying to joehoyle:

A bunch of utility autoloaders is handy I guess, however given the simple callable nature of spl_autoload_register then I see those as handy utilities rather than integral to this.

FWIW, I'd also be happy to just have some functions with the autoloading built into them, but it makes sense to make these reusable because it's so little work to do so, and we can reuse it ourselves too.

#35 @dnaber-de
12 months ago

The reason of »making things reusable« is not because it's little work. The reason is interoperability with things outside of the WP universe, even with things that aren't written yet. The other reason is testability.

Even though, WP core isn't anything but object oriented yet, that does not mean that new components shouldn't be written object oriented and that means, following the SOLID principles.

Just my two cents to the discussion of »over complicated« implementations ;)

@rmccue I'll have a look at your patch and will respond to it soon.

#36 @atimmer
11 months ago

I think the most important thing about the autoloader is that WordPress should eventually move towards PSR-4. The reasons being that interoperability is important and the PHP community has done a lot of work in this field and I think we should follow their lead. This depends on dropping PHP5.2 support and moving classes to their correct folder.

The biggest thing we can do in the meantime is to remove the expectation of people that they have to require a file to use a certain class. So anyone writing code will be comfortable with just writing: new Class_That_Has_Not_Been_Loaded_Yet(). Because of this reason, I think we should implement a class map autoloader which lets developers just do new Class(); anywhere they like. Then once we drop PHP5.2 we can just switch all new classes to PSR-4.

I don't have a strong opinion about composer vs custom because the eventual goal should be PSR-4 which is the same and the user can ignore the specific implementation because it is a standard.

Another thing: When we make a class map, I think it is a wise choice to make the generation of the class map automated. That will prevent a lot of headaches later on when we start moving files.

#37 follow-up: @schlessera
11 months ago

I think whether we adopt Composer usage or not isn't hugely relevant to this conversation; while it has the ability to solve some of the problems here, it's also a huge architectural change to get it to the point where it would make sense. Let's discuss this on a new ticket rather than here.

We're not talking about using Composer for dependency management here, but rather using Composer as a build tool to generate an autoloader through the de factor standard PHP autoloader generator, instead of rolling our own implementation, so I would say it is highly relevant.

I won't discuss any of the code in detail, as I think we're still not sure about some principalities here. I just want to note that, regarding the polyfill, you need to keep in mind that __autoload() will probably throw a notice with PHP 7.1 (https://wiki.php.net/rfc/deprecations_php_7_1). Also, I think the polyfill should not throw exceptions, as it will only be used with 5.2, and prior to 5.3 these are not able to be caught and will throw fatal errors (see note in http://php.net/manual/en/language.oop5.autoload.php).

The discussion so far has been to decide whether to use a custom implementation to allow adding namespaces at runtime so that plugins and themes can also use it from the get-go (@dnaber's initial proposal), or to stick with autoloading for Core for now until we're more confident in what we're doing (and have namespaces!) by using Composer as a build tool to generate the autoloading.

@rmccue The polyfill is a good idea (for as long as introducing deprecated code is preferable to bumping the PHP version), but the rest of your proposal now unites only the drawbacks of both of these methods, by providing a build-time-only, Core-only autoloader, but not using the standard, mature, tested, highly optimized one from Composer. If we only want to have a classmap for Core for now, why not just use that 1 Composer line and have it generate something we can rely upon without much further thought, and know that we're not locking ourselves down for the future?

I completely agree to @atimmer's entire comment.

So, I would recommend discussing the polyfill and the autoloader implementation as two separate issues, and for the autoloader implementation I don't see any particular advantage (but some disadvantages) to @rmccue's proposal over just having it be automatically generated through Composer. If all we want is a class map for Core, there's already a tool for that.

#38 in reply to: ↑ 37 @rmccue
11 months ago

Replying to schlessera:

We're not talking about using Composer for dependency management here, but rather using Composer as a build tool to generate an autoloader through the de factor standard PHP autoloader generator, instead of rolling our own implementation, so I would say it is highly relevant.

Going from no autoloading to some is a huge step; how we actually create the autoloader(s) is a bit more of an implementation detail. I don't have a huge issue with using Composer to generate a classmap, but I want us to take small steps to ensure we get something in here. :)

I won't discuss any of the code in detail, as I think we're still not sure about some principalities here. I just want to note that, regarding the polyfill, you need to keep in mind that __autoload() will probably throw a notice with PHP 7.1 (https://wiki.php.net/rfc/deprecations_php_7_1).

Won't be an issue, as the whole file will only be loaded for 5.2-without-SPL.

Also, I think the polyfill should not throw exceptions, as it will only be used with 5.2, and prior to 5.3 these are not able to be caught and will throw fatal errors (see note in http://php.net/manual/en/language.oop5.autoload.php).

The autoloader itself doesn't throw exceptions, just the autoloader registration code (spl_autoload_register); it's up to the actual autoloader implementations themselves to decide whether the compatibility here matters or not.

@rmccue The polyfill is a good idea (for as long as introducing deprecated code is preferable to bumping the PHP version)

The PHP version decision isn't mine to make, so I'd prefer to avoid it. :) I've split the polyfill into a separate ticket at #36926, as we can ship it independently of the autoloader implementation.

but the rest of your proposal now unites only the drawbacks of both of these methods, by providing a build-time-only, Core-only autoloader, but not using the standard, mature, tested, highly optimized one from Composer. If we only want to have a classmap for Core for now, why not just use that 1 Composer line and have it generate something we can rely upon without much further thought, and know that we're not locking ourselves down for the future?

I actually want to remove the entire classmap code from core; the only reason my code includes this is because we currently don't have a clean, standardised mapping (whether PSR-0, PSR-4, or WP-style) for various legacy reasons. Ideally, the classmap will disappear entirely from core.

That's not to say I don't like classmaps, but I want it to primarily be a performance optimisation built on top of a clean system, rather than the current hacky approach that doesn't really improve how terrible the system in WP is right now. Independently of autoloading or anything, the current approach makes it hard to actually work with core.

So tl;dr: I'm in favour of automated classmap generation once we have a stable class -> file mapping system. Let's stick to small patches that we can ship quickly rather than trying to reform all of WP at once.

#39 follow-up: @schlessera
11 months ago

Going from no autoloading to some is a huge step; how we actually create the autoloader(s) is a bit more of an implementation detail. I don't have a huge issue with using Composer to generate a classmap, but I want us to take small steps to ensure we get something in here. :)

Yes, totally agree. But adding Composer autoloading is adding 1 tiny file (composer.json) and adding 1 line of PHP to one of the bootstrapping files (like wp-load.php): require_once( ABSPATH . '/vendor/autoload_52.php' );. This results in a working autoloader that keeps everything up-to-date as Core classes are changed.

Also agree with my two PHP observations being no real issues, just added them for reference.

I've split the polyfill into a separate ticket at #36926, as we can ship it independently of the autoloader implementation.

Yes, one should not block the other.

I actually want to remove the entire classmap code from core; the only reason my code includes this is because we currently don't have a clean, standardised mapping (whether PSR-0, PSR-4, or WP-style) for various legacy reasons. Ideally, the classmap will disappear entirely from core.
That's not to say I don't like classmaps, but I want it to primarily be a performance optimisation built on top of a clean system, rather than the current hacky approach that doesn't really improve how terrible the system in WP is right now. Independently of autoloading or anything, the current approach makes it hard to actually work with core.
So tl;dr: I'm in favour of automated classmap generation once we have a stable class -> file mapping system.

I just referenced the classmap as this is what you had submitted. Composer can actually work with any imaginable way of locating the classes, including a classmap or PSR-4.
The classmap is more of an intermediary step to have autoloading work while we don't have a standardized file/naming structure yet. Composer could take us all the way from first temporary/intermediary steps to final implementation in an automated, robust way.
The nice thing about the classmap though, is that it can make autoloading work right away, letting us remove the majority of the require statements, and still let us rearrange files in an ongoing process.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


11 months ago

#41 in reply to: ↑ 39 @jtsternberg
11 months ago

Replying to schlessera:

Yes, totally agree. But adding Composer autoloading is adding 1 tiny file (composer.json) and adding 1 line of PHP to one of the bootstrapping files (like wp-load.php): require_once( ABSPATH . '/vendor/autoload_52.php' );. This results in a working autoloader that keeps everything up-to-date as Core classes are changed.

This makes total sense to me. Running composer update as part of the grunt build process would ensure the class map was always up-to-date, even when we begin trying to fix the class/file-naming conventions. The composer class-map generator would have us covered. Sounds like the alternative is a class-map that has to be maintained by hand? Not sure I understand what the benefit there is? Unless it's about cognitive overhead?

#42 @rmccue
11 months ago

  • Keywords 2nd-opinion added

We had a long chat about this in the Post Status Slack (raw log for posterity). The main arguments for Composer appear to be:

  • We wouldn't have to maintain an autoloader ourselves.
  • Composer can generate classmaps automatically, so we wouldn't have to edit the classmap if/when we add new classes.

The main arguments against my proposal:

  • Adding autoloader support could be a blocker against adding Composer later. (It removes some of the need for Composer.)
  • Having our own autoloader reinvents the wheel, as we're maintaining the code ourselves.

My response to these:

  • Adding Composer is an entirely separate issue that needs agreement from the leads, whereas just adding basic autoloaders is a smaller discussion, and mostly a technical one.
  • Maintaining our own autoloaders is not unreasonable. The code I've attached is 47 lines of autoloader code in 3 classes.
  • The classmap is temporary, with the long-term goal being to eliminate it and move our code to match the WP standard of Some_Class_Name -> class-some-class-name.php.

Let me know if I missed any key arguments either way there. :)

I'd like to keep the Composer discussion happening, but I think it's a separate discussion. I don't believe implementing our own autoloaders holds back supporting Composer later in any way.

I don't think we managed to convince each other either way, so I think another committer or a lead should chime in to steer the conversation. :)

#43 @schlessera
11 months ago

Thanks for updating the ticket with our discussion, @rmccue.

I'd like to add some details to the summary above.

Additional arguments for Composer:

  • Composer can just as well generate any autoloader from the first temporary classmaps to the final structured layout using whatever rules we come up with, so it provides the entire update path from first tests to final implementation.
  • Composer is very mature, it has been tested and optimized (and used at the largest scales) for 5 years.

Additional arguments against @rmccue 's proposal specifically, but also against any custom implementation in general:

  • A custom implementation again creates some distance between WordPress and the larger PHP ecosystem.
  • A custom implementation will probably result in a custom autoloader to be used for plugins & themes later on as well, which will eventually cause all sorts of issues with the now de facto standard autoloader already used in a lot of plugins (Composer).
  • Planning a custom implementation while we still don't have namespaces will result in an implementation that will be too easy to break (naming conflicts).

Observations regarding @rmccue 's responses to these objections:

Adding Composer is an entirely separate issue [...]

I repeat again that we are not talking about using Composer for dependency management, we are talking about using Composer's autoloader generation as a build-time tool. We already have npm (the node-js command line build tool) as a build-time tool within Core, so why should it not be obvious to also have Composer ( the php command line build tool) as a build-time tool within Core?

Maintaining our own autoloaders is not unreasonable.

No, but neither is there any advantage to be had through that additional manual effort. It is neither sensible, nor productive.

The classmap is temporary [...]

But the code also includes interfaces & classes. Although they are meant to be temporary, we all know how temporary stuff is dealt with in a codebase that makes backwards-compatibility an absolute priority.
I would guess that it will just be the too-briefly-planned start of an elaborate coding effort to create a WordPress autoloader that can support all edge cases that the rest of the codebase can come up with.

I agree that there's two fundamentally different approaches being discussed here, and while I understand the reasons why @rmccue would like to proceed how he proposed, I still think this to be an approach we'll regret in the future, adding one or more example to the list of problems where WordPress didn't care to learn from the mistakes of other platforms.

So, for what it's worth, my position is to either go with Composer as a build-time tool or to prefer not to have an autoloader in Core at all until conditions have changed enough to rediscuss (namespaces!).

#44 @schlessera
11 months ago

Following up on further discussion with @rmccue , I want to detail a specific use case here, to illustrate why having a custom autoloader in Core would probably cause issues in the future, as this does not seem to be clear for everyone.

PHP has a very strict limitation in that each class can only ever be defined once. So, if you try to define a class a second time, you'll get an error.

Using an autoloader will remove that error, as the class definition is only loaded the first time the class is encountered and is not known yet. This creates another problem though. If you still have two class definitions with the same name, you don't know which one will get loaded. The autoloader will just load the first require statement it encounters, and never see the second definition. This is no issue if both of the class definitions are the same, but if you have AClass v1.0.0 & AClass v2.0.0, you can't be sure which one will get loaded. If v1.0.0 happens to be encountered first, all code relying on v2.0.0 will produce random errors, which are hard to debug, because they might not even appear in a consistent way.

Every autoloader implementation will face this issue.

What is currently considered best practice (mostly used for larger projects), is to have Composer deal with generating the autoloaders, and let it do so recursively from the site root (a site-wide Composer install). Composer will generate 1 single autoloader that will include the requested autoloaders at each sub-level, and always try to compare the version requirements to make sure that such an issue as described above can not happen. In this scenario, the WordPress Core, also being part of the site root, is also pulled in via Composer.

If it encounters a conflict, it will immediately throw an error, which is much preferable to bugs that you can hardly replicate. So, generating the autoloader through Composer will either work reliably, or immediately throw an error pointing you to the exact plugin/theme/library that is causing an issue.

If you now include a custom autoloader within WordPress Core, Composer will not be able to include this autoloader in its set of checks. So, plugins' libraries' versions will not be verified against WordPress' libraries' versions, and the WordPress Core autoloading code will not be included in the one generated at the site root.

You can not even control anymore which autoloader will get precedence, because the loading order of the autoloader does not equal the loading order of their respective classes. As the classes are only loaded when they are actually needed to be referenced, you might get either one, depending on what exact path you ran through the codebase. So you'll end up with completely unreliable code that might break without any readily apparent patterns.

I am currently one of the people who are heavily using Composer with such a site-wide autoloader with large projects. This all works perfectly (and much better than any pre-Composer code), as long as the libraries (Core, plugins, themes, ...) either include a Composer autoloader, or no autoloader at all.

The reason why this is used is because there's no other way to get different sets of autoloaders to work reliably otherwise, because of this inherent PHP limitation. There's namespaces to avoid collisions to even happen at all, but they only work for a certain category of code. For code such as reusable libraries, which might be used in several places at once (Core + 5 plugins, each working with the same library), you are certain to have the exact same namespace used in several locations, and they cannot be resolved properly through the autoloader alone. An external check has to be made to ensure that they never want to include conflicting versions, there's no way around that.

So, considering the above, my assertions would be that:

  • Reliable autoloading in PHP will eventually need namespaces + Composer + dependency management
  • A custom autoloader is certain to negatively impact reliable autoloading in PHP

Note:
I did not want to talk about dependency management here to not veer discussion into the wrong direction. However, my last discussion with @rmccue made me aware that it cannot be completely ignored, as autoloading for an entire project (not just WP alone) cannot possibly work reliably without dependency management. And while I only want to discuss a Core autoloader for now (so please don't focus on the dependency management part now), I felt I needed to explain my reasons why I think a custom autoloader would prove to be problematic in the future. Only considering Core in isolation will not reveal these issues.

#45 follow-ups: @boonebgorges
11 months ago

A Composer-generated classmap seems like the right way forward here. I don't see why we should reinvent the wheel, even if it only takes 47 lines to do it.

I have some implementation questions about how a Composer classmap would work alongside the WP codebase, which could probably be answered with a patch from someone with more Composer-fu than I've got. Putting a composer.json in the root of the repo checkout and running composer dump-autoload puts the autoloader into /vendor. What's the best practice for moving it into ABSPATH? Should we have a (Grunt?) wrapper that runs dump-autoload and then moves vendor into src, intended to be run pre-commit?

#46 in reply to: ↑ 45 @CoenJacobs
11 months ago

Replying to boonebgorges:

A Composer-generated classmap seems like the right way forward here. I don't see why we should reinvent the wheel, even if it only takes 47 lines to do it.

I couldn't agree more.

I have some implementation questions about how a Composer classmap would work alongside the WP codebase, which could probably be answered with a patch from someone with more Composer-fu than I've got. Putting a composer.json in the root of the repo checkout and running composer dump-autoload puts the autoloader into /vendor. What's the best practice for moving it into ABSPATH? Should we have a (Grunt?) wrapper that runs dump-autoload and then moves vendor into src, intended to be run pre-commit?

Let's answer the question on why to move it to ABSPATH in the first place. The Composer autoloader works fine from outside the root of your files, it is actually designed to work in that way. Just loading vendor/autoload.php from really early on, say in wp-load.php would work fine. The only obstacle in the way of implementing a Composer autoloader would be mapping class names to WordPress standard file names, which is something Composer offers a way to do so by introducing a custom mapper.

The vendor directory will be placed in the same directory as where the composer.json lives, so if we want to have that in the src directory, that is where the composer.json file should live as well, in my opinion.

Last edited 11 months ago by CoenJacobs (previous) (diff)

#47 @boonebgorges
11 months ago

The Composer autoloader works fine from outside the root of your files, it is actually designed to work in that way. Just loading vendor/autoload.php from really early on, say in wp-load.php would work fine.

WordPress releases are built packages that contain files to be stored in ABSPATH. The autoupdate and installation code do not assume that it'll be possible to write to the directory tree outside of ABSPATH. I understand why it's best practice to keep vendor outside of src (it's a black box, and it's not "owned" by WordPress), and it would be OK to keep it this way in development mode, but when building a package for release, the files will have to be moved to inside of ABSPATH. (This is probably just an implementation detail, and I don't want to get hung up on it. I just want to be clear that this will likely be a requirement, and get a sense of how we'll achieve this requirement within the context of our existing build routines, so we can make an informed decision about what introducing a Composer-built autoloader would entail.)

#48 in reply to: ↑ 45 @afercia
11 months ago

Replying to boonebgorges:

I don't see why we should reinvent the wheel, even if it only takes 47 lines to do it.

+1 :)

#49 @schlessera
11 months ago

What's the best practice for moving it into ABSPATH? Should we have a (Grunt?) wrapper that runs dump-autoload and then moves vendor into src, intended to be run pre-commit?

No, just set the desired vendor folder from within your composer.json (relative to where that one is located):

{
    "config": {
        "vendor-dir": "src/vendor"
    }
}

#50 @schlessera
11 months ago

Of course, you can also name it something else. The point is to:

  • have a separate folder for third-party code that you can .gitignore in one go
  • have it be named so that people know this is external/generated

#51 in reply to: ↑ 45 @DvanKooten
11 months ago

Replying to boonebgorges:

A Composer-generated classmap seems like the right way forward here. I don't see why we should reinvent the wheel, even if it only takes 47 lines to do it.

This.

I'm strongly in favor of using Composer for autoloading as the main argument against it seems to be that we should be taking baby steps.

Using Composer just for autoloading for now seems to be the right baby step for me. It's not introducing any changes to WordPress its public API _and_ will work just for WP's core classes (what seems to be the goal here). It will also make things easier for real Composer support in the future (I dream of this day every single night).

The following example composer.json file scans wp-includes/ and wp-admin/ for classes and then dumps a PHP 5.2 compatible autoloader. It then copies the generated files to wp-includes/autoloader.

{
  "autoload": {
    "classmap": [
      "wp-includes/",
      "wp-admin/"
    ]
  },
  "config": {
    "vendor-dir": "wp-includes/autoloader"
  },
  "require": {
    "php": ">=5.2.14",
    "xrstf/composer-php52": "1.*"
  },
  "scripts": {
    "post-install-cmd": [
      "xrstf\\Composer52\\Generator::onPostInstallCmd"
    ],
    "post-update-cmd": [
      "xrstf\\Composer52\\Generator::onPostInstallCmd"
    ],
    "post-autoload-dump": [
      "xrstf\\Composer52\\Generator::onPostInstallCmd"
    ]
  }
}

Edit: scratch that, Composer uses relative directory paths so simply moving the autoloader won't work. @schlessera's approach is much better so I've updated the example to reflect his suggestion.

Last edited 11 months ago by DvanKooten (previous) (diff)

#52 @schlessera
11 months ago

The following example composer.json file scans wp-includes/ and wp-admin/ for classes and then dumps a PHP 5.2 compatible autoloader. It then copies the generated files to wp-includes/autoloader.

Yes, this is pretty much the same thing I did in my tests documented here: https://www.alainschlesser.com/adding-central-autoloader-wordpress/

However, I would recommend against moving the autoloader manually, this just introduces an unnecessary liability that might cause platform issues. I would just let Composer handle that as well, it includes platform-agnostic filesystem management that is tested on lots of platforms/environments.

My recommendation would be to just go with default Composer behavior (less surprises), and have WordPress Core simply assume that classes are present. Moving the autoloader into wp-includes gives the impression that this is Core-specific "source" code, while it is in reality only a build-time artifact.

EDIT: Ok, noticed the edit in the previous comment too late.

Last edited 11 months ago by schlessera (previous) (diff)

#53 @jorbin
8 months ago

#37816 was marked as a duplicate.

#54 @wonderboymusic
8 months ago

  • Keywords has-patch added; 2nd-opinion removed
  • Milestone changed from Future Release to 4.7

36335.diff makes this work, rips out a plethora of require call. A couple of unit integration tests are failing. Looking.

Let's see what our new reality can be.

#55 follow-up: @JohnPBloch
8 months ago

I really like the direction we're going with @wonderboymusic's latest patch. I'm pretty sure it won't cause any issues with sites already managed using Composer and doesn't preclude core from adding Composer support or plugin/theme autoloading down the road.

One thing I think might need to be added to the patch (at least, I didn't see it) is a polyfill for spl_autoload_register() like @rmccue outlined above. Never mind me, clearly not watching core closely enough.

Last edited 8 months ago by JohnPBloch (previous) (diff)

#56 in reply to: ↑ 55 @jeremyfelt
8 months ago

Replying to JohnPBloch:

One thing I think might need to be added to the patch (at least, I didn't see it) is a polyfill for spl_autoload_register() like @rmccue outlined above.

This is available (via Requests requirements) as of [37636] in 4.6.

#57 follow-up: @jorbin
8 months ago

If the solution is going to include a composer file, related: #23912

#58 in reply to: ↑ 57 ; follow-up: @JohnPBloch
8 months ago

Replying to jorbin:

If the solution is going to include a composer file, related: #23912

I think that ticket is only tangentially related to this one. This ticket introduces a composer.json file as a development tool in service of building a release package, whereas that ticket is more about introducing composer.json in the built package so as to add support for using the built package in non-core contexts.

I'd love to see that other ticket move forward too, but I think each of these can move forward independently of one another.

@netweb
8 months ago

#59 @netweb
8 months ago

36335.4.diff updates 36335.3.diff to use https for w.org links, no other changes.

#60 @schlessera
8 months ago

Ah, nice, @wonderboymusic , you made the effort to create a patch with the code in my article. I never bothered because the feedback was not very conclusive, but if there's indeed some interest to really evaluate this approach, I'm all in!

I see you've already included all of the "easy-wins" in one patch already. It probably makes sense to have one big patch with all the easily converted classes in there, and then create additional issues for anything that needs more complex changes.

Also, within the classes you're autoloading, there are some functions to fetch a shared instance. You've moved these around, but I think the longer term plan should be to deal with these with something along the lines of #37699 (awesome work on that one as well, btw). Ideally, the instantiation, and the decision whether to share the same instance or create a new one for each request, should be centralized in some form and taken out of the inner layers.

#61 follow-ups: @jorbin
8 months ago

Some initial thoughts on the proposal:

1) Removing the instantiation of things like $GLOBALS['wp_press_this'] = new WP_Press_This(); from the original file is a breaking change.

2) I would like to see a travis's build matrix updated to include a 5.2 environment with spl disabled. Should likely have been done with the original addition of the shim, but

3) I apply the patch, and now development doesn't work right away. All I get is Autoloader was not found, aborting. That can't be committed.

4) I don't like the additon of the toplevel vendor folder. this should go inside wp-includes.

5) running composer install generates warnings.

Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd

6) I'm not sure yet how I feel about automatically making so much of wp-admin available on everypage load. In the past, core has forced developers to make a cognitive decision to include those pieces on the front end if they want to use them. Is that a change that should be made? why?

#62 in reply to: ↑ 61 @toscho
8 months ago

Replying to jorbin:

6) I'm not sure yet how I feel about automatically making so much of wp-admin available on everypage load. In the past, core has forced developers to make a cognitive decision to include those pieces on the front end if they want to use them. Is that a change that should be made? why?

Because it's very time consuming to check the core files and build fragile static dependencies on pathes each time you build something that overcomes the old backend/frontend dualism. These dependencies are also blocking core development with strong backwards compatibility constraints.

#63 in reply to: ↑ 61 @schlessera
8 months ago

Replying to jorbin:

1) Removing the instantiation of things like $GLOBALS['wp_press_this'] = new WP_Press_This(); from the original file is a breaking change.

That instantiation was executed at the point where the require was made. To avoid a breaking change, we just need to add the instantiation so it covers all of the scenarios that would have triggered the require.

3) I apply the patch, and now development doesn't work right away. All I get is Autoloader was not found, aborting. That can't be committed.

Trunk should include a composer install within its build process. There should be 1 single action that would trigger all of the required build steps. This should be either npm install ->triggering composer install or composer install ->triggering npm install. As this is mainly a PHP project, the latter would be preferable, but both would work just fine. For the normal WordPress distribution, autoloaders will of course be readily included.

4) I don't like the additon of the toplevel vendor folder. this should go inside wp-includes.

This can easily be changed. However, do note that there should be no hardcoded reference to the vendor folder either way, it is not supposed to be a location you can depend on.

5) running composer install generates warnings.

Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd

This is one example of bad class design in a third-party library. A quick search seems to indicate that the fix for this would be completely contained within that third-party library, by renaming the two classes, and changing line 906 in wp-admin/includes/class.ftp.php.

6) I'm not sure yet how I feel about automatically making so much of wp-admin available on everypage load. In the past, core has forced developers to make a cognitive decision to include those pieces on the front end if they want to use them. Is that a change that should be made? why?

Well, developers still need to make a cognitive decision to use those on the front-end. They just don't need to hard-code any path to get to them.

Also, it will take the burden off the developers who are less experienced at only including stuff that is used within the specific context. For most plugins that are built "sub-optimally", this will on average improve load times and memory consumption.

Last edited 8 months ago by schlessera (previous) (diff)

#64 in reply to: ↑ 61 ; follow-up: @wonderboymusic
8 months ago

Replying to jorbin:

Some initial thoughts on the proposal:

1) Removing the instantiation of things like $GLOBALS['wp_press_this'] = new WP_Press_This(); from the original file is a breaking change.

Press This was never loaded automatically, it was only loaded whenever we are specifically in a Press This enviro: press-this.php or one of the AJAX callbacks. I feel extremely comfortable making this change, and then waiting out the most bizarre of edge cases: someone who built something that loads WP_Press_This in isolation.

2) I would like to see a travis's build matrix updated to include a 5.2 environment with spl disabled. Should likely have been done with the original addition of the shim, but

Great idea. Thank you.

3) I apply the patch, and now development doesn't work right away. All I get is Autoloader was not found, aborting. That can't be committed.

If we are just using Composer for dev, should we check in the vendor files and pretend that the composer file doesn't exist? As a first step, I think we should.

4) I don't like the additon of the toplevel vendor folder. this should go inside wp-includes.

Why? To be forward-thinking, we should use the default name and it should encompass all classes in the codebase. Hence, be outside of the roots the autoloader searches.

5) running composer install generates warnings.

Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd
Warning: Ambiguous class resolution, "ftp" was found in both "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-sockets.php" and "/srv/www/blonderepublicansexkitten.com/public_html/src/wp-admin/includes/class-ftp-pure.php", the first will be used.
> xrstf\Composer52\Generator::onPostInstallCmd

I feel like I checked that library last year, and we may be using code that is no longer maintained ("I might be wrong" - Thom Yorke). If so, we should rename those classes. Yes, it is entirely possible that some person on this globe is directly using one of those files, not likely though.

6) I'm not sure yet how I feel about automatically making so much of wp-admin available on everypage load. In the past, core has forced developers to make a cognitive decision to include those pieces on the front end if they want to use them. Is that a change that should be made? why?

It's already available on every page load, the code just chooses not to include it. There is a bunch of image-related functionality which can happen on the frontend which has to include admin files. The way our files are organized is wanting, and I think the only reason some files from admin were desirable to be loaded on the frontend were to expose a class or some utility functions anyway.

#65 @wonderboymusic
8 months ago

36335.5.diff has the vendor folder (I understand that we may not want this to be checked in, let's see... )

I also added a list of files to be autoloaded in composer.json. This rips out the giant list of wp-includes/{box of functions grouped together}.php from wp-settings.php. I also moved the inclusion of the autoloader to wp-settings.php, so that it doesn't need to be included by any of the unit test files.

#66 @wonderboymusic
8 months ago

36335.6.diff has vendor and the composer.json, and is rebooted after my most recent class-splitting. Gonna look for low hanging fruit to parse out of this regardless of autoloading.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 months ago

#68 @wonderboymusic
8 months ago

In 38371:

Load: load class-phpass.php (PasswordHash class) early in wp-settings.php, instead of require_once()'ing it in several places.

See #36335.

#69 @wonderboymusic
8 months ago

In 38373:

Load: remove session.php and, instead, move the 2 require calls to wp-settings.php. This simplifies the include path.

See #36335.

#70 @wonderboymusic
8 months ago

In 38374:

Load: remove class-feed.php (There is no class named Feed or WP_Feed, it just loads other classes) and, instead, move the require calls to the only place they are ever included: inside fetch_feed(). This simplifies the include path.

Tested with this feed in a widget: http://rss.nytimes.com/services/xml/rss/nyt/HomePage.xml.

See #36335.

#71 @wonderboymusic
8 months ago

In 38380:

FTP: ensure that there is only one class named ftp, which is what is expected in the loading of this arcane library. This ensures that an autoload generator, something along the lines of Composer, won't hiccup when it gets to these files.

See #36335.

#72 @wonderboymusic
8 months ago

In 38384:

Bootstrap: add a composer.json file to the project. No code relies on it (yet), and no vendor dir is checked in (yet). No autoloader is being used (yet). Taking this first step will hopefully encourage the perfectionists of our world to scrutinize each line.

To fiddle around with what it does, run composer install from the project root. If you do not have Composer installed:
https://getcomposer.org/doc/00-intro.md#locally

tl;dr for Mac/Homebrew users: brew install composer

Classes from wp-includes and wp-admin are eligible for autoloading via autoload.classmap. Through a tornado of recent commits, many unsuitable files have been transitioned into a more acceptable state for autoloading: 1 file per class, no side effects.

The file bootstrap in wp-settings.php can transition into autoload.files. This will be done with care and attention.

See #36335.

#73 @wonderboymusic
8 months ago

In 38386:

Bootstrap: load option.php in wp-settings.php, not in functions.php.

See #36335.

@jorbin
8 months ago

#74 in reply to: ↑ 64 ; follow-up: @jorbin
8 months ago

Replying to wonderboymusic:

Replying to jorbin:

Some initial thoughts on the proposal:

1) Removing the instantiation of things like $GLOBALS['wp_press_this'] = new WP_Press_This(); from the original file is a breaking change.

Press This was never loaded automatically, it was only loaded whenever we are specifically in a Press This enviro: press-this.php or one of the AJAX callbacks. I feel extremely comfortable making this change, and then waiting out the most bizarre of edge cases: someone who built something that loads WP_Press_This in isolation.

This is an edgecase, but it is a breaking change. It needs to be documented on make/core as such.

4) I don't like the additon of the toplevel vendor folder. this should go inside wp-includes.

Why? To be forward-thinking, we should use the default name and it should encompass all classes in the codebase. Hence, be outside of the roots the autoloader searches.

With the number of people who install WordPress in the root of their checkout, I can easily see /vendor being a conflict. Putting /vendor inside /wp-includes makes it clear that this folder belongs to WordPress and also is less likely to run into conflicts. I've updated the patch accordingly.

5) running composer install generates warnings.

I feel like I checked that library last year, and we may be using code that is no longer maintained ("I might be wrong" - Thom Yorke). If so, we should rename those classes. Yes, it is entirely possible that some person on this globe is directly using one of those files, not likely though.

Still getting one warning related to the new class loader.

Warning: Ambiguous class resolution, "xrstf_Composer52_ClassLoader" was found in both "/srv/www/ilovewhite.space/public_html/src/wp-includes/vendor/composer/ClassLoader52.php" and "/srv/www/ilovewhite.space/public_html/src/wp-includes/vendor/xrstf/composer-php52/lib/xrstf/Composer52/ClassLoader.php", the first will be used.

Patch is updated so it applies correctly again. Let's make sure the final version soaks for at least 48 hours with the commit tag before this lands.

#75 follow-up: @jorbin
8 months ago

Some additional questions:

How do we ensure composer.json's autoload.files property stays up to date? Is this something that needs to updated manually? Should there be a grunt pre commit hook to help with this? Something else? Are there any regular composer file

Do we have anything to worry about if plugins are using composer or the 5.2 autoloader? Is there a string we can grep the plugin repo for so we can alert any plugin authors?

I added the composer.lock file in my patch. Was there a specific reason for not including it before?

Let's also do a review of the composer.json file to ensure it has only the things we want. I haven't done it yet, but found the schema for composer.json

Tasks that are still needed:

  • Update .travis.yml to include 5.2 with SPL disabled.
  • Draft a post for make/core with what developers need to do to test and how they can update their code.

#76 in reply to: ↑ 74 ; follow-up: @JohnPBloch
8 months ago

Replying to jorbin:

Still getting one warning related to the new class loader.

Warning: Ambiguous class resolution, "xrstf_Composer52_ClassLoader" was found in both "/srv/www/ilovewhite.space/public_html/src/wp-includes/vendor/composer/ClassLoader52.php" and "/srv/www/ilovewhite.space/public_html/src/wp-includes/vendor/xrstf/composer-php52/lib/xrstf/Composer52/ClassLoader.php", the first will be used.

That's being caused because the PHP 5.2-compatible autoloader library copies its own copy of ClassLoader.php into the composer-generated autoload directory. I believe this is related to the point @wonderboymusic made about keeping vendor outside of the directories that composer searches through for classes to be autoloaded in the classmap.

Evidently you can add a "exclude-from-classmap" entry to the autoload object in composer.json to exclude wp-includes/vendor from the classmap, so I think that would solve both concerns, yes? I'm still looking for documentation on that key, so I'll update if it doesn't quite work as I suspect.

#77 in reply to: ↑ 76 @JohnPBloch
8 months ago

Replying to JohnPBloch:

Evidently you can add a "exclude-from-classmap" entry to the autoload object in composer.json to exclude wp-includes/vendor from the classmap, so I think that would solve both concerns, yes? I'm still looking for documentation on that key, so I'll update if it doesn't quite work as I suspect.

Adding the following to the composer.json autoload block should fix that class notice and prevent composer from adding vendor classes to the classmap:

"autoload": {
  "exclude-from-classmap": [
    "wp-includes/vendor"
  ]
}

#78 @wonderboymusic
8 months ago

In 38391:

Bootstrap: exclude some files from the Composer autoload setup via exclude-from-classmap.

See #36335.

#79 in reply to: ↑ 75 @JohnPBloch
8 months ago

Replying to jorbin:

Do we have anything to worry about if plugins are using composer or the 5.2 autoloader?

I know for sure that Yoast SEO uses it. The only conflict that is likely to happen is that the plugin would have to use the xrstf_Composer52_ClassLoader class from core. The "real" autoloader class has its name generated using md5( uniqid() ) as a suffix, so there's an extremely low likelihood of a collision occurring between core and plugins. That being said, the core package could manually set the suffix by defining it in config.autoloader-suffix of composer.json. In short, I really don't think it's something to worry about. The xrstf_Composer52_ClassLoader class is loaded using an autoloader to begin with, so it won't get accidentally included twice (unless someone is REALLY doing composer the wrong way).

Is there a string we can grep the plugin repo for so we can alert any plugin authors?

class xrstf_Composer52_ClassLoader

I added the composer.lock file in my patch. Was there a specific reason for not including it before?

I can't speak for @wonderboymusic but I would suggest keeping the .lock tracked in version control so that contributors can be assumed to be using the correct version of any given dependency.

#80 follow-up: @wonderboymusic
8 months ago

Remaining steps:

1) to avoid collision, I say we name it wp-vendor and leave it outside wp-includes

2) Commit the composer.lock and wp-vendor files

3) Do a patch that rips out every require(_once)? for every class that is in the classmap, and add the inclusion of the Autoloader to wp-settings.php

4) Move the function files into autoload.files slowly, and remove their extraneous includes, running unit test after each file. We need to make sure that no file produces unexpected side effects. If so, we will move the side effects to wp-settings.php

Can everyone please agree on wp-vendor? If not, why not? Also, any tweaks for composer.json, let's do those as step #0.

The reason I am racing to get this in is to see what may or may not break. #early #yolo

WordPress is going on a massive diet.

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


8 months ago

#82 in reply to: ↑ 80 @GaryJ
8 months ago

Replying to wonderboymusic:

1) to avoid collision, I say we name it wp-vendor and leave it outside wp-includes

As I was reading the last few comments, wp-vendor at the top level is what sprung to mind for me too, so I'm +1 to this if top-level vendor is a no-go.

Any reason for selecting 1.* as the version for xrstf/composer-php52, instead of ^1.0? Composer lists the latter as being "the recommended operator for maximum interoperability when writing library code."

#83 @wonderboymusic
8 months ago

In 38393:

Bootstrap: run composer require xrstf/composer-php52 to get the updated suggested version. Change the vendor-dir to wp-vendor.

See #36335.

#84 @wonderboymusic
8 months ago

In 38394:

Bootstrap: add composer.lock and src/wp-vendor files. Nothing is using this code yet, just going through the motions. Ignore the files in src/wp-vendor/composer that will explode in PHP 5.2.

See #36335.

#85 @wonderboymusic
8 months ago

In 38395:

Bootstrap: exclude more files from the autoloader.classmap in composer.json.

See #36335.

#86 @ChriCo
8 months ago

Howdy and good morning.

I'm following this issue for a while now and..

what about moving *all* classes into wp-vendor and into sub-folders (packages) to group them by "topic" such as the Rest api?!

#87 follow-up: @wonderboymusic
8 months ago

In 38399:

Bootstrap: Autoload classes using a Composer-generated PHP 5.2-compatible Autoloader.

  • wp-admin and wp-includes are scanned for classes to autoload
  • Several 3rd-party and Ryan McCue-shaped libraries are excluded when the classmap is generated, see composer.json: autoload.exclude-from-classmap
  • wp-vendor/autoload_52.php is included at the top of wp-settings.php - no changes need to be made to unit tests to include the autoloader
  • An avalanche of require() and require_once() calls that loaded class files have been removed from the codebase.

The following files have been added to svn:ignore - they are not 5.2-compatible and fail during pre-commit:

  • src/wp-vendor/autoload.php
  • src/wp-vendor/composer/autoload_real.php
  • src/wp-vendor/composer/autoload_static.php
  • src/wp-vendor/composer/ClassLoader.php

We favor these files instead:

  • src/wp-vendor/autoload_52.php
  • src/wp-vendor/composer/autoload_real_52.php
  • src/wp-vendor/composer/ClassLoader52.php

When new PHP classes are added to the codebase, simply run composer install or composer update from the project root to update the autoloader.

The future is now.

See #36335.

#88 @swissspidy
8 months ago

In 38401:

After [38399], update the .gitignore file accordingly.

The following files have been added to svn:ignore already as they are not compatible with PHP 5.2:

  • src/wp-vendor/autoload.php
  • src/wp-vendor/composer/autoload_real.php
  • src/wp-vendor/composer/autoload_static.php
  • src/wp-vendor/composer/ClassLoader.php

See #36335.

#89 in reply to: ↑ 87 ; follow-ups: @dd32
8 months ago

Replying to wonderboymusic:

In 38399:
Bootstrap: Autoload classes using a Composer-generated PHP 5.2-compatible Autoloader.

FYI; That broke for anyone using core.svn as an external, as the built files still reference `/src/`. (Edit: Worth noting it was also trying to include /home/user/src/... instead of /home/user/public_html/src/... too, so doubly wrong.)

Do we need the wp-vendor directory there? I'm wondering if that wouldn't be better off as wp-includes/vendor or something, it'd be a little more "WordPressy".

Last edited 8 months ago by dd32 (previous) (diff)

#90 in reply to: ↑ 89 @swissspidy
8 months ago

Replying to dd32:

Replying to wonderboymusic:

In 38399:
Bootstrap: Autoload classes using a Composer-generated PHP 5.2-compatible Autoloader.

FYI; That broke for anyone using core.svn as an external, as the built files still reference `/src/`.

Do we need the wp-vendor directory there? I'm wondering if that wouldn't be better off as wp-includes/vendor or something, it'd be a little more "WordPressy".

Having the composer.json file inside src solves this. wp-includes/vendor would probably make ideas like #31744 easier.

#91 in reply to: ↑ 89 @jeherve
8 months ago

Replying to dd32:

FYI; That broke for anyone using core.svn as an external

Came here to report the same thing:

Warning: include(/home/user/src/wp-includes/wp-db.php): failed to open stream: No such file or directory in /home/user/mysite/wp-vendor/composer/ClassLoader52.php on line 186

Warning: include(): Failed opening '/home/user/src/wp-includes/wp-db.php' for inclusion (include_path='.:/usr/local/share/pear70') in /home/user/mysite/wp-vendor/composer/ClassLoader52.php on line 186

Fatal error: Uncaught Error: Class 'wpdb' not found in /home/user/mysite/wp-includes/load.php:404 Stack trace: #0 /home/user/mysite/wp-settings.php(103): require_wp_db() #1 /home/user/mysite/wp-config.php(149): require_once('/home/user/...') #2 /home/user/mysite/wp-load.php(39): require_once('/home/user/...') #3 /home/user/mysite/wp-blog-header.php(13): require_once('/home/user/...') #4 /home/user/mysite/index.php(17): require('/home/user/...') #5 {main} thrown in /home/user/mysite/wp-includes/load.php on line 404

I can't remember the last time trunk brought my site down. :)

#92 @jorbin
8 months ago

Can someone revert this until we can ensure core.svn.wordpress.org isn't broken please. I am AFK for the next 7-8 hours.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 months ago

#94 @dd32
8 months ago

In 38402:

Bootstrap: Revert [38399] as it's broken /build/ and subsequently core.svn.wordpress.org.

The generated classmaps reference /src/ files and operates in the assumption that the base directory is one level above wp-settings.php, which it isn't after our build processes are run.

See #36335

#95 in reply to: ↑ 58 @dnaber-de
8 months ago

Nice to see, that things move. Thanks for all your effort on this. However I'm wondering what is the main goal left on this ticket? The original intention was to have a single, dynamic autoloader instance provided by the core that can be used by plugins. That's obviously deprecated by the concept of using composer to build an autoloader (which I agree with, don't get me wrong).

In the end it should be our goal (not necessarily of this ticket) to be able to use WordPress as a composer package and leave the autoloader management of the complete app to composer to have one central autoloader.

The current composer.json defines the developer repository (git://develop.git.wordpress.org) as type wordpress-core. But popular installer configurations out there assumes that the build repository is of type wordpress-core (the structure of johnpbloch/wordpress). I don't think this as a tough problem, installer scripts can be adapted to the structure of this new and original wordpress-core package.

However, we should leave the composer.json in the root of the repository and should not move it to src/ as suggested by swissspidy:

Having the composer.json file inside src solves this. wp-includes/vendor would probably make ideas like #31744 easier.

Replying to JohnPBloch:

Replying to jorbin:

If the solution is going to include a composer file, related: #23912

I think that ticket is only tangentially related to this one. This ticket introduces a composer.json file as a development tool in service of building a release package, whereas that ticket is more about introducing composer.json in the built package so as to add support for using the built package in non-core contexts.

I'd love to see that other ticket move forward too, but I think each of these can move forward independently of one another.

In this case we should discuss both package name and type of both repositories. But as suggested above, the best way would be to use the developer-repository as composer package and adapt installers to this structure than providing two packages with two autoloader configurations and thus, two different build processes.

#96 @wonderboymusic
8 months ago

In 38409:

Bootstrap: move composer.{json|lock} into src.

See #36335.

#97 @wonderboymusic
8 months ago

In 38410:

Bootstrap: run composer install after r38409.

See #36335.

#98 @wonderboymusic
8 months ago

In 38411:

Bootstrap: after r38409 and r38410, revert r38402 which reverted r38399.

This fixes the paths in wp-vendor/ that were including src. I want to drop this in so we can find out what else will break.

See #36335.

@JohnPBloch
8 months ago

#99 @JohnPBloch
8 months ago

load-styles.php and load-scripts.php were both unable to find their classes due to lack of autoloader being loaded. 36335.15 fixes that.

#100 @azaozz
8 months ago

I think I'm still missing something important here.

Classes autoloading is very nice for OOP apps. However WordPress is not an OOP app. It's true, there are about 200 classes. Many of them are just containers for namespacing the functions inside. These are not meant to be extended or used in any OOP way.

There are also about 4750 "loose" functions. What does autoloading do for them?

As far as I see autoloading is maybe nice to have for the couple of hundred classes in WordPress. We can certainly do that when the minimum required PHP version allows it. However using a "side" functionality of a third party app to add this now seems premature. Also considering all the things that are breaking. I don't see the big benefits from doing this.

#101 @wonderboymusic
8 months ago

In 38412:

Script Loader: autoload in load-{scripts|styles}.php.

Props JohnPBloch.
See #36335.

#102 @azaozz
8 months ago

Another thing that needs clarification: if we have autoloading, would a plugin be able to stop some of the core classes from loading, noop or replace them etc. That would be a backwards compatibility disaster down the road :)

Last edited 8 months ago by azaozz (previous) (diff)

#103 @kraftbj
8 months ago

Since this landed, there seems to be an order issue where WP_Screen isn't being loaded until, at least, after the admin_init hook. I'm unfamiliar with the methods we're using to debug it immediately.

Prior, no notice thrown. After:

NOTICE: wp-includes/functions.php:4027 - convert_to_screen(), add_meta_box() was called incorrectly. Likely direct inclusion of wp-admin/includes/template.php in order to use add_meta_box(). This is very wrong. Hook the add_meta_box() call into the add_meta_boxes action instead. Please see Debugging in WordPress for more information. (This message was added in version 3.3.0.)
require_once('wp-admin/admin.php'), do_action('admin_init'), Akismet_Admin::admin_init, add_meta_box, convert_to_screen, _doing_it_wrong, trigger_error

When add_meta_box has the screen arg set as a string, convert_to_screen fires. convert_to_screen checks if ( ! class_exists( 'WP_Screen', false ) ) {, which then throws the _doing_it_wrong.

#104 @kraftbj
8 months ago

Including in the autoloader fixes my issue in the previous comment. See incoming patch.

I'm not familiar enough with autoloading to know if the other instances of using class_exists( XXX, false ) are problematic or not.

Last edited 8 months ago by kraftbj (previous) (diff)

@kraftbj
8 months ago

Fixes WP_Screen notice

#105 @wonderboymusic
8 months ago

In 38414:

Admin: allow WP_Screen to be checked via autoload in convert_to_screen().

Props kraftbj.
See #36335.

#106 @toscho
8 months ago

Is the class_exists() check still needed at all? I don't see why.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


8 months ago

#108 @swissspidy
8 months ago

http-tests.diff fixes the failing external HTTP tests.

#109 @swissspidy
8 months ago

In 38416:

Bootstrap: Fix failing external HTTP tests after [38411].

When testing the transports, the transport name needs to be capitalized for the class name to be correct (WP_Http_Curl vs. WP_Http_curl ).

See #36335.

This ticket was mentioned in Slack in #core by afragen. View the logs.


8 months ago

#112 follow-ups: @schlessera
8 months ago

Regarding the difficulties with the build-time move from src/* paths to final paths, there is an alternative solution:
We could use a custom autoloader generator that does not use relative paths, but rather paths based on a set constant, like ABSPATH. This would remove any assumptions about the build steps changing the folder hierarchy, as long as the surrounding code always properly defines this constant.

In reply to @azaozz :

Classes autoloading is very nice for OOP apps. However WordPress is not an OOP app. It's true, there are about 200 classes. Many of them are just containers for namespacing the functions inside. These are not meant to be extended or used in any OOP way.

Autoloading has several benefits out of the box, even with a more procedural codebase like WordPress:

  • Instead of loading everything that might be used within a given code path (which often ends up being almost the entire code base), you effectively only load and process the exact amount of code you need, when you need it. This will most probably reduce (only benchmarks can tell for sure) memory consumption and processor work, and result in faster load times. I expect that especially AJAX calls (which often only do a small task and immediately die) to see drastic improvements, once the move to autoloading is completed.
  • The code does not need to know the path to classes anymore. This removes a lot of hard-coded strings and assumptions, and makes moving and reorganizing the code a breeze. After work on the autoloader is completed, moving files around would not be a breaking change anymore, as only the autoloader generator should need to know about their location in the first place.
  • Usage of an autoloader makes a lot of "timing" related bugs obvious, where code was only working "by chance" because the exact execution path of a section of code was necessary to have the correct files loaded when they were used.
  • An autoloader makes the inclusion of third-party libraries (of which there are a number already included within WP Core) much easier, if each of the libraries just knows how to include its own autoloadable assets. This way, your code does not need to know anything about the actual file layout of a third-party library, it just needs to know what initial class/function to use.
  • The order of loading becomes irrelevant, as everything is loaded at the point where it is required.

Not all of these benefits can be completely delivered with this first iteration, because the code base is not yet fully adapted to make the best use of an autoloader. But already having an autoloader is a first necessary step, and it makes a lot of other changes to the code much easier.

There are also about 4750 "loose" functions. What does autoloading do for them?

Not much, indeed. Their files can be added to the Composer autoloader, but this will just immediately include then when the autoloader is loaded.

However, this does not in any way reduce the other benefits that the autoloader provides. The fact that one half of the code base will not become faster and lighter does not make it useless to have the other half become faster and lighter. And I think it is obvious that there's a clear trend towards OOP with the more recent code additions, so the benefits will only grow with time.

Another thing that needs clarification: if we have autoloading, would a plugin be able to stop some of the core classes from loading, noop or replace them etc. That would be a backwards compatibility disaster down the road :)

Yes, a plugin could potentially do this if it would require a class with the same name before it was loaded through the autoloader. However, I can't see why a plugin would want to do that, and it would probably break half of the time, because autoloading means there's no set point in time where you know the class was or wasn't loaded any more. Also, I think there's a number of classes and functions that contain a class_exists() check before being loaded, so these could just as easily be replaced by a plugin version.

#113 in reply to: ↑ 112 ; follow-ups: @Clorith
8 months ago

Replying to schlessera:

Another thing that needs clarification: if we have autoloading, would a plugin be able to stop some of the core classes from loading, noop or replace them etc. That would be a backwards compatibility disaster down the road :)

Yes, a plugin could potentially do this if it would require a class with the same name before it was loaded through the autoloader. However, I can't see why a plugin would want to do that, and it would probably break half of the time, because autoloading means there's no set point in time where you know the class was or wasn't loaded any more. Also, I think there's a number of classes and functions that contain a class_exists() check before being loaded, so these could just as easily be replaced by a plugin version.

This is the part that worries me, you say they shouldn't, I agree. But from experience, they will, they most definitely will.

I can guarantee that this will lead to an increase in broken setups, plugins and themes will re-declare classes, they won't use class_exists (because why should they, core will only make this a problem if core is loaded before them, there's not much consideration for other plugins and themes in that regard), they will have class name collisions with other plugins that are trying to be just as clever.

Just look at 4.6 which had this with a single generically named function for the_post_thumbnail_caption which themes and plugins were already using, already declared function and classes means server error, this can't be fixed without accessing the file system in some way, which is a technical step many of our users are not comfortable making (and we shouldn't force upon them if we can avoid it).

I was thinking of ways we could possibly sign core classes so we don't auto load plugin/theme ones as replacements, but that won't work if they just do an include or require right off the bat, which is the more likely scenario than them piggybacking off a core auto loader.

#114 in reply to: ↑ 113 @toscho
8 months ago

Replying to Clorith:

I was thinking of ways we could possibly sign core classes so we don't auto load plugin/theme ones as replacements […]

That way exists already: namespaces, available since PHP 5.3.

But I agree with @schlessera: Whoever tries to replace core classes will run into many problems immediately, and their code should break. This is not a problem the core has to resolve.

#115 @schlessera
8 months ago

I feel there are differences in how people perceive "error conditions" and their use/handling in software development.

The way I see it, there are two general types of error conditions:

  1. Error conditions that are due to something in the required context of your piece of code not being in the state you were expecting. This is stuff like the DB not being accessible, a query that does not produce results, etc... These types of error conditions need to be gracefully handled, as they occur at runtime for otherwise perfectly valid code. A 100% bug-free (yeah, right) plugin can still be hit by a DB connection issue.
  1. Error conditions that are due to code that is logically broken, in whatever way. This is stuff like a plugin using the wrong function, omitting required arguments, overriding what is not to be overridden, etc... These types of errors need to break as soon and as hard as possible. You don't want to continue "somewhat running correctly" with broken code, only to have it completely failed a month later on the production server. The more critical your code base fails with such errors, the better.

So, under these assumptions, someone overriding a WordPress Core class, and doing stuff that is not meant to be done, is creating error conditions of the second type. Ideally, this would reliably throw a hundred error messages at them. This is currently not the case, either with or without autoloader. This does not mean however that we should try to "avoid" throwing errors in such a case.

#116 in reply to: ↑ 113 @flixos90
8 months ago

Replying to Clorith:

This is the part that worries me, you say they shouldn't, I agree. But from experience, they will, they most definitely will.

I agree with @schlessera - I don't think it's a good idea to base code decisions in Core on how developers might misuse it. There's always ways that someone can mess up WordPress by doing things in a way that are not intended to be handled that way. Anything like that falls under the second type of error condition and should fatally fail if possible (so that they find out it's wrong during development) or be forbidden by guidelines like "Writing a Plugin" (where it is already stated that one should use a custom prefix for custom stuff).

Just look at 4.6 which had this with a single generically named function for the_post_thumbnail_caption which themes and plugins were already using, already declared function and classes means server error, this can't be fixed without accessing the file system in some way, which is a technical step many of our users are not comfortable making (and we shouldn't force upon them if we can avoid it).

This is an example of exactly the above. A plugin or theme that declares such a function (especially without a function_exists check) is most certainly badly developed to some degree. These plugins and themes will always exist, and us not making it easier for them to hijack stuff will not make them go away.

Last edited 8 months ago by flixos90 (previous) (diff)

#117 follow-up: @Clorith
8 months ago

I fear I might've not used my words right, and my intent wasn't very clear, so let me try again :)

My worry is that per today's approach, core loads certain things up, and they are there; with auto loading, they're never really loaded until they are needed (if not already loaded), so if a plugin loads up the same class name, we won't load ours. There's currently a list of some 70-80 files from wp-settings.php which are not pluggable in any way, but with this, those would also be re-enabled (unless I'm missing something here, and I would be happy to be told so :) )

This increases the probability of breakage, and we are enabling them to do so, we are "making core pluggable" (which is a terminology I like for this) by allowing replacement of previously non-replaceable classes, and it means even something simple like a bugfix to a class in a point release, which are automatically pushed, could result in unexpected behavior one way or another.

You're right that we shouldn't let other plugin or theme developers affect how we implement things, but we DO NEED to take user expectations into consideration, and users won't know what's going on when one plugin declares a core class which another plugin relies on later down the line and gets a whole different behavior because the first one to load their own isn't a carbon copy of core, or they both try to declare that same class because core is pluggable now because we never load up core versions of classes if they are already declared.
From a users point of view, their site is now broken, and they may not be tech savvy enough to fix it (because a fix to a fatal error requires hands on modifications one way or another), and this would be on us, we enabled them to break it this badly and if we can let them break it, we should at least make an effort to help prevent it.

#118 @dnaber-de
8 months ago

Well, everywhere you filter an object instance, plugins are able to compromise the type (and some doing so by now). So this problem already exists. Bad code can pretty much break anything in some way, that should not influence code development.

Last edited 8 months ago by dnaber-de (previous) (diff)

#119 follow-up: @charliespider
8 months ago

Just a heads up... we're getting the following error popping up when running our unit tests on https://circleci.com:

PHP Fatal error:  Class 'WP_HTTP_Cookie' not found

which pulls in master branch from git://develop.git.wordpress.org/

Haven't nailed it down completely yet, but thinking it's a result of the changes in this ticket.

It appears that class-wp-http-cookie.php is not getting loaded despite not being listed in composer.json: autoload.exclude-from-classmap, but I haven't read through everything here to see if we are expected to do something different when referencing WP core classes.

#120 in reply to: ↑ 119 @schlessera
8 months ago

Replying to charliespider:

Just a heads up... we're getting the following error popping up when running our unit tests on https://circleci.com:

PHP Fatal error:  Class 'WP_HTTP_Cookie' not found

which pulls in master branch from git://develop.git.wordpress.org/

Haven't nailed it down completely yet, but thinking it's a result of the changes in this ticket.

It appears that class-wp-http-cookie.php is not getting loaded despite not being listed in composer.json: autoload.exclude-from-classmap, but I haven't read through everything here to see if we are expected to do something different when referencing WP core classes.

The class is actually called WP_Http_Cookie (see case).

#121 in reply to: ↑ 117 @schlessera
8 months ago

Replying to Clorith:

This increases the probability of breakage, and we are enabling them to do so, we are "making core pluggable" (which is a terminology I like for this) by allowing replacement of previously non-replaceable classes, and it means even something simple like a bugfix to a class in a point release, which are automatically pushed, could result in unexpected behavior one way or another.

What you're describing is certainly something worth discussing. I'm not happy with that terminology, however, because it relates this to the already "pluggable" elements in WordPress Core like db.php and advanced-cache.php. The point about these is that they are meant to be replaced.

Nobody is meant to replace core WordPress classes. And if they do, they would have done it with or without an autoloader. The addition of an autoloader does not in any way change the fact that plugins can already replaces whatever they want on most systems, and cause havoc. Correct me if I'm wrong, but I think that on most systems (those which belong to "end-users"), a plugin can replace any filesystem file within web root if it wants. So, with or without an autoloader, there are no guarantees, and you can't keep malicious/un-smart people from doing malicious/un-smart things.

Does that mean that we should discuss how best to proceed so that some precautions can be taken? Yes, absolutely! But does that mean that we should avoid a definite code improvement because it could be "mis-used"? I don't think so. I'd rather, we make it easier and better for the proper developers to do their jobs and for WordPress Core to more easily adapt to changing requirements.

You're right that we shouldn't let other plugin or theme developers affect how we implement things, but we DO NEED to take user expectations into consideration, and users won't know what's going on when one plugin declares a core class which another plugin relies on later down the line and gets a whole different behavior because the first one to load their own isn't a carbon copy of core, or they both try to declare that same class because core is pluggable now because we never load up core versions of classes if they are already declared.

From a users point of view, their site is now broken, and they may not be tech savvy enough to fix it (because a fix to a fatal error requires hands on modifications one way or another), and this would be on us, we enabled them to break it this badly and if we can let them break it, we should at least make an effort to help prevent it.

Well, although I don't see why this would be an "autoloader issue" (as bad plugins kill users' sites, even today), I understand why we might need to worry that errors happen at a different level and prevent plugin deactivation (although I'm sure almost any "end-users" have had to deal with a WSOD before). So, apart from improving the WordPress error handler in general, we might also improve the standard class loader that gets registered to provide some safeguards. These safeguards would have a minor impact on performance, though, so ideally, we could also provide a switch for the pros to just disable these safeguards. If end-users are a concern, a proper error handler would not kill the site, but allow admin logins in safe-mode and ask the user whether to deactivate the offending plugin. But that's definitely a topic for a different ticket.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 months ago

#123 @jorbin
8 months ago

I just ran some siege tests of single page loads using twentyfifteen and they show no real speed difference between r38398 ( before the autoloader) and r38433 (now) ( 4.86 to 4.89 ). If speed is going to be used as an argument towards this, we need something that demonstrates it.

#124 in reply to: ↑ 112 ; follow-up: @azaozz
8 months ago

I can't see why a plugin would want to do that, and it would probably break half of the time, because autoloading means there's no set point in time where you know the class was or wasn't loaded any more.

Unfortunately this is not entirely true. The plugins are loaded relatively early. They will always be loaded before a bunch of classes are first needed effectively letting the plugins always "preload" some classes. I agree with @Clorith: "...you say they shouldn't, I agree. But from experience, they will, they most definitely will."

Having "pluggable" classes in WordPress is out of the question.

@schlessera, @flixos90, @flixos90 not sure how well you know the WordPress codebase but there is a thing called "pluggable.php". As far as I know this is a left-over from forking b2 14 years ago that wasn't fixed/removed in time. This is probably the worst architectural decision ever. Why would you want to extend it? :)

Autoloading has several benefits out of the box, even with a more procedural codebase like WordPress

I agree. Autoloading is great for classes. After a quick look through the code, about 80-100 of the 200 classes will always be loaded, and half of the rest would never be loaded together. So currently autoloading will affect maybe about 5% of the code. This is still good, that percent will increase in the future.

Nobody is meant to replace core WordPress classes. And if they do, they would have done it with or without an autoloader. The addition of an autoloader does not in any way change the fact that plugins can already replaces whatever they want on most systems, and cause havoc.

Not true. Plugins cannot replace any of the required classes.

Does that mean that we should discuss how best to proceed so that some precautions can be taken? Yes, absolutely! But does that mean that we should avoid a definite code improvement because it could be "mis-used"? I don't think so.

Totally agree. As far as I see the way to proceed here is:

  • Make an experimental branch where all this can be tested and cleaned up before merging in core.
  • Find a way to "lock down" core classes so they cannot be replaced by plugins. Think of this like having the final keyword in class definitions. One possible way would be to have a custom autoloader that has a list of the core classes and doesn't do class_exists() before loading any of them (and keeps some sort of state to test if they are already loaded).

#125 follow-up: @afragen
8 months ago

PHP 5.3 and namespacing sure seems like it would negate many of these potential issues.

Is it time?

Last edited 8 months ago by afragen (previous) (diff)

#126 in reply to: ↑ 124 @toscho
8 months ago

Replying to azaozz:

Having "pluggable" classes in WordPress is out of the question.

Many class instances are stored in a global variable, and these can be replaced already. So "pluggable classes" are already here, as "pluggable instances". WP_Scripts is one example.

#127 in reply to: ↑ 125 @jorbin
8 months ago

Replying to afragen:

PHP 5.3 and namespacing sure seems like it would negate many of these potential issues.

Is it time?

How? Even with namespaces, a plugin could still include it's own version with whatever namespace we end up using. Example.

#128 @afragen
8 months ago

PHP 5.3 and namespacing sure seems like it would negate many of these potential issues.
Is it time?

How? Even with namespaces, a plugin could still include it's own version with whatever namespace we end up using. Example.

@jorbin but the unaware dev would now have to duplicate both the namespace and the class name. A simple class name collision would no longer occur.

There is always be the possibility of someone doing it wrong. I don't believe we can stop all instances, especially if the dev's intent is to override the class.

#129 @iamfriendly
8 months ago

I'm not sure it's about catching/preventing all of the instances of someone doing it wrong. It's about setting a precedent and, possibly more importantly, setting expectations.

Ultimately, this has a potential to break a lot of things, very quickly and very badly. (Please note I am very pro this change, I'm just urging an abundance of caution). One mis-step can lead to developers going down a winding path without us really realising it. This can end badly for the user. (See the shortcodes API changes last year, which admittedly was a necessary change to a feature and this is more architectural, but the point stands).

There are currently many ways to override core pieces (in a doing_it_wrong fashion) but that doesn't mean we should simply ignore that and give developers another chance to shoot themselves and their users in the foot.

I'm +1 for the addition of a branch which would allow us to really hammer this and see what breaks in a lot of scenarios. It (probably) means a slower inclusion into core, but it sure as heck gives us a real chance to see what's happening and also can give developers a chance to test and test again. It also gives us a way to compare and contrast items like speed and memory usage.

I truly believe that, when merged, this will be one of the biggest, most positive changes to WP in a long time. I'm so glad we're having this conversation and hashing out all of the ways we can get this done, to get it right.

#130 follow-up: @schlessera
8 months ago

Replying to jorbin:

If speed is going to be used as an argument towards this, we need something that demonstrates it.

It is too early to run any benchmarks. Conversion has barely started, which will only lay the groundwork. Right now, there's a large chance that the loading with the autoloader will include the exact same set of classes as before. Once the conversion to an autoloader has been made, there's lots of optimization opportunities. Only after doing these will we see an improvement. Also, some scenarios like AJAX calls will offer more optmization potential then others.

Replying to azaozz:

Having "pluggable" classes in WordPress is out of the question.

As most objects are passed around through globals, you already have "pluggable" objects either way.

@schlessera, @flixos90, @flixos90 not sure how well you know the WordPress codebase but there is a thing called "pluggable.php". As far as I know this is a left-over from forking b2 14 years ago that wasn't fixed/removed in time. This is probably the worst architectural decision ever. Why would you want to extend it? :)

Yes, I know that "thing" and also commented on it. There's a fundamental difference here. The original "pluggable.php" was a mechanism where developers were meant to replace core parts, and it was badly designed to fulfil that purpose. Here, we developers are not meant to replace classes, and the possibility to do so nevertheless is just part of the language. You can't make the fact go away that bad coders will do bad things. If they intend to replace a class, they will do it, with or without autoloader.

I agree. Autoloading is great for classes. After a quick look through the code, about 80-100 of the 200 classes will always be loaded, and half of the rest would never be loaded together. So currently autoloading will affect maybe about 5% of the code. This is still good, that percent will increase in the future.

Yes, this is the state of the current code. As there has not been an autoloader so far, the code is not optimized to make effective use of an autoloader. This is something that can be easily changed, though.

Not true. Plugins cannot replace any of the required classes.

They can replace pretty much any object that is stored in a global variable. They can overwrite Core files (on most systems). They can replace a lot of core functionality through hooks. And I'm sure they'll find even more creative ways to break stuff I wouldn't even think about. I agree that this is a problem that needs to be dealt with, but I don't see this as an argument for or against an autoloader. The issue of replacing core parts would ideally be dealt with by accepting that this is unavoidable, providing interfaces, and thus having contracts in place, which still allow plugins to replace parts, but define what behavior they need to adhere to to not break stuff.

Totally agree. As far as I see the way to proceed here is:

  • Make an experimental branch where all this can be tested and cleaned up before merging in core.
  • Find a way to "lock down" core classes so they cannot be replaced by plugins. Think of this like having the final keyword in class definitions. One possible way would be to have a custom autoloader that has a list of the core classes and doesn't do class_exists() before loading any of them (and keeps some sort of state to test if they are already loaded).

That's not how an autoloader works. The autoloader does not do class_exists() because it will only get invoked if it doesn't yet. If the class has already been defined, the autoloader will not even be triggered. If you want to lock down core classes, you need to let the code using them make a check to make sure they got the right one. You can only check the usage, not the creation. But I don't recommend this, it will only cause lots of trouble and horrible code, while still not being bullet-proof.

Replying to jorbin:

Replying to afragen:

PHP 5.3 and namespacing sure seems like it would negate many of these potential issues.

How? Even with namespaces, a plugin could still include it's own version with whatever namespace we end up using.

Yes, namespaces would not prevent explicit overriding of anything. But they would prevent accidental collisions, and that's what we should worry about.

Replying to iamfriendly:

I'm not sure it's about catching/preventing all of the instances of someone doing it wrong. It's about setting a precedent and, possibly more importantly, setting expectations.

You set expectations through documentation, doc blocks, typehints and asserts, not by avoiding language constructs.

Ultimately, this has a potential to break a lot of things, very quickly and very badly. (Please note I am very pro this change, I'm just urging an abundance of caution). One mis-step can lead to developers going down a winding path without us really realising it. This can end badly for the user. (See the shortcodes API changes last year, which admittedly was a necessary change to a feature and this is more architectural, but the point stands).

The shortcodes API change broke the actual user content in the databases of the users. I don't think you can compare a code change that will have an incidence on future plugins with a change that breaks ten year old existing content. But I agree that cautionary tales are always there for a reason...

There are currently many ways to override core pieces (in a doing_it_wrong fashion) but that doesn't mean we should simply ignore that and give developers another chance to shoot themselves and their users in the foot.

How about giving them another chance to produce better code? Those who intend to produce quality code will be grateful for an autoloader.

I'm +1 for the addition of a branch which would allow us to really hammer this and see what breaks in a lot of scenarios. It (probably) means a slower inclusion into core, but it sure as heck gives us a real chance to see what's happening and also can give developers a chance to test and test again. It also gives us a way to compare and contrast items like speed and memory usage.

I don't know enough about workflow/deployment internals to comment on this. But I sure was shocked to read that trunk is deployed as is on .org, so a branch is probably the safer bet.

I truly believe that, when merged, this will be one of the biggest, most positive changes to WP in a long time. I'm so glad we're having this conversation and hashing out all of the ways we can get this done, to get it right.

Yes, once the autoloader is in and we start optimizing the bootstrapping process to make use of it, WordPress will make huge strides.

#131 @schlessera
8 months ago

As already mentioned in Slack, I have created a custom WordPress autoloader generator and class loader that can be adapted to meet very specific WordPress needs: https://github.com/schlessera/composer-wp-autoload

The code is not brilliant, as I am a bit pressed for time right now, and I have re-used large parts of the xrstf/composer52 package. I intend to further clean it up in the coming days, though.

Main features that differ from standard Composer autoloader (taken from README.md file):

  • The generated autoloader is compatible with PHP 5.2. Classes containing PHP 5.3+ code will be skipped and throw warnings.
  • The paths to the classes are relative to a set constant. The default constant being used is ABSPATH.
  • The class maps can optionally be configured to be case-insensitive.

Regarding the case-insensitivity, I would recommend against using that, as the case-sensitivity just points to potential latent bugs. However, I provide the functionality as an optional setting nevertheless, as I wouldn't want the inclusion of an autoloader to be blocked because of minor case mismatches.

@schlessera
8 months ago

Using custom WordPress-specific autoloader plugin.

#132 @schlessera
8 months ago

I added a diff that shows how the custom autoloader plugin is used.

Some notes:

  • When case-sensitive is set to false, both generated class maps and the class loader will run the class name through mb_strtolower. This could be changed to use the faster strtolower, in case MB support is not needed.
  • The ABSPATH constant needs to be set before the autoloader is included, otherwise the class map is broken.
  • This is not a final implementation, but rather a quick first version to show that Composer is flexible enough to adapt to any requirements. The code can still be optimized, and the project can also be properly integrated into WP codebase (runs off my personal GitHub account right now).

This ticket was mentioned in Slack in #core by schlessera. View the logs.


8 months ago

#134 @wonderboymusic
8 months ago

In 38469:

General: revert [38386], functions.php was probably too tempting for some people to not load by itself.

See #36335.

#135 @wonderboymusic
8 months ago

In 38470:

Bootstrap: do not go gentle into that good night r38411, r38412, and parts of r38389.

See #36335.

#137 in reply to: ↑ 130 ; follow-ups: @azaozz
8 months ago

Replying to schlessera:

The shortcodes API change broke the actual user content in the databases of the users.

No. The change broke the way some plugins use shortcodes. Shortcodes are placeholders. Nothing more. That breakage happened because these plugins misused the core functionality and used the shortcode API in an unintended and unsupported way, even introducing security problems in the process. And that happened because the API was (and still is) too permissive.

The original "pluggable.php" was a mechanism where developers were meant to replace core parts, and it was badly designed to fulfil that purpose. Here, we developers are not meant to replace classes, and the possibility to do so nevertheless is just part of the language. You can't make the fact go away that bad coders will do bad things. If they intend to replace a class, they will do it, with or without autoloader.

Right. In "pluggable.php" this is the intended behaviour, and the design is bad. However because of the way WordPress works, adding standard autoloading of classes introduces exactly the same design flaws like in pluggable.php. Of course we can add all the documentation and warnings we want, but do you believe all developers will always follow them? After the example with the shortcodes the answer tends to be firm "No".

I'm not sure how to make this easier to understand :) This is somewhat similar to using private in a class. Is there even one OOP app out there that instead of using private adds some documentation like "Please don't touch this from the outside. Thanks.". Why is there a private keyword anyway? :)

Not true. Plugins cannot replace any of the required classes.

They can replace pretty much any object that is stored in a global variable. They can overwrite Core files (on most systems). They can replace a lot of core functionality through hooks.

Not exactly. The parts that are replaceable through hooks are sufficiently "insulated" and we go to great lengths to maintain backwards compatibility for them. Replacing globals doesn't work in many cases as most classes are instantiated right before using them. That makes it quite inconsistent even for badly written plugins.

At the same time I'm sure there are plugins that are trying to do that even now. This is very limited simply because it doesn't work well. However if we let such plugins replace core classes before they are loaded, they will.

That's not how an autoloader works. The autoloader does not do class_exists() because it will only get invoked if it doesn't yet. If the class has already been defined, the autoloader will not even be triggered.

What I meant is that one possible way of making this work would be to rewrite the logic that checks whether a class is already loaded, and add a whitelist of the core classes. If a core class is not loaded from the respective location/file, it would throw an exception and exit. This was just a quick suggestion. Hopefully there is a better way that would let us use the native PHP functionality.

As there has not been an autoloader so far, the code is not optimized to make effective use of an autoloader. This is something that can be easily changed, though.

No, no matter what we do many classes will always be loaded as they are needed by the rest of the procedural code. Most of the remaining classes are never needed together. They will never be loaded at the same time with or without an autoloader. So currently any gains from autoloading in WordPress are minuscule. This doesn't mean we shouldn't add it, but we shouldn't set any expectations that it would make WordPress lighter or faster.

Last edited 8 months ago by azaozz (previous) (diff)

#138 follow-up: @johnjamesjacoby
8 months ago

In "pluggable.php" this is the intended behaviour, and the design is bad.

The design in pluggable.php is /bad/ because it only allows 1 opportunity to intercept, with no additional way to verify or override the decisions made before you.

The audience of developers actually needing to dig this deep is - admittedly - infinitesimal; but to them, the design of pluggable.php is a critical aspect of their WordPress powered application; they couldn't do what they need to do without a hundred other bespoke actions & filters.

What's a bigger maintenance burden: hundreds of tiny decisions, or 1 big one? I think it's maybe a-horse-a-piece.

FWIW, huge swaths of both BuddyPress & bbPress can internally be gutted & swapped out, but (anecdotally) I've yet to see breakage or complaints (or even really communicate the need for this functionality above the high-availability environments I've been involved in that have directly required it.)

// For example...
define( 'BP_PLUGIN_DIR', 'anywhere/you/want' );         // BuddyPress
add_filter( 'bbp_includes_dir', 'anywhere/you/want/' ); // bbPress

Will this cause advanced developers to think outside of the box? Absolutely. Hopefully. The alternative is standing in the way of creativity, and enforcing artificial constraints into a system that is largely defined by it's flexibility already.

#139 in reply to: ↑ 138 ; follow-up: @azaozz
8 months ago

Replying to johnjamesjacoby:

The design in pluggable.php is /bad/ because it only allows 1 opportunity to intercept, with no additional way to verify or override the decisions made before you.

I don't see any difference with the current autoloading. The first plugin to require a class wins, any other plugins will not be able to replace it any more, and will trigger fatal errors.

This ticket was mentioned in Slack in #core by helen. View the logs.


8 months ago

#141 in reply to: ↑ 137 @schlessera
8 months ago

Replying to azaozz:

The shortcodes API change broke the actual user content in the databases of the users.

No. The change broke the way some plugins use shortcodes. Shortcodes are placeholders. Nothing more. That breakage happened because these plugins misused the core functionality and used the shortcode API in an unintended and unsupported way, even introducing security problems in the process.

I agree about the technical nuances. But the change was more user-facing than the autoloader one.

And that happened because the API was (and still is) too permissive.

No, it happened because there just was no appropriate API for what both the plugin authors and users needed. Making stuff less permissive just pushes developers into "illegal" behavior.

I'm not sure how to make this easier to understand :) This is somewhat similar to using private in a class. Is there even one OOP app out there that instead of using private adds some documentation like "Please don't touch this from the outside. Thanks.". Why is there a private keyword anyway? :)

Well, most of the time, they differentiate between private and protected to communicate intent. Sure, you can break the hell out of a protected property. But it's worse if you need to use hacks if there is no protected property you can reuse.

That's not how an autoloader works. The autoloader does not do class_exists() because it will only get invoked if it doesn't yet. If the class has already been defined, the autoloader will not even be triggered.

What I meant is that one possible way of making this work would be to rewrite the logic that checks whether a class is already loaded, and add a whitelist of the core classes.

Yes, but as I said, that is not how autoloading works. This "logic" is actual the PHP language itself calling the autoloader callback _after_ it noticed that a class is missing.

#142 in reply to: ↑ 139 @schlessera
8 months ago

Replying to azaozz:

The design in pluggable.php is /bad/ because it only allows 1 opportunity to intercept, with no additional way to verify or override the decisions made before you.

I don't see any difference with the current autoloading. The first plugin to require a class wins, any other plugins will not be able to replace it any more, and will trigger fatal errors.

The difference is not in the result of doing it wrong. The difference is in the fact that "replacing autoloaded core classes" is not a mechanism provided to developers.

Replacement mechanism => plugin breaks core by replacing => replacement mechanism design at fault
No replacement mechanism => plugin breaks core by replacing => plugin at fault

#143 @wonderboymusic
8 months ago

In 38480:

Bootstrap: goodnight sweet prince.

See #36335.

#144 in reply to: ↑ 14 ; follow-ups: @MikeSchinkel
8 months ago

Time for a contrary opinion. I think there has been too much group think on this ticket (except for @azaozz).

Composer

I assume we are discussing how the average WordPress would work with WordPress. For that I am currently strongly against using Composer for WordPress core. First, it's autoloader.

For our own project (WPLib) we added our own autoloader in front of Composer's autoloader because Composer's autoloader often runs through a painful amount of code before it actually identifies the file to load. This is relevant not just for performance but because it makes debugging with XDEBUG a major PITA. Our autoloader is two lines long using a classmap, and it is far less painful to use when XDEBUGging.

Second is Composer's complexity

I think we should reinvent the wheel here. I have been working the past year on WPLib Box (Vagrant local dev server) with a goal of creating a standized platform for doing WP development -- at least for users of our box -- I had embraced Composer.

However, recently I have come to the conclusion that while Composer is brilliant for PHP frameworks it is a nightmare to work with for WordPress because it is opinionated in many ways and those opinions different in most things that WordPress needs to do.

So I am now planning to design a WordPress-specific format for WPLib Box with a goal to greatly simplify what you need to specify to get a WP site to work. We might even generate a Composer file and use it behind the scenes, merging in any Composer files that work well with Composer, e.g. those things found at packagist.org.

That said, I would love for the WordPress community to do this work with me so it it can be improved by me not working on it in a vacuum.

In summary related to Composer is that it is not well suited for WordPress website.

And I would hate to see Composer become the "standard" for WordPress.

Too Much Code Running During Autoloading

My preference would be that WordPress core would leverage a highly-optimized class-map autoloader (and not have the classmap be "temporary") used both for performance and for minimizing amount of code that one has to step through during debugging with XDEBUG, and encourage plugins and themes to use the class-map too.

Ideally the class-map autoloader would be optimized for the number of lines that need to run for a file to be autoloaded, and that the class-map loader would be run first in front of the other optional autoloaders. And rather than separating concerns for the class-map loader, optimize it instead.

class-foo-bar.php vs. WP_Foo_Bar.php

I started our WPLib autoloader following that pattern, but am currently working on a version that will move to using classnames for filenames because it solves the problem of to find the filename for a class in a classmap. This is very helpful when writing code that needs to be able to determine what plugin a class is in, or if it in the theme, for example.

Class-Map Generation.

BTW, I've found a really nice way to build a class-map even with the other autoloaders. Have each autoloader add any classes found to the classmap and then when WP_DEBUG is active write out to a file on a 'shutdown' hook which can then be loaded in wp-settings.php. Which it works well for WPLib there are issues with implementing it in core, but it is something to explore.

Last edited 8 months ago by MikeSchinkel (previous) (diff)

#145 in reply to: ↑ 144 @schlessera
8 months ago

Replying to MikeSchinkel:

Time for a contrary opinion. I think there has been too much group think on this ticket (except for @azaozz).

@azaozz raises very valid concerns, and I think we should try our best to address each one of them and find a solution that works for all concerned.

However, what you call "groupthink" (which I find rude, tbh), is an effort to bring de facto PHP best practices to WordPress, trying to get it out of the "legacy software" drawer it keeps getting put into more and more. I think that all the people contributing on Trac are interested in seeing a bright future for WordPress, so it hurts to have it be too tightly stuck in the past.

This is relevant not just for performance but because it makes debugging with XDEBUG a major PITA. Our autoloader is two lines long using a classmap, and it is far less painful to use when XDEBUGging.

If you use a tried and tested autoloader, there's no need to step through it with XDEBUG, so I don't see why this would be relevant.

However, recently I have come to the conclusion that while Composer is brilliant for PHP frameworks it is a nightmare to work with for WordPress because it is opinionated in many ways and those opinions different in most things that WordPress needs to do.

If it is problematic to apply (de-facto) best practices to a project, chances are the best practices are not a fault. This probably just points to an inherent design flaw that should be addressed.

[...] WPLib Box [...]

There are lots of solutions available for something like this, all with different kinds of merits. I don't think this ticket is the best place to discuss these, though.

My preference would be that WordPress core would leverage a highly-optimized class-map autoloader (and not have the classmap be "temporary") used both for performance and for minimizing amount of code that one has to step through during debugging with XDEBUG, and encourage plugins and themes to use the class-map too.

The Composer autoloader is generated by an AutoloadGenerator ... which can be completely customized. If there is a concensus on how the resulting autoloader should look, just let me know and I'll quickly code up a generator for that particular format. Composer is flexible enough to support ANY resulting classloader format, not just the one shipped by default.

Ideally the class-map autoloader would be optimized for the number of lines that need to run for a file to be autoloaded, and that the class-map loader would be run first in front of the other optional autoloaders. And rather than separating concerns for the class-map loader, optimize it instead.

The optimization is certainly something that needs to be done, but premature optimization at this point is completely useless (and counter-productive), as we're just figuring out how to autoload at all without breaking stuff at this point.

That being said, the Composer autoloader is already tightly optimized in a proven way, and it is difficult to get any improvements over it with anything but very simple scenarios.

class-foo-bar.php vs. WP_Foo_Bar.php

While I agree that using class names instead of arbitrary filenames, this discussion is not relevant for the autoloader and should be done in the context of the WP Coding Standards efforts.

I started our WPLib autoloader following that pattern, but am currently working on a version that will move to using classnames for filenames because it solves the problem of to find the filename for a class in a classmap. This is very helpful when writing code that needs to be able to determine what plugin a class is in, or if it in the theme, for example.

The whole point of a class map autoloader is to associate arbitrary filenames with class names. If you can determine one from the other, you don't need a class map. You can just use a PSR-0 or PSR-4 autoloader.

Class-Map Generation.

BTW, I've found a really nice way to build a class-map even with the other autoloaders. Have each autoloader add any classes found to the classmap and then when WP_DEBUG is active write out to a file on a 'shutdown' hook which can then be loaded in wp-settings.php. Which it works well for WPLib there are issues with implementing it in core, but it is something to explore.

I fail to see how that would be preferable to have a mature, 5-year-tested class map builder that is used by the rest of the PHP world.

#146 in reply to: ↑ 144 ; follow-up: @schlessera
8 months ago

Replying to MikeSchinkel:

For our own project (WPLib) we added our own autoloader in front of Composer's autoloader because Composer's autoloader often runs through a painful amount of code before it actually identifies the file to load.

This made me curious, so I checked to see what the actual code path is for the proposed Composer class map autolaoder that was committed.

First, let's check what function gets registered as the autoload callback:

spl_autoload_register(array($this, 'loadClass'), true);

So, when a class is not yet known, the PHP autoloader will be invoked, and it will call the ClassLoader::loadClass() method. So far so good, not much to optimize there, as this is all PHP internals up to that point.

It gets more interesting if we take a look at the code if this loadClass() method:

public function loadClass($class)
{
    if ($file = $this->findFile($class)) {
        include $file;
        return true;
    }
}

It tries to find the needed file, includes it and returns. Not much to optimize here either.

And then finally, let's take a look at the code path of the ClassLoader::findFile() method, provided that we're using a class map as is currently the case here:

public function findFile($class)
{
    if ('\\' === $class[0]) {
        $class = substr($class, 1);
    }

    if (isset($this->classMap[$class])) {
        return $this->classMap[$class];
    } // [...]
}

So, it first makes sure to have a normalized class name, then retrieves it from the class map and returns.

I don't think there's much potential for optimization in the code above, for the scenario we have been describing here.

Yes, if the class was not found, the code continues, and yes, if you use different kinds of autoloaders, different code paths get executed. But as you mentioned you were using class maps, I fail to see how you optimized the above by prepending your own class loader.

#147 in reply to: ↑ 137 ; follow-up: @dnaber-de
8 months ago

Replying to azaozz:

Right. In "pluggable.php" this is the intended behaviour, and the design is bad. However because of the way WordPress works, adding standard autoloading of classes introduces exactly the same design flaws like in pluggable.php. Of course we can add all the documentation and warnings we want, but do you believe all developers will always follow them? After the example with the shortcodes the answer tends to be firm "No".

Could you please explain how there's a difference between:

<?php
add_action( 'setup_theme', function() {
    $GLOBALS[ 'wp_rewrite' ] = new TotallyBrokenWpRewrite;
} );

and

<?php
add_action( 'plugins_loaded', function() {
    require_once 'src/TotallyBrokenWpRewrite.php';
} ) );

The last one would possibly bypass the core autoloader. The first one replaces the global instance. Both override the default WP_Rewrite implementation. The first one is possible right now.

We're talking about a slightly different facet of an already existing problem. But maybe I miss something here.

#148 in reply to: ↑ 147 @jdgrimes
8 months ago

Replying to dnaber-de:

I think the primary difference is that in the first case multiple plugins can do that, and one can't prevent another from doing so. A plugin can even wrap the original object and pass everything that it doesn't specifically need to override through to the original object with __call() and __get(). Multiple plugins can do this, likely without outright killing the site, though results will probably vary. :-)

With the second case, once one plugin has included the class, no other class of that name can be loaded without a fatal error. So it is a race to be first, because only the first to load the class wins.

#149 in reply to: ↑ 144 ; follow-up: @chrisvanpatten
8 months ago

Replying to MikeSchinkel:

In summary related to Composer is that it is not well suited for WordPress website.

Admittedly I'm coming from the perspective of someone using Composer for WordPress dependency management—which I know is not even remotely in the scope of this ticket—but based on that experience, on my own research, and the comments of Composer pros like @schlessera on this ticket, it seems more than flexible enough for anything you can throw at it with regards to autoloading.

Do you have specific examples of things that you experienced that make it poorly suited for autoloading?

Last edited 8 months ago by chrisvanpatten (previous) (diff)

#150 follow-ups: @MikeSchinkel
8 months ago

However, what you call "groupthink" (which I find rude, tbh)

I apologize if it came across as rude. That was not my intention.

I used "groupthink" in the same context that management consultants use it to try to improve business decision making, for example. Think of it like me saying "Let me propose a strawman" but in a different context.

is an effort to bring de facto PHP best practices to WordPress, trying to get it out of the "legacy software" drawer it keeps getting put into more and more.

See my comments ''(to you)'' here about there being different levels of programmers and how we should not be forcing all of them to be at the highest skill level.

Think of them as diffferent constituents, all of which we should guard their interests and not forsake any of them.

I think that all the people contributing on Trac are interested in seeing a bright future for WordPress,...

And my reason for pushing this issue is that I think that taking it down the path or ratcheting up the skill level required to be productive with WordPress would dim that future significantly.

If you use a tried and tested autoloader, there's no need to step through it with XDEBUG, so I don't see why this would be relevant.

In theory. But the reality is that when debugging you are in discovery mode and frequently you are still trying to get your bearing so frequently (at least I) accidently step into autoloaders when I do not intend to.

If it is problematic to apply (de-facto) best practices to a project, chances are the best practices are not a fault. This probably just points to an inherent design flaw that should be addressed.

You are assuming that so-called "best practices" are actually fit for purpose. That logic is called "appeal to authority appeal to authority" and is a logical fallacy.

In my 25+ years of professional development I have seen many "best practices" exposed as only being Well it seemed like a good idea at the time. So we should not view "best practices" as a sacred sword and instead validate each practice to see if it actually applies in the current context.

but premature optimization at this point is completely useless (and counter-productive),

Unless, by your arguments in support of your positions, that doing so would paint us into an architectural corner that we could not later extract ourself from. And it is my currently believe that once Composer gets into core it will be there until WordPress is no longer relevant such like SVN and other infrastructure technologies.

That being said, the Composer autoloader is already tightly optimized in a proven way, and it is difficult to get any improvements over it with anything but very simple scenarios.

It is not nearly as an autoloader that works like this:

function _autoload( $class ) {	
   if ( isset( self::$_classmap[ $class ] ) ) {
	  require self::$_classmap[ $class ] 
   } 
}

this discussion is not relevant for the autoloader and should be done in the context of the WP Coding Standards efforts.

Beg to disagree. The autoloader is in part driving the requirement. To discuss in WP Coding Standards would be the cart driving the horse.

The whole point of a class map autoloader is to associate arbitrary filenames with class names. If you can determine one from the other, you don't need a class map. You can just use a PSR-0 or PSR-4 autoloader.

PSR-0 and PSR-4 are optimized for PHP framework style projects. WordPress organizes its code differently and thus -- with the exception of using libraries from packagist.org on WordPress -- PSR-0 and PSR-4 are not a good match for WordPress.

And classmaps can easily be generated.

I fail to see how that would be preferable to have a mature, 5-year-tested class map builder

Because Composer's defaults actively work against WordPress's architecture.

You don't use a sports car to haul a load of hay, even if it is the best damn sports car in the world.

#151 in reply to: ↑ 146 @MikeSchinkel
8 months ago

Replying to schlessera:

I don't think there's much potential for optimization in the code above, for the scenario we have been describing here.
I fail to see how you optimized the above by prepending your own class loader.

See the example code from my previous message.

#152 follow-up: @TJNowell
8 months ago

It's nice to see somebody playing devils advocate :)

I would note that a Composer autoloader is not a full copy of Composer, and PSR standards autoloaders aren't relevant here as Core doesn't use PSR. Composer is being used as a build time tool here much in the way that NPM or Grunt might be used, only the final autoloader produced is relevant.

As an aside, Core could include a Composer autoloader if it exists, even if it doesn't make use of it internally. This would be a great help for those of us who use Composer, eliminating an extra step in the setup, and standardising when the autoloader is added.

The Composer classmap autoloader is widespread and battle tested, I believe atm that puts it ahead of homegrown options regardless of best practices. If better can be made I'm sure the Composer project will be interested in our findings.

Mike, if you'd like to work on a generator proposal so that we can have Composer build an autoloader that matches your own specifications rather than those of the Composer project I believe that would be the most productive action to take. At that point we can make direct comparisons, and perhaps learn things of practical use

#153 in reply to: ↑ 149 @MikeSchinkel
8 months ago

Replying to chrisvanpatten:

it seems more than flexible enough for anything you can throw at it with regards to autoloading.

Flexibility is not my concern. Complexity is.

Do you have specific examples of things that you experienced that make it poorly suited for autoloading?

Great question. See my follow up to @TJNowell in a few minutes.

#154 in reply to: ↑ 152 ; follow-up: @MikeSchinkel
8 months ago

Replying to TJNowell:

I would note that a Composer autoloader is not a full copy of Composer...

Pulling back from the debate thanks to @chrisvanpatten and your comments, it is clear I am not even clear what is being debated on this ticket. I think the reason is because this ticket was inadvertently hijacked to go in a different direction from the OP's intent.

Minimally I think if we are going to discuss an autoloader based on Composer we should respect @dnaber-de and not hijack his ticket. Instead we should discuss his proposal (which I think is mostly good) and give it a thumbs up or a thumbs down.

If we instead want to advocate for a Composer autoloader it should be written up in another ticket form and we can and should debate it there.

So rather than me answer your questions here I would suggest someone who is a strong advocate of using a Composer autoloader create a clear proposal on another ticket and link from here to that ticket and then I can register my objections (or agreements) there.

Mike, if you'd like to work on a generator proposal...

Frankly if we consider this ticket's original proposal it would be very close to what I would propose so it would not make sense for me to split discussion of improvements to @dnaber-de's proposal on to another ticket.

#155 in reply to: ↑ 150 ; follow-up: @jb510
8 months ago

First, I read this whole thread and learned a lot. I really wish more trac tickets had such intelligent discussion. TY to all of you.

Replying to MikeSchinkel:

is an effort to bring de facto PHP best practices to WordPress, trying to get it out of the "legacy software" drawer it keeps getting put into more and more.

See my comments ''(to you)'' here about there being different levels of programmers and how we should not be forcing all of them to be at the highest skill level.

Think of them as diffferent constituents, all of which we should guard their interests and not forsake any of them.

As one of the those in the "different levels of programmers" group. I don't see how adding an autoloader to core would hurt my ability, or my fellow novice and intermediates, from working with WordPress. For the most part novice and intermediate level developers expect core to "just work" out fo the box and it does so with or without an autoloader.

In fact I feel like most new developers are coming to WP having learned current best practices elsewhere (Zend/Laravelle) so they are more confused when they discover WP doesn't do things more modern (advanced) way. Adopting an auto-loader might even help the them. Really though my point is I don't think developer expertise issue really comes into play when we're talking about core until you get really freaky.

If/when plugin devs can later leverage a core loader I think it would further help those same programmers since they'd start seeing things done in a similar way across multiple plugins, rather than the anarchy that currently exists (most often seems to be using Composer, but I see weird bespoke systems too).


While it sounds like the goal of this ticket is simply an auto loader for core (yay for narrow focus) and that the likely candidate for the is Composer while still considering other options. A point I'm unclear on at this point in the discussion is about use outside of core which I think needs at least superficial consideration.

If the answer is based on composer. Is the thought that plugin/theme developers could then leverage composer directly from core to create their own auto-loader, or is the thought that they would directly use a Core autoloader class? As a middle level dev, I only really care about two things, 1) that how core does it works seamlessly and painlessly, and 2) if I can/can't/should/shouldn't leverage how core does it.

From what I've read here, it sounds like there would be a lot of value to using composer in a way that plugin/theme devs could leverage it directly, but as a middle level developer, I'll certainly defer to the experienced folks here just thought an opinion from a "different group" might help.

#156 in reply to: ↑ 155 ; follow-up: @MikeSchinkel
8 months ago

Replying to jb510:

As one of the those in the "different levels of programmers" group. I don't see how adding an autoloader to core would hurt my ability, or my fellow novice and intermediates, from working with WordPress.

You misunderstood my comments. I would definitely like to see an autoloader added to WordPress. I proposed one 22 months ago here on Trac. Instead I have an objection to the use of Composer as I believe it is a poor fit for use with WordPress.

In fact I feel like most new developers are coming to WP having learned current best practices elsewhere (Zend/Laravelle) so they are more confused when they discover WP doesn't do things more modern (advanced) way.

You are definitely not in the majority of people working with WordPress and thus I did not include your archetype in the list above. My experience you represent a very tiny percent of people who are new to WordPress; most people new to WordPress were not previously PHP developers. AND you also represent a group of people who (hopefully would) have the skills to figure things out vs. those who first learned PHP in order to theme WordPress or write a plugin for WordPress.

A point I'm unclear on at this point in the discussion is about use outside of core which I think needs at least superficial consideration....If the answer is based on composer. Is the thought that plugin/theme developers could then leverage composer directly from core to create their own auto-loader, or is the thought that they would directly use a Core autoloader class? As a middle level dev, I only really care about two things, 1) that how core does it works seamlessly and painlessly, and 2) if I can/can't/should/shouldn't leverage how core does it.

Those are great questions, and ones I am definitely not clear on either.

But I do think to discuss Composer it should be done on a ticket that provides the proposal clearly in the title and description of the ticket.

#157 in reply to: ↑ 156 ; follow-up: @jb510
8 months ago

Replying to MikeSchinkel:

You are definitely not in the majority of people working with WordPress and thus I did not include your archetype in the list above. My experience you represent a very tiny percent of people who are new to WordPress; most people new to WordPress were not previously PHP developers. AND you also represent a group of people who (hopefully would) have the skills to figure things out vs. those who first learned PHP in order to theme WordPress or write a plugin for WordPress.

I'm not sure I follow or agree here. I maintain that the vast majority whom are new to WordPress and to PHP _really_ don't care how an auto loader in core gets constructed. They just care that core works, full stop. Nothing about an autoloader in core increases the technical hurdle for the vast majority doing all the things they do with WordPress. By the time they care about core, they're at least at my level (Unrelated but you probably over-estimate my PHP skills, I've been around WP a long time and can figure things out by I'm a hack).

The vast majority that touch PHP just tweak themes, and maybe right a simple functional plugins. Nothing about that space changes with an autoloader in core, let alone how an auto loader gets constructed.

But I do think to discuss Composer it should be done on a ticket that provides the proposal clearly in the title and description of the ticket.

Certainly, adding composer for composer's sake should get discussed elsewhere and I certainly didn't mean to derail into that. Just as I said above, I don't think a composer vs non-composer autoloader makes a technical difference for anyone looking into core for the first time.

FWIW, the interface vs. implementation abstraction seems way more complicated to me but I could figure it out and after a lot of years I've learned to embrace such abstractions.

#158 in reply to: ↑ 154 ; follow-up: @dnaber-de
8 months ago

Replying to MikeSchinkel:

Minimally I think if we are going to discuss an autoloader based on Composer we should respect @dnaber-de and not hijack his ticket. Instead we should discuss his proposal (which I think is mostly good) and give it a thumbs up or a thumbs down.

I'm perfectly fine with the direction the ticket goes (until the discussion went somehow off topic), but thanks. My intention was to have a single autoloader instance for performance reasons. If that would have been a custom implementation I'd have to write a composer plugin that uses this WordPress own implementation. If WordPress uses Composer itself, it's even better. I read opposed arguments, thought about it and finally let them convince me that this is the best way to go.

Replying to TJNovel

As an aside, Core could include a Composer autoloader if it exists, even if it doesn't make use of it internally. This would be a great help for those of us who use Composer, eliminating an extra step in the setup, and standardising when the autoloader is added.

Exactly. That would be the next step to take, to make WordPress configurable to include an external composer autoload file if WordPress is installed via composer. So let's focus on the topic and discuss @schlessera 's proposal and move the composer.json back to the root of the repository.

#159 in reply to: ↑ 157 ; follow-up: @MikeSchinkel
8 months ago

Replying to jb510:

I'm not sure I follow or agree here.

Agreed. you are again not following my objections and thus are misrepresented them.

I maintain that the vast majority whom are new to WordPress and to PHP _really_ don't care how an auto loader in core gets constructed.

Agreed. But my issue is not people "New to WordPress" (I only mentioned ""new to WP"" because you said you were new to WP.) My issue is that there are many skill levels of people who extend WP, and many of those people are not skilled PHP programmers even though they end up writing PHP. And most of these people are definitely not new to WP.

The vast majority that touch PHP just tweak themes, and maybe right a simple functional plugins. Nothing about that space changes with an autoloader in core, let alone how an auto loader gets constructed.

On this we disagree. An autoloader for core should not be added to core that does not at least contemplate being used for themes and plugins too. And for that Composer is too complicated for many of the people who are currently empowered by WordPress because it is easy. IMO.

BTW, if the core developers decide to choose Composer I will accept it and move on. But as long as they have not spoken and informed us of a concrete decision I feel the need to provide an opposing voice to what otherwise appears to be group-think based on an unvalidated assumption that using Composer is the best approach to take.

#160 in reply to: ↑ 158 ; follow-up: @MikeSchinkel
8 months ago

Replying to dnaber-de:

I'm perfectly fine with the direction the ticket goes (until the discussion went somehow off topic), but thanks.

Then let me elaborate on my primary reason for wanting another ticket; to ensure that we can get one clear proposal about the direction that is evidently now being proposed because comments don't work well to establish the primary thesis to evaluate. You mention @schlessera' proposal, but he has posted 25 messages in this thread with 159 messages, and it is unclear to me which of his messages was his proposal.

Worse case, if sticking with this ticket makes sense, can we nominate someone to write up the new thesis in a concise form and post it here as a revised proposal, because as I said it is not longer clear what is actually currently being proposed?

Last edited 8 months ago by MikeSchinkel (previous) (diff)

#161 in reply to: ↑ 159 @tfrommen
8 months ago

Replying to MikeSchinkel:

Replying to jb510:

I'm not sure I follow or agree here.

Agreed. you are again not following my objections and thus are misrepresented them.

Mike, I'm afraid he's not the only one.

#162 in reply to: ↑ 160 ; follow-up: @tfrommen
8 months ago

Replying to MikeSchinkel:

Replying to dnaber-de:

I'm perfectly fine with the direction the ticket goes (until the discussion went somehow off topic), but thanks.

Then let me elaborate on my primary reason for wanting another ticket; to ensure that we can get one clear proposal about the direction that is evidently now being proposed because comments don't work well to establish the primary thesis to evaluate. You mention @schlessera' proposal, but he has posted 25 messages in this thread with 159 messages, and it is unclear to me which of his messages was his proposal.

The "proposal" is presented in this post.

Worse case, if sticking with this ticket makes sense, can we nominate someone to write up the new thesis in a concise form and post it here as a revised proposal, because as I said it is not longer clear what is actually currently being proposed?

I would also welcome this. However, there will still be several ways to do this, and several opinions about how to do this "right".

Last edited 8 months ago by tfrommen (previous) (diff)

#163 in reply to: ↑ 150 ; follow-up: @schlessera
8 months ago

Replying to MikeSchinkel:

I apologize if it came across as rude. That was not my intention.
I used "groupthink" in the same context that management consultants use it to try to improve business decision making, for example. Think of it like me saying "Let me propose a strawman" but in a different context.

No worries, and thanks for taking the time to clarify.

is an effort to bring de facto PHP best practices to WordPress, trying to get it out of the "legacy software" drawer it keeps getting put into more and more.

See my comments ''(to you)'' here about there being different levels of programmers and how we should not be forcing all of them to be at the highest skill level.

Discussed this argument here

If it is problematic to apply (de-facto) best practices to a project, chances are the best practices are not a fault. This probably just points to an inherent design flaw that should be addressed.

You are assuming that so-called "best practices" are actually fit for purpose. That logic is called "appeal to authority appeal to authority" and is a logical fallacy.
In my 25+ years of professional development I have seen many "best practices" exposed as only being Well it seemed like a good idea at the time. So we should not view "best practices" as a sacred sword and instead validate each practice to see if it actually applies in the current context.

We are talking about a very specific problem here, and the solution we're proposing has been considered a best practice for pretty much the entire PHP ecosystem for years now. I agree that one should always apply critical thinking before accepting "best pratices", but I am fully confident about our assertion here.

It is not nearly as an autoloader that works like this:

function _autoload( $class ) {	
   if ( isset( self::$_classmap[ $class ] ) ) {
	  require self::$_classmap[ $class ] 
   } 
}

As I already dissected in a later comment, that is pretty much exactly what the Composer ClassMap loader does, plus a single additional instruction to normalize the class name, so that it does not break with absolute class names (which the above one probably does).

I fail to see how that would be preferable to have a mature, 5-year-tested class map builder

Because Composer's defaults actively work against WordPress's architecture.

Composer has no defaults. Its defaults are to do nothing at all. If you are talking about any particular way of setting it up, that's entirely configurable.

One more though on the general direction of this ticket:

My preference would be to only consider Composer as built-time tool for now to build a 5.2-compatible class map that WordPress Core can use. Then use this to modify Core so that it can make better use of the autoloader. I expect his to have a positive impact (directly and indirectly) on the code quality and architecture of Core.

Considering writing an API to provide a class autoloader to plugins & themes is too early right now, and not all of the preconditions for properly designing something like this are met yet.

#164 in reply to: ↑ 163 ; follow-up: @swissspidy
8 months ago

Replying to schlessera:

One more though on the general direction of this ticket:

My preference would be to only consider Composer as built-time tool for now to build a 5.2-compatible class map that WordPress Core can use. Then use this to modify Core so that it can make better use of the autoloader. I expect his to have a positive impact (directly and indirectly) on the code quality and architecture of Core.

Considering writing an API to provide a class autoloader to plugins & themes is too early right now, and not all of the preconditions for properly designing something like this are met yet.

I think this sums it up quite nicely.

#165 in reply to: ↑ 164 @boonebgorges
8 months ago

Replying to swissspidy:

Replying to schlessera:

One more though on the general direction of this ticket:

My preference would be to only consider Composer as built-time tool for now to build a 5.2-compatible class map that WordPress Core can use. Then use this to modify Core so that it can make better use of the autoloader. I expect his to have a positive impact (directly and indirectly) on the code quality and architecture of Core.

Considering writing an API to provide a class autoloader to plugins & themes is too early right now, and not all of the preconditions for properly designing something like this are met yet.

I think this sums it up quite nicely.

Agreed. To reiterate: the only use of Composer being suggested here is to generate a classmap at build time. The only people who will ever have to worry about using Composer, on this proposal, are committers, and core contributors whose patches change the classmap (by adding/removing/renaming classes).

#166 follow-up: @JohnPBloch
8 months ago

It seems to me that @MikeSchinkel's point that Composer does a lot of extra processing after classmaps is one worth acknowledging and incorporating. For core's own autoloading needs, obviously this won't be an issue. The performance question comes into play later for any plugins that might also trigger the autoloader. Because those plugin classes aren't in core's classmap, it would needlessly run through all the other autoloader checks only to predictably come up with nothing.

To that end, I propose that we run with @schlessera's proposal to fork the PHP 5.2 autoloader generator and, in addition to making it use ABSPATH rather than a path relative to composer.json, it should strip out (i.e., neglect to generate) all logic in the loading method after the class map until such time that core actually uses one of the autoloading PSRs (which is a completely different conversation).

#167 in reply to: ↑ 166 ; follow-ups: @rmccue
8 months ago

Replying to JohnPBloch:

To that end, I propose that we run with @schlessera's proposal to fork the PHP 5.2 autoloader generator and, in addition to making it use ABSPATH rather than a path relative to composer.json, it should strip out (i.e., neglect to generate) all logic in the loading method after the class map until such time that core actually uses one of the autoloading PSRs (which is a completely different conversation).

Half the reason Composer was suggested in this ticket was to reduce how much work we'd have to do, since we could have something ready immediately that "just works". Now, the proposal is to rewrite this functionality anyway.

I still think this seems to be a solution (Composer) searching for a problem.

#168 in reply to: ↑ 167 @jtsternberg
8 months ago

Half the reason Composer was suggested in this ticket was to reduce how much work we'd have to do, since we could have something ready immediately that "just works". Now, the proposal is to rewrite this functionality anyway.

Right, and there is no reason this cannot be an iterative approach, starting w/ the tried/true composer autoloader and optimizing after the cow paths have become worn in.

#169 in reply to: ↑ 162 @MikeSchinkel
8 months ago

Replying to tfrommen:

The "proposal" is presented in this post.

Thanks. That was very helpful.

However, the proposal did not summarize so I'm still a bit unclear with what is being proposed. Let me summarize to see if I can get it right:

  1. Add composer.json to WordPress
  2. Change wp-config.php to load a Composer autoloader.
  3. Generate a PHP52 compatible autoloader using xrstf/composer-php52
  4. Have Composer generate a classmap for wp-includes directory
  5. Fix anything in wp-includes that is not 1 file=1 class.
  6. Include the generated classmap and autoloader with WordPress core.
  7. Once we get beyond PHP52, bundle Composer into WordPress core.

Did I correctly summarize the proposal that is currently being debated? If not, what did I get wrong?

Last edited 8 months ago by MikeSchinkel (previous) (diff)

#170 @MikeSchinkel
8 months ago

Since I don't think I will have much time over the next week I am going to go ahead adiscuss my concerns with it.

I am definitely in favor of add composer.json to WordPress. It does not make anything complicated for people who do not use Composer but for those who do use it having it in core makes their lives easier.

Beyond that it seems the primary short-term benefit of using Composer is to generate the classmap? (It can't be because of autoloader generation because if you have a classmap the autoloader is trivial.) Is there not any other benefit?

Check me here if I am wrong, but is generating a classmap really that hard? I do it for WPLib and it seems pretty trivial. What am I missing?


And then there is the idea of bunding Composer with WordPress. What would be benefit of that? For people who use composer already, or for end-users who download and install plugins and themes from WP.org?

If they use Composer already, why would they want to have to worry about incompatible versions?

If they are end-users how would you get Composer to handle the dynamic load scenario of plugins and themes?


Further, if plugins and themes wanted to use an autoloader would they using their own directories for code storaage, or in the wp-vendor directory? Would they be responsible to create a project on packagist.org to point to their code for their plugin, and then the plugin file in the plugins directory would just be a loader for the real plugin in vendors?

Or would each plugin need to define its own package type so they can each map their own includes directory inside composer.json so that Composer will know to place their autoload files in the right plugin or theme directory?, e.g.

type:yoast-seo => `wp-content/plugins/wordpress-seo/inc

Frankly, quoting @rmccue:

I still think this seems to be a solution (Composer) searching for a problem.

#171 follow-up: @TJNowell
8 months ago

And then there is the idea of bundling Composer with WordPress. What would
be benefit of that? For people who use composer already, or for end-users
who download and install plugins and themes from WP.org?

Nobody is suggesting we bundle Composer with WordPress, Composer is a CLI tool, and to bundle it with Core would be very unusual. That's not how Composer is meant to be installed or used.

What we're talking about is an autoloader, which is generated as output by the Composer tool, not Composer itself. It was mentioned because Composer generated autoloaders are widespread, well known, reliable, well tested, and have had the attention of some of the biggest companies and most experienced PHP developers alive. It's also in use in a great number of WordPress agencies, plugins, and themes, so there is existing familiarity with how it works, it's not a foreign unknown.

This gets you these 10 files:

  • vendor/autoload.php - the bootstrapper
  • vendor/composer/ClassLoader.php - implements a PSR-0, PSR-4 and classmap class loader
  • vendor/composer/LICENSE - license
  • vendor/composer/autoload_classmap.php - an array of class names and the files they can be found in
  • vendor/composer/autoload_files.php - an array of files that will always be loaded immediatley
  • vendor/composer/autoload_namespaces.php - an array of folder to namespace mappings
  • vendor/composer/autoload_psr4.php - an array mapping folders to PSR4 namespaces
  • vendor/composer/autoload_real.php - an autogenerated class with a build specific hash in the name to create the autoloader and fill it with the above files
  • vendor/composer/autoload_static.php - a map of all classes/namespaces, and their files, an amalgam of the other autoload_*.php array files
  • vendor/composer/installed.json - a json file of what is being autoloaded

Composer can also be passed a generator that controls what is generated when an autoloader is created. The above list is merely the default.

I'm also unsure why we're renaming the vendor folder to wp-vendor, it's strange and non-standard.

I'd also advise against including the autoloader in wp-config.php, wp-config.php is for setting constants and configuring, including the autoloader there will necessitate modifying every config file that exists already, as well as handling those scenarios when the file is up one directory, etc

#172 in reply to: ↑ 171 @MikeSchinkel
8 months ago

Replying to TJNowell:

Nobody is suggesting we bundle Composer with WordPress.

Not true. From @schlessera's own comments on his proposal we are discussing:

For the approach I recommended above, Composer would only be used at development time for as long as WP stays at PHP 5.2. After that, you can just as well include Composer with WordPress, it does not need to be installed globally. So, if you want to bake in an autoloader into WordPress Core, why not just use the one that the rest of the PHP world uses?

Composer generated autoloaders are widespread, well known

The fact that something is in widespread use is not a reason to use it when it is not fit for purpose

Composer champions an approach that places all autoloadable classes into a /vendor/ directory and then maps to the file based on class names. WordPress places autoloadable files in at least 5 different general locations and mapping from classname to those locations would require torturous naming conventions, or it would require the Composer autoloader to look into tens if not hundreds of directories:

  • wp-includes
  • wp-admin
  • wp-content/plugins/
  • wp-content/mu-plugins
  • wp-content/themes/

Sure, we could collapse wp-admin into wp-includes by moving all the admin classes into wp-includes`, but does that really make sense?

Then within each of /plugins and /mu-plugins there can be a potentially unlimited number of locations to look for classes to autoload.

The fact you cannot map classes in WordPress plugins and themes simply by their classnames is the main reasons why Composer is not a good fit as an autoloader for WordPress. And no one on this ticket has acknowledged and addressed this concern.

I have been working with this exact problem in WPLib for several years, so I have some strong insight into the problem. I know that had I not gone through this painful effort I would not have recognized the issues with Composer either and would have thought using it to be a great idea. Matter of fact, I previously did think Composer was a great idea for WordPress.

Autoloader requirements for add-ins turns out to be different than an autoloader for site builders unless WordPress changes the location of plugins and themes to be inside a /vendor/ directory. And even then it would still have an impedance mismatch.

Really the only approach that works across all of WordPress is a classmap, and there is no reason to include a complex Composer autoloader just to get a classmap autoloader. And having core use its own classmap autoloader does not preclude anyone from using Composer's autoloader for PSR0/4 libraries.

Composer generated autoloaders are ... reliable, well tested,

A classmap autoloader is so trivial that it makes this concern moot.

I could provide a working example to illustrate but this discussion is coming at a really bad time for me because I have other hard deadlines over the next 10 days that keep me from devoting the time to it.

This gets you these 10 files:

Why include (some of) these 10 files when you really only need two trivial files? One containing a trivially simple autoloader and another file containing a trivially simple classmap?

In addition core could load core's classmap and then run hooks to allow plugins and themes to contribute their own classmaps.

Composer can also be passed a generator that controls what is generated when an autoloader is created. The above list is merely the default.

Yes Composer is flexible, by why go to so much effort to make it contort in ways the Composer team choose not to support? (Go back and read some of their GitHub discussions.)

BTW, I do really like the OP's original proposal of having pluggable autoloaders to make it easy to use PSR0 and PSR4 when site builders want to use libraries that were designed with PSR0/4 in mind.

I'd also advise against including the autoloader in wp-config.php, wp-config.php is for setting constants and configuring, including the autoloader there will necessitate modifying every config file that exists already, as well as handling those scenarios when the file is up one directory, etc

Agreed there.

#173 follow-up: @TJNowell
8 months ago

Not true. From @schlessera's own comments on his proposal we are discussing:

That makes no sense, that's not how Composer works, perhaps you've misunderstood use of composer for inclusion of composers entire codebase, something that is not being suggested. The language could be misleading for somebody without knowledge of how Composer is used.

Composer champions an approach that places all autoloadable classes into a /vendor/ directory and then maps to the file based on class names.

These are the PSR standards, we don't use those, and we don't need to, this ticket discusses classmaps, and the classmap autoloader is what's under discussion. You do not need to put classes in the vendor folder to autoload them, and you do not need to follow PSR conventions.

Then within each of /plugins and /mu-plugins there can be a potentially unlimited number of locations to look for classes to autoload.

This is the core autoloader, mu-plugins and plugins are not core, plugin developers can and do handle loading of files themselves, this is a separate topic.

And having core use its own classmap autoloader does not preclude anyone from using Composer's autoloader for PSR0/4 libraries.

We're not using PSR0/4, neither is it necessary to use such PSR0/4

Why include (some of) these 10 files when you really only need two trivial files? One containing a trivially simple autoloader and another file containing a trivially simple classmap?

Some plugin authors may wish to use PSR0/4, and it would prevent compatibility problems if a plugin required a library that used such standards. While core doesn't use it, we shouldn't prevent it, or cause issues elsewhere. Or we could generate our own custom autoloader. For example rmccue/requests

In addition core could load core's classmap and then run hooks to allow plugins and themes to contribute their own classmaps.

I agree, the main ClassLoader object has methods for adding classmaps and other information about where to load things, it should be passed through a filter, if only for debugging purposes

#174 in reply to: ↑ 173 @MikeSchinkel
8 months ago

Replying to TJNowell:

That makes no sense, that's not how Composer works, perhaps you've misunderstood use of composer for inclusion of composers entire codebase, something that is not being suggested. The language could be misleading for somebody without knowledge of how Composer is used.

What makes no sense? My comments on @schlessera's comment, or @schlessera's comment?

If you think the former, please explain how my language is misleading.

If the latter, then of course I agree, but don't shoot the messenger.

These are the PSR standards, we don't use those, and we don't need to, this ticket discusses classmaps, and the classmap autoloader is what's under discussion. You do not need to put classes in the vendor folder to autoload them, and you do not need to follow PSR conventions.

Exactly. So if we do not need PSR standards then why use a tool that was purpose-built to support PSR standards and bring along all its related baggage?

This is the core autoloader, mu-plugins and plugins are not core, plugin developers can and do handle loading of files themselves, this is a separate topic.

Why paint ourselves into a corner like that? It is downright foolish to adopt a solution for just core and ignore how it would eventually be used for plugins and themes especially since we've already pointed out how problematic using it would be for plugins and themes.

To illustrate the mismatch with Composer and WordPress with Plugins and Themes let me quote from the Composer team member's comment that I linked but you evidently missed:

Pretty much every project except wordpress at this point works in the same way: you have a composer.json and declare the list of plugins you want, and they get installed, and that's that. One autoloader, then some plugins/libraries/whatever packages. If every wordpress plugin embeds a composer autoloader, it's because the ecosystem is broken and wordpress core doesn't give a damn about moving forward. It's not something I am willing to waste time on I'm sorry..

Some plugin authors may wish to use PSR0/4, and it would prevent compatibility problems if a plugin required a library that used such standards.

What compatibility problems? The plugin cannot use PSR0/4 AND the pre-generated autoloader that is being proposed for inclusion in core unless the plugin moves its files into the /vendor directory (or similar), which is a non-starter.

Composer autoloader generation is designed to be used at project (site) build time. That simply does not match how WordPress plugins and themes work.

If a site builder wants to use PSR4 to autoload their own plugins classes then they can use Composer and there will be no compatibility problems.

For example rmccue/requests

And Requests is a PHP library, it is not a WordPress plugin or theme. And it would be included at build time. So whoever wants to use it and Composer can with no compatibility problems with a simple core autoloader.

#175 follow-up: @TJNowell
8 months ago

What compatibility problems? The plugin cannot use PSR0/4 AND the

pre-generated autoloader that is being proposed for inclusion in core
unless the plugin moves its files into the /vendor directory (or
similar)
, which is a non-starter.

That's not true, anybody who has used Composer to set up Core and plugins will tell you so. I'm also not a fan of taking a widespread package, and redistributing one of its core classes with modifications. It's like taking a 6 pack of hotdog buns and tearing the packaging apart in the shop because you only want 2 for dinner.

It's clear that your understanding of how Composer is being used in WordPress installs, and the difference between the CLI tool, and the autoloader, is incomplete. I'm not going to derail this ticket any further in an effort to enlighten you

#176 in reply to: ↑ 175 @MikeSchinkel
8 months ago

Replying to TJNowell:

That's not true, anybody who has used Composer to set up Core and plugins will tell you so.

Please explain how it would work, because I am in the set of people you say will tell me so (I use Composer to set up Core and plugins, often) and I would not tell myself that.

I'm also not a fan of taking a widespread package, and redistributing one of its core classes with modifications.

I agree with your statement in general I do not follow the relevance in our debate.

I'm not going to derail this ticket any further in an effort to enlighten you

Convenient.

Last edited 8 months ago by MikeSchinkel (previous) (diff)

@MikeSchinkel
8 months ago

wp-classmapgen.php and example generated wp-classmap.php

@MikeSchinkel
8 months ago

wp-classmapgen.php and example generated wp-classmap.php

#177 follow-up: @MikeSchinkel
8 months ago

I've attached a strawman classmap generator and example output showing how classmaps can easily be generated without the need for Composer.

The next step would be to implement a simple classmap autoloader to be required at the beginning of wp-settings.

I would propose as another strawman that we create wp-includes/classes and wp-admin/classes directories and slowly start moving classes that we want to autoload into those two directories, renaming their files to match their classname.

So for example:

/wp-includes/class-wp-editor.php

Would be moved to:

/wp-includes/classes/_WP_Editors.php

Once all classes were moved into those directories then classmap generation could be handled automatically (if desired) in a 'shutdown' hook when a constant was set to true, e.g. WP_GENERATE_CLASSMAP

In addition I would envision adding a register_autoload_dir() that would allow plugins and themes to register their own autoload directories and this would allow WordPress to auto-generate their classmaps as well.

I intend to work on this after I finish some other time-critical projects.

#178 in reply to: ↑ 177 ; follow-up: @GaryJ
8 months ago

Replying to MikeSchinkel:

I would propose as another strawman that we create wp-includes/classes and wp-admin/classes directories and slowly start moving classes that we want to autoload into those two directories, renaming their files to match their classname.

FWIW, I would disagree about the classes naming of those directories, as it semantically excludes the potential future use of interface and traits, without having to add other directories to map to.

#179 in reply to: ↑ 178 ; follow-up: @tfrommen
8 months ago

Replying to GaryJ:

Replying to MikeSchinkel:

I would propose as another strawman that we create wp-includes/classes and wp-admin/classes directories and slowly start moving classes that we want to autoload into those two directories, renaming their files to match their classname.

FWIW, I would disagree about the classes naming of those directories, as it semantically excludes the potential future use of interface and traits, without having to add other directories to map to.

+1

In addition, there still would be classes that do not live in that folder.

#180 in reply to: ↑ 167 ; follow-ups: @JohnPBloch
8 months ago

Replying to MikeSchinkel:

Please explain how it would work, because I am in the set of people you say will tell me so (I use Composer to set up Core and plugins, often) and I would not tell myself that.

composer/installers handles putting plugins into wp-content/plugins, themes into wp-content/themes, and mu-plugins into wp-content/mu-plugins, or any location one wishes to configure in the root composer.json. Any package may define its type as wordpress-plugin|theme|mu-plugin and it will get installed in the correct location and all autoloading will simply work as expected.

This is all kind of beside the point of this ticket, as the goal point of this ticket is to add an autoloader for core to use, not plugins/themes (obviously, we don't want to preclude that enhancement later).

Replying to rmccue:

I still think this seems to be a solution (Composer) searching for a problem.

I've spent the long weekend mulling over this and really thinking about it and at this point I think you're mostly right. We've already figured out that stock composer out of the box is not a solution that fits core's needs very well. It's flexible enough that plugins could make it fit, sure, but at that point, we've almost definitely over-complicated things and need to ask ourselves why we're shaving this darn yak in the first place.

I still think it would be useful to use composer eventually, but this is mostly because I'm thinking of the possible uses for, say, adding dev dependencies like a specific version of phpunit, etc. However, such use cases are clearly out of scope for this ticket.

Besides, a home-grown autoloader now does not necessarily preclude using composer down the road if it starts to make more sense later, especially if we're careful about what the integration points look like for 3rd party code.

@schlessera @TJNowell What are your thoughts? you know how much I would love to use composer; but isn't it starting to seem to you that the work spent bending composer into a WP shape would be better spent sculpting WP into something a tiny bit more composer-shaped?

#181 @TJNowell
8 months ago

@JohnPBloch if the Composer autoloader is the best, it will show in time, so lets work on a simpler homegrown autoloader for now and learn from the Composer projects lessons.

I will say though, that we should filter things such that we can replace the autoloader. A composer autoloader still has merit for some developers, and I'd like to see experimentation in the future be possible, perhaps as a wp-config.php constant that disables the autoloader, on the premise that you will be loading your own.

A classmap containing an absolute namespaced class name and the file it resides in will suffice for our needs. 2 files, for an autoloader implementation, and a generated mapping file should be all that we need

#182 in reply to: ↑ 180 ; follow-up: @chrisvanpatten
8 months ago

Replying to JohnPBloch:

[...] isn't it starting to seem to you that the work spent bending composer into a WP shape would be better spent sculpting WP into something a tiny bit more composer-shaped?

I think there's a lot of sense in this final point. But if that's a generally shared conclusion, is there any point in pursuing a custom autoloader at all at this stage?

I think there's a lot of merit to the concerns about the "political" implications of a non-Composer autoloader. Sure, Core "could" switch to Composer at some point, but—if we're honest with ourselves—will that _really_ happen? Or will the custom autoloader become more entrenched, start to serve as a template for less experienced developers who adopt variations of it in their own work, and ultimately become so ingrained as "The WordPress Way" that a Composer autoloader changes from "some day" to "no way"?

If the goal is to—over dozens of cycles—make WordPress more consistent with generally accepted PHP best practices, what's the gain right now from putting in code—and creating technical debt—that's out of line with those best practices?

I'm not saying there isn't merit to a non-Composer autoloader, just that it seems like this is the kind of thing where it might be better to avoid an autoloader entirely and push for improvements in other areas… renaming classes, getting some kind of consistent hierarchy in place, pushing for 5.3 version minimum and namespaces, etc. That might take a while, but it seems like that groundwork is necessary anyway to really get the _most_ value out of any autoloader, whether custom or Composer-based.

Last edited 8 months ago by chrisvanpatten (previous) (diff)

#183 in reply to: ↑ 180 @schlessera
8 months ago

Replying to JohnPBloch:

Replying to rmccue:

I still think this seems to be a solution (Composer) searching for a problem.

I've spent the long weekend mulling over this and really thinking about it and at this point I think you're mostly right. We've already figured out that stock composer out of the box is not a solution that fits core's needs very well.

I fail to see in what way it does not fit. Just using it (without making changes to account for broken plugins) is a matter of a handful of lines. A handful of lines that make a huge leap forward to close some of the gap that has formed between WordPress and PHP development.

It's flexible enough that plugins could make it fit, sure, but at that point, we've almost definitely over-complicated things and need to ask ourselves why we're shaving this darn yak in the first place.

Still, I can't really follow. Maybe I'm just not intent enough on "preserving ways of the past".

  • "use existing mature library" vs "design, create and test own library, with runtime classes, CLI commands, etc..."
  • "point to Composer documentation" vs "create new sections in developer's handbook"
  • "use existing deployment tools" vs "creating/adapting WP-specific deployment tools"
  • ...

I fail to see how using Composer is more complicated than recreating an entire "ecosystem" from scratch.

I still think it would be useful to use composer eventually, but this is mostly because I'm thinking of the possible uses for, say, adding dev dependencies like a specific version of phpunit, etc. However, such use cases are clearly out of scope for this ticket.

Yes, they are out of scope, but they are possible today, without any additional changes...

Besides, a home-grown autoloader now does not necessarily preclude using composer down the road if it starts to make more sense later, especially if we're careful about what the integration points look like for 3rd party code.

I would agree if we were talking about a semantically versioned project with regular update cycles. We're talking about WordPress here, though, with a BC philosophy that transforms everything that enters the codebase into technical debt eventually (excuse the overly harsh words here, it is meant to bring a point across, not to offend people).

@schlessera @TJNowell What are your thoughts? you know how much I would love to use composer; but isn't it starting to seem to you that the work spent bending composer into a WP shape would be better spent sculpting WP into something a tiny bit more composer-shaped?

As I have been doing several test versions, and created a custom Composer plugin to meet additional WP needs (https://github.com/staylor/develop.wordpress/pull/1), I still don't really agree with such an assertion. The way I see it, the only "bending" that has been done is to adapt to outdated PHP versions (5.2-compatible class map), broken plugins (case-insensitive class names) and a too rigid file layout (using ABSPATH, which is even optional, as it worked without). And still, Composer just coped with it with half an hour of work.

So, what is this bending and over-complicating all about? Because I sure seem to find the creation of an autoloader design from scratch far more complicated than what we've done so far... The current ticket is just full of arguments that have nothing to do with Composer or not-Composer. A lot of them are just : "yes, but we like it as it was before...".

And once again, I want to clarify, we are talking about using Composer at build-time (which can be completely automated by including it into the grunt pipeline => 0 complexity) and using the generated class-map to be able to start refactoring the code base in hopes of making it more flexible (100s requires out, one require in => 0 complexity). Yes, changing the code base so that it makes better use of the autoloader might be complicated, but that has nothing to do with Composer. It is just a refactoring that is long overdue, and that will be necessary with a custom autoloader as well.

#184 follow-up: @schlessera
8 months ago

As I find it very difficult to assess whether there's even any real interest in having an autoloader, I created a (non-binding !) Twitter poll to get a better overview. I would welcome anyone interested in the topic to leave their vote:

https://twitter.com/schlessera/status/773178479953272834

#185 @iamfriendly
8 months ago

Random plugin aside; If you use FeedWordPress with the current nightly, which includes [38374] then you'll have a bad time, as that plugin directly requires ABSPATH . WPINC . '/class-feed.php' in 2 places.

It's fixable by hacking the plugin (yay) to replace that with

<?php
require_once( ABSPATH . WPINC . '/class-wp-feed-cache.php' );
require_once( ABSPATH . WPINC . '/class-wp-feed-cache-transient.php' );
require_once( ABSPATH . WPINC . '/class-wp-simplepie-file.php' );
require_once( ABSPATH . WPINC . '/class-wp-simplepie-sanitize-kses.php' );

Would have been nice to have a deprecation notice about this, or something along those lines.

#186 @swissspidy
8 months ago

In 38535:

Revert [38401] after [38480].

See #36335.

#187 in reply to: ↑ 184 ; follow-up: @schlessera
7 months ago

Replying to schlessera:

As I find it very difficult to assess whether there's even any real interest in having an autoloader, I created a (non-binding !) Twitter poll to get a better overview. I would welcome anyone interested in the topic to leave their vote:
https://twitter.com/schlessera/status/773178479953272834

The poll was running for a whole week and we now have the final voting results:

https://www.alainschlesser.com/wp-content/uploads/2016/09/Screenshot-2016-09-14-20.55.22.png

As the poll was meant to get reactions from developers, I think that participation was not too bad:

https://www.alainschlesser.com/wp-content/uploads/2016/09/Screenshot-2016-09-14-21.03.21.png

Assuming that the result is at least _somewhat_ representative, it let's us conclude the following:

  1. There's a general agreement that an autoloader is needed within Core and the the time is right.
  2. The majority prefers the Composer-provided autoloader over a custom-built one.

It was decided to continue evaluating an implementation as a feature project, but I still wanted to add the results of this poll to the ticket for later reference.

#188 in reply to: ↑ 179 @MikeSchinkel
7 months ago

Replying to GaryJ:

FWIW, I would disagree about the classes naming of those directories, as it semantically excludes the potential future use of interface and traits, without having to add other directories to map to.

Sure. Like I said, it was a strawman proposal. I choose classes simply because wp-includes/includes felt a bit redundant. What would you propose as an alternate?

Replying to tfrommen:

In addition, there still would be classes that do not live in that folder. What would you propose as an alternate?

Absolute purity it not really a requirement.

That said, the name of the directory is not at all critical to the proposal.

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#189 in reply to: ↑ 180 @MikeSchinkel
7 months ago

Replying to JohnPBloch:

composer/installers handles putting plugins into wp-content/plugins, themes into wp-content/themes, and mu-plugins into wp-content/mu-plugins, or any location one wishes to configure in the root composer.json. Any package may define its type as wordpress-plugin|theme|mu-plugin and it will get installed in the correct location and all autoloading will simply work as expected.

That is correct, except getting the configuration correct in composer.json is far from trivial, especially when you want some plugins to be in /plugins/ and others to be in /mu-plugins/.

#190 in reply to: ↑ 182 ; follow-up: @MikeSchinkel
7 months ago

Replying to chrisvanpatten:

If the goal is to ... make WordPress more consistent with generally accepted PHP best practices

Is that an actual explicitly stated goal of the WordPress project?

WordPress best practices and some of PHP best practices are often at odds with one-another. Trying to force all of PHP best practices on WordPress will ultimately do more harm to WordPress than benefit it. Especially related to integrating Composer into core. JMTCW.

So before we power ahead trying to make WordPress follow PHP best practices when they are obviously at odds with the WordPress way we should get the core team to explicitly embrace that direction, and a good place to do it would be here:

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#191 in reply to: ↑ 187 ; follow-up: @MikeSchinkel
7 months ago

Replying to schlessera:

Replying to schlessera:

As I find it very difficult to assess whether there's even any real interest in having an autoloader, I created a (non-binding !) Twitter poll to get a better overview. I would welcome anyone interested in the topic to leave their vote:
https://twitter.com/schlessera/status/773178479953272834

Wow, what an ingenious way to beg the question.

I am sorry, but running a poll on your Twitter feed looking to validate interest in using Composer in WordPress core is as likely to be biased and has about as much statistical validity as running a poll during a Donald Trump rally asking who would be the USA's next best POTUS!

If a poll is really needed it should be taken from an appropriate sample, not from your Twitter followers. Running the poll on wordpress.slack.com#core would be one way to address it, but that would probably also be heavily biased to core developers and not WordPress developers at large.

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#192 in reply to: ↑ 191 @schlessera
7 months ago

Replying to MikeSchinkel:

Replying to schlessera:

Replying to schlessera:

As I find it very difficult to assess whether there's even any real interest in having an autoloader, I created a (non-binding !) Twitter poll to get a better overview. I would welcome anyone interested in the topic to leave their vote:
https://twitter.com/schlessera/status/773178479953272834

Wow, what an ingenious way to beg the question.

My intention was not to beg the question here. Although I enjoy our current back-and-forth discussions here on Trac (as any discussion just helps to more clearly define one's own arguments), I'm a bit fed up by the fact that a handful of people are duking it out in a subject that can clearly not provide a right or wrong answer, but rather needs as much feedback as possible to assess what the "majority" thinks. And, to be honest, had the Twitter poll shown that there's just no interest in having an autoloader right now (Composer or not), I would just have thrown the towel and invested my time into something more fruitful.

I'll just ignore your comment on the current political situation here and address the more general question of bias and representativity...

First of all, as the stated goal of the WordPress project is to "democratize publishing" ( http://wordpressfoundation.org/ ), it is very sad to recognize that it is a democracy without a voting system. The basic requirements of a democracy are not met, and the whole project just takes over the meritocracy it inherits from its open source roots.

I don't want to debate about what system is better here to manage the project, as I don't think a democracy always yields the better results (see: World). However, the project should stand by whatever system it wants to use and make it work in an effective manner.

Right now, my impression is that any more uncomfortable discussions are often dismissed with the "majority" or "80/20" arguments (pointing to a democracy), but there's no way to assert what the majority is or wants. There are some access stats that probably account for more bot nets and inactive sites than real users, and that's it. When it comes to issues where the end users should not even be concerned, it is just strongly held opinions and historical reasons.

So, if the WordPress project wants to democratize something, it should provide the basic democratic tools that would allow us to truly evaluate what the majority really wants. If it prefers to be run as a meritocracy, I'm all fine with that. But then, drop the "majority" argument, as it is just something that can't be argued or reasoned about. As soon as someone is perceived to have that argument on his/her side, all other reasonable arguments are effectively vetoed.

As WordPress does not have any voting system in place, a Twitter poll was just the best I could come up with in the short term. I'm very open to any other suggestions on how to better handle this, as I am genuinely interested in the data. I want to plan my time investment based on that, so what would be the point of wanting skewed results?

#193 in reply to: ↑ 190 @schlessera
7 months ago

Replying to MikeSchinkel:

Replying to chrisvanpatten:

If the goal is to ... make WordPress more consistent with generally accepted PHP best practices

After 7 years of working with WordPress professionally (and numerous programming languages before WordPress) I have come to the conclusion that WordPress best practices and PHP best practices are often at odds with one-another.

I don't think there's such things as WordPress best practices and PHP best practices, I guess you're thinking about the WordPress Coding Standards here, which are mostly about style. You don't get to choose "your own personal best practices", they are an industry consensus, with the industries being web development and/or the more general software development.

So, there's "The WordPress Way" and there's industry best practices, and yes, these two are sometimes at odds. The question then is: "Are we forced/required to adhere to industry best practices, or is it preferable to keep our own ways?"

No one forces you to embrace best practices, but it is generally in your own best interest. The best practices are derived from experience that has come from projects that have the breadth, depth and scope to find out what impact initial design choices will ultimately end up having. They allow you to avoid making costly mistakes because of design choices that initially seem to be a good solution, and end up being the contrary.

What kinds of best practices are we touching upon here, when we are talking about "Using Composer to build an autoloader vs building a custom one"? Note: I'm just improvising to come up with a list here, and I don't really know where to find a meaningful "list of software best practices" without investing days of work. Let me know if I missed something obvious.

  1. Use mature, battle-tested libraries
  2. Use standards
  3. Don't reinvent the wheel
  4. Aim for interoperability
  5. Keep the number of external dependencies low
  6. Avoid tight coupling to a framework

Comparing Composer autoloader vs custom autoloader would result in something like this:

 Composer --- Custom
     +     1.    -
     +     2.    -
     +     3.    -
     +     4.    -
     -     5.    +
     +     6.    -

This simplified enumeration is of course non-sense. But I wanted to illustrate what the types of "best practices" are that we are talking about here. This is not something like "spaces vs tabs"...

#194 follow-ups: @TJNowell
7 months ago

While I would prefer a composer autoloader, I think that there are points that need addressing that have been raised by others:

  • What performance improvements can autoloading provide?
  • What problem does this solve?

So far, Mike is the main opponent of a Composer based autoloader, but if I read correctly he has no opposition to an autoloader, just the Composer variety. This means there is still work that can be done without addressing that.

Mike has indicated he wanted to work on an autoloader proof of concept but didn't have time in that week. Now that time has passed, I'm curious to see what he's devised. We also have the composer autoloader, and I think both can be used to test and measure performance improvements so that autoloading is an easier sell for those still sceptical

#195 in reply to: ↑ 194 @Cybr
7 months ago

Replying to TJNowell:

  • What performance improvements can autoloading provide?
  • What problem does this solve?

Performance improvements:

  1. None when every file is required. It's actually about twice as slow, but we're talking microseconds here.
  2. A lot when only some files are required. When WordPress only requires files and classes it needs, the memory overhead is much lower. Requiring/including files is also the about the slowest thing within PHP.

Problem(s) it solves:

  1. Developers can freely use any class within WordPress without being afraid to crash the system for when the class has been called "too early".
  2. Memory issues are pretty much gone. For example, on a sitemap, I do not require to load the loop.

Problem(s) it creates:

  1. The known action order is gone. This might not seem like a big deal, but with some things I had to face the issue of a JIT action/filter injection. These JIT injection things might break as some actions are spread over multiple files and classes.

I'm very much against using Composer.
It creates another dependency to which Core needs to fix bugs against.
Updating it also creates issues. For example, why are we still stuck using jQuery 1.x instead of using the much faster 2.x or even 3.x?
Answer: WordPress loves backwards compatibility (a little too much).

#196 in reply to: ↑ 194 @schlessera
7 months ago

Replying to TJNowell:

While I would prefer a composer autoloader, I think that there are points that need addressing that have been raised by others:

  • What performance improvements can autoloading provide?

An autoloader comes with an inherent overhead, and when you load 100% of your classes, the autoloader will be slower than direct requires. When a code base is optimized for autoloading, it will most certainly run faster and consume less memory.

In a very simple performance test (http://blog.ircmaxell.com/2012/07/is-autoloading-good-solution.html), Anthony Ferrara found out that, for empty classes, the autoloader is faster once you're using 75% or less of the classes. The heavier your classes are (think WP_Query), the higher that percentage is. For the WP code, not loading a handful of classes will probably have the autoloader pull ahead in terms of performance.

  • What problem does this solve?
  • Only load classes when effectively needed (saves processing time and memory).
  • Loading order is irrelevant (makes some procedural logic unnecessary).
  • You don't need knowledge about the filesystem layout (of your code, as well as of your dependencies).
  • As such, you are free to reorganize your file layout as you see fit without breaking the code.

So far, Mike is the main opponent of a Composer based autoloader, but if I read correctly he has no opposition to an autoloader, just the Composer variety. This means there is still work that can be done without addressing that.

Mike has indicated he wanted to work on an autoloader proof of concept but didn't have time in that week. Now that time has passed, I'm curious to see what he's devised. We also have the composer autoloader, and I think both can be used to test and measure performance improvements so that autoloading is an easier sell for those still sceptical

As we don't have an autoloader yet, the code base is not optimized for the use of an autoloader either. This makes performance benchmarks completely pointless, as the result is probably something along the lines of "performance without autoloader + autoloader overhead". The end result we're after is not to have an autoloader be included, but to have a code base that makes proper use of an autoloader. That is what would need to be benchmarked, not "old code + autoloader class".

#197 @steveglick
7 months ago

As there's been a lot of discussion around performance benefits, I'm wondering if anyone has done benchmarking on this? Could this autoloader eliminate the need for building our own ajax handlers if we want decent performance on ajax heavy pages? ( See https://wp-dreams.com/articles/2014/03/better-ajax-handler-for-wordpress-super-fast-ajax/ )

If this is a functional patch for 4.6 I could do some testing and run some benchmarks.

#198 @steveglick
7 months ago

I should have read the last comment...

As we don't have an autoloader yet, the code base is not optimized for the use of an autoloader either. This makes performance benchmarks completely pointless, as the result is probably something along the lines of "performance without autoloader + autoloader overhead". The end result we're after is not to have an autoloader be included, but to have a code base that makes proper use of an autoloader. That is what would need to be benchmarked, not "old code + autoloader class".

I was confused why everyone is talking about performance and not benchmarking anything, but this makes sense now.

#199 @TJNowell
7 months ago

I would think we should be using register_rest_route rather than WP AJAX, measurements suggest a 15% performance boost there.

Can we make a demonstrative optimisation that we can benchmark to prove these aren't just theoretical improvements? The moment we can say this is a 2% or a 5% improvement this becomes an obvious sell. At the moment we're relying on best practices, personal experience and theory, but we have nothing hard and concrete. Identifying an easy win to demonstrate practically would be a big win

@MikeSchinkel
7 months ago

Refactoring of WordPress core to enable autoloading of classes + a simple classmap generator and classmap autoloader.

#200 @MikeSchinkel
7 months ago

Replying to TJNowell:

Mike has indicated he wanted to work on an autoloader proof of concept but didn't have time in that week. Now that time has passed, I'm curious to see what he's devised.

Sorry, life got in the way (clients, WordCamp, etc. etc.)

I have implemented the autoloader (which took ~10 minutes), reworked the classmap generator (~30 minutes) and then refactored WordPress core so that as many classes could be autoloaded as reasonably possible (and that took over 8 hours.)

See attached patch 36335-autoloader.diff which contains all the code mentioned.

In order to get this actually done and working I took some liberties regarding and source files that had more than one class; I split them apart and created a new file for each of them.

Given the work I've done on WPLib I've learned that working with an autoloader ends up being easier if the files names actually match the class names as opposed using a class- prefer with dashes and all lowercase; i.e. WP_Query.php instead of class-wp-query.php. The main benefit is that you can do a directory scan and get the class names without requiring any transform. So I renamed all of the class files to use the same name as the class.

I also created two (2) /autoload/ directories: /wp-includes/autoload and /wp-admin/autoload. I created subdirectories inside these autoload directories to organize the code better, e.g. /wp-includes/autoload/filesystem and /wp-admin/customize/, for example.

In addition I created two (2) /constants/ directories: /wp-includes/constants and /wp-admin/constants which I used to move any constants that were defined in class files. I load these constants in /wp-includes/constants/default-constants.php and /wp-admin/constants/include/admin.php, respectively.

As for performance, I do not have any benchmarks set up but I can honestly say it feels extremely snappy. I would love it if someone who is good at benchmarks could try their hand and benchmarking this.

Finally, this is a proof-of-concept for people to actually see what I have been proposing. I have tested it in a cursory manner but I do not have WordPress' full test suite set up so it is highly likely to be broken in more than a few places.

Still, there is a lot of work in here to separate out the classes to get them into a form that can be autoloaded, even if we did ultimately choose a Composer autoloader, but this patch will become stale pretty quickly so if we want to move forward with this we would be best to make a decision sooner than later otherwise most of the work I did will have to be redone again on a future version of WordPress.

P.S. I have not yet implemented automatic generation of the classmap on a shutdown hook as I mentioned in previous comments on this ticket. I have used that approach with WPLib and it works extremely well in combination with a constant that you would only define during development, e.g. WP_GENERATE_CLASSMAP.

But it might not be the best approach for core since the classmap generator could easily be run during a build step but it could be useful for those who would like to be able to autoload any classes in themes or plugins that are deployed via version control.

It won't take long to add that code, but I'll wait to see if this approach gets traction before moving forward with this.

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#201 @MikeSchinkel
7 months ago

Since the patch is too large for Trac to display I created a Git repo containing all the code to make it easier to evaluate. The simplest way to quickly evaluate is to:

  1. Clone WPLib Box,
  2. Delete the www directory inside the cloned repo,
  3. Clone Autoloading WordPress to replace www,
  4. Run vagrant up and
  5. Browse to http://wplib.box.

You can have it up and running in less than 10 minutes this way (assuming you already have VirtualBox and Vagrant installed.)

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#202 follow-ups: @TJNowell
7 months ago

I'm not a fan of moving the files at this stage, that can be done once their location is irrelevant and it might break existing code that includes certain files directly

Generating the classmap should also be a build step, adding constants for runtime seems counterproductive.

I'm also concerned that the generation code relies on global variables and isn't very testable, it wouldn't be possible to write an easy unit test. I'm also concerned by the new top level files, we should keep the root clean and adding new top level files is going to garner some flack, I would move those to wp-includes

Regarding performance, even crude benchmarks would be helpful. Without optimisations an autoloader would make WP slower due to the larger number of files, so we need to be clear about what performance improvements are possible and which ones have been made, as well as some preliminary benchmarks so that they can be replicated and expanded upon.

Thanks for taking the time Mike, I appreciate the effort and it's good to see progress

#203 in reply to: ↑ 202 @MikeSchinkel
7 months ago

Replying to TJNowell:

I'm not a fan of moving the files at this stage,

How are we going to validate, test and benchmark without doing so?

and it might break existing code that includes certain files directly

IMO that is why we would run it through the WordPress test suite.

Generating the classmap should also be a build step, adding constants for runtime seems counterproductive.

Agreed on the first part, the second part would not be for core but instead for website projects for people who do not use build tools. IOW, 98% of people who set up WordPress sites. :-D

Currently the classmap gets generated from classes in /wp-admin/ and /wp-includes/ and stores the classmap as /wp-classmap.php.

For the generation for those in the wild it could generate a user class map for classes in /wp-content/ and store the result in /wp-content/classmap.php.

I'm also concerned that the generation code relies on global variables and isn't very testable, it wouldn't be possible to write an easy unit test.

The classmap generator itself is proof-of-concept written as quickly as possible. Please don't get tripped up and assume the proof-of-concept is meant to be production ready code.

That said, the classmap generator is really simple so refactoring it to remove globals and make it testable would not be a big job.

I'm also concerned by the new top level files, we should keep the root clean and adding new top level files is going to garner some flack, I would move those to wp-includes

Yeah, nothing about this proof-of-concept requires top-level files, that was just the arbitrary choice because it seemed the most logical given it represented files from both /wp-admin/ and /wp-includes/.

Regarding performance, even crude benchmarks would be helpful.

Definitely.

Without optimisations an autoloader would make WP slower due to the larger number of files...

Honestly, my seat-of-the-pants for loading pages in the admin was noticeably faster. And given the architecture I chose there is very little overhead in the autoloader so if it loads 1/2 as many files that would be why.

But I know perceptions are subjective so I am really anxious for some real benchmarks. I have never implemented benchmarks for a WordPress site before and don't really have the time to research how to do it so I am hoping someone else can step up and try this.

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#204 @MikeSchinkel
7 months ago

Following up since it has been 4 days since last post and no comments on what was previously a very active ticket (and it is Friday evening so I have time to!)

Thus far the big debate has been over "use a composer autoloader" vs. "use an autoloader optimized for WP" and that has been contentious. So let me suggest a different first step that I think that maybe all of us will see the need for (or at least all of us that would like to see an autoloader built in to WP core.)

Proposal: First discuss making core files autoloadable, and then divide and conquer to analyze the core files in need of changes? Some files will be very easy to make autoloadable whereas others will be require more finesse. Because without making core files autoloadable the "which autoloader" question is moot. Why not go ahead and prepare patches to apply that are easy to apply and that we know will not break anything?

  1. If we use a classmap autoloader we can have great flexibility in how we organize files. In my prior patch I created two locations for autoload files: wp-admin/autoload and wp-includes/autoload, for hopefully obvious reasons. However if we want to plan for the potential to use a Composer autoloader so that we can move this ticket forward then we will need to have one root location for autoload files. This could be wp-autoload but based on some prior related comments on this thread I think some people might object to a new directory in the root? Putting it in wp-includes/autoload and then moving all files from within wp-admin that need to be autoloaded just feels wrong. So, can we discuss this point: What should the autoloader's root directory be? In the interim I am going to use wp-autoloader but that will be trivially easy to change if we so desire. (if we do not need to leave the option open to use a Composer autoloader then we can easily just use wp-admin/autoload and wp-includes/autoload.)
  1. No matter which autoloader we ultimately choose the autoloader must derive the file name from the class name, so I propose we change the file naming convention for autoloaded classes to be their classname, e.g. wp-autoloader/WP_Query.php vs. the current wp-includes/query.php. Can we get a thumbs-up or a thumbs-down for this approach? And if a thumbs-down it will help if you provide a detailed reason why not along with a better alternate proposal including the benefits for the alternate approach?

I'll continue with details related to dividing an conquering on another ticket so these two questions do not get lost.

#205 @MikeSchinkel
7 months ago

Taking a look at the files in WordPress core 4.6.1 I found many different ways to group them that will help us decide how to move forward with patches:

Admin vs. includes

  1. Classes declared in files found within /wp-admin
  2. Classes declared in files found within /wp-includes

Ideally for classes found in those directories we would like to keep them within those directories, but if we need to anticipate an autoloader that will some day not use a classmap it will be more performant to have one root directory for files.

So this distinction of admin vs. includes will not really matter unless we assume we will always use a classmap. If we can always assume a classmap then we could leave the files within the same top level directories where they are currently found. But since so many people here want to have the option of moving away from a classmap I will assume we will have one autoloader root combining files from both of these sources.

BTW, if we ever do have an autoloader that does not use a classmap the is_file() calls used to validate if a file exits before an attempt to load it is made can add up and can be a significant performance hit. Or at least that was made clear to me when several of our projects were code-reviewed by 10up for an enterprise client. Their code sniffer, which I understand was the same code sniffer used for WordPress VIP flagged all calls to is_file() as errors and would not allow us to use them if not cached in persistent cache.

The above is one reason I have been so adamant that our autoloader needs to be classmap based. But the patches I am currently working on to make WordPress core classes autoloadable will not require a classmap so there is no reason to halt progress waiting on a decision about this.

#206 @MikeSchinkel
7 months ago

One Class Per File vs. Multiple Classes per File

Next we consider classes that are contained in a file by themselves vs. files that contain multiple classes.

There appear to be 57 classes that are each contained in a file that contains at least one other class. These are all in the wp-includes directory. Most of these come from externally included projects such as:

  1. IXR - The Incutio XML-RPC Library
  2. Text/Diff - General API for generating and formatting diffs
  3. pomo - Translations
  4. SimplePie - RSS and Atom feed parsing
  5. ID3 - Extracts information from MP3s & other multimedia files
  6. MagpieRSS - RSS and Atom parser (deprecated in WP)
  7. Services_JSON - Converts to and from JSON format
  8. AtomLib - Atom Syndication Format PHP Library
  9. PHPMailer - PHP email creation and transport class

Some of these include constant declarations inside the class files (ugh!) including MagpieRSS, Test/Diff, SimplePie, ID3, and pomo. That means to autoload these we'd have to extract the defines and decide where and when to include them (my huge patch from a last week moved them into /wp-includes/constants and /wp-admin/contstants and then loaded them from /wp-settings.php and ./wp-admin/includes/admin/php, respectively.)

Some of these would be easy to break out since they do not perform any require()/require_once() or include()/include_once and are not loaded with dynamic name generation, such as PHPMailer, AtomLib, and Services_JSON, but since they came from external code I don't know what the core team's opinion is on restructuring their code?

Other files containing classes would be really easy to break out but since I analyzed WP 4.6.1 as I write this I am discovering that someone already has done this in trunk. So no need for a patch for these as I had planned!

As for the ones above that would not be easy I expect we will tackle them last, or maybe not at all, all depending on what the core team decides.

#207 follow-ups: @schlessera
7 months ago

@MikeSchinkel : I think you're completely derailing this ticket. If you want to discuss changing the WordPress Coding Style, or the WordPress folder layout, please do so in a new ticket.

Reading through the last handful of comments, it seems like I still wasn't able to fully communicate what Composer brings to the table.

When going with Composer, we don't buy into the Composer Autoloader, as the standard Composer Autoloader is unusable for WordPress. What we want is the Composer Autoloader Generator. We want to add another tool to the build-time chain that let's us parse our current code base and create an autoloader adapted to our needs. The Composer Autoloader Generator parses the actual PHP in the files to discover the classes. It doesn't care about the filenames, it manages several classes in one file, etc...

The resulting autoloader that is built from that parse step can be completely customized to whatever we think is best. We can include any special WP cases, we can optimizie how we want, etc...

So, when you state above:

but this patch will become stale pretty quickly so if we want to move forward with this we would be best to make a decision sooner than later otherwise most of the work I did will have to be redone again on a future version of WordPress.

...this is the exact reason why I initially proposed to use Composer as a build-time tool in the first place. We will not optimize the WordPress Core in one or two releases, and I'm surprised you would think such a thing to be feasible.

The goal with using Composer Autoloader Generator as a build-time tool was always to just have a very quick, first step to make autoloading even possible, to get to the real problem: needing to shuffle classes, files and folders around for several releases to optimize the Core for autoloading. In the meantime, with every change, a simple re-run of the Grunt build will fix whatever you broke in the autoloader. And I've said it before: the actual autoloader that is generated by Composer can be freely adapted.

So, that being said, I would be relieved if we could stop talking about how optimized a specific autoloader implementation is, as that is completely besides the point at this time. Also, we don't need to rename or move files at this point, Composer will just take care of it. The only thing that needs to be done is to remove global functions from files we want to have autoloaded, which has already ben done in the initial patch by @wonderboymusic and can currently be seen in the autoloader branch on https://github.com/staylor/develop.wordpress/tree/autoloader.

@MikeSchinkel the way I see it, the solution you propose:

  • eliminates the advantage that initially brought us to Composer (hence your need to rename files and move stuff around),
  • eliminates all other advantages that Composer would add in the future,
  • only seemingly adds one perceived advantage of its own (tightly optimized custom autoloader), which is not really an advantage, because you can generate this exact same autoloader with Composer Autoloader Generator.

Unless I'm missing something crucial, I don't see in what ways this solution would be preferable.

@TJNowell If anyone wants to play around with benchmarks, please go ahead. I personally will not waste time with benchmarks right now. If the goal is to see what autoloader implementation is faster, then this has nothing to do with Composer/no-Composer. And if the goal is to see whether an autoloader even makes sense in the first place, then I'm baffled, and must admit that we will never find out, because first of all the benefits are not only about performance, and secondly, we would need to implement it and optimize Core for it first to get proof. So, in essence we're trying to benchmark the chicken and egg problem.

Last edited 7 months ago by schlessera (previous) (diff)

#208 in reply to: ↑ 207 @MikeSchinkel
7 months ago

Replying to schlessera:

@MikeSchinkel : I think you're completely derailing this ticket. If you want to discuss changing the WordPress Coding Style, or the WordPress folder layout, please do so in a new ticket.
Reading through the last handful of comments, it seems like I still wasn't able to fully communicate what Composer brings to the table.

Please re-read comment #204 where I said (with added bold):

Thus far the big debate has been over "use a composer autoloader" vs. "use an autoloader optimized for WP" and that has been contentious. So let me suggest a different first step that I think that maybe all of us will see the need for (or at least all of us that would like to see an autoloader built in to WP core.)

Proposal: First discuss making core files autoloadable, and then divide and conquer to analyze the core files in need of changes? Some files will be very easy to make autoloadable whereas others will be require more finesse. Because without making core files autoloadable the "which autoloader" question is moot. Why not go ahead and prepare patches to apply that are easy to apply and that we know will not break anything?

My latest tickets have tried to sidestep the debate and work on things we'd need with or without a Composer autoloader. So how is that derailing the ticket?!?

#209 follow-up: @schlessera
7 months ago

@MikeSchinkel: I did not say that what you are discussing is not necessary. But if we now "side-step" the main question and start to discuss implementation details, file naming conventions, folder layout, etc... we will just end up with a "spaces-vs-tabs" debate and this will go nowhere.

I have my own ideas about what layout and naming schemes would make sense for WordPress. But this ticket is complicated enough as it is, we don't need to add more factors.

#210 in reply to: ↑ 207 ; follow-up: @MikeSchinkel
7 months ago

Replying to schlessera:

The Composer Autoloader Generator parses the actual PHP in the files to discover the classes. It doesn't care about the filenames, it manages several classes in one file, etc...

Generating classmaps is trivial. My ~50 line classmap generator proof-of-concept that was included in my huge patch illustrates this.

So we can please refrain from using "Composer can generate a classmap" as a reason to use Compser and instead move on to focus on any other potential benefits Composer can provide that I am evidently missing?

Wait. Strike that. Can we just table Composer-or-Not for now and work on moving anything else can we move foward?

The resulting autoloader that is built from that parse step can be completely customized to whatever we think is best. We can include any special WP cases, we can optimizie how we want, etc...

But not without having to using is_file() a lot. Or are you suggesting that we go ahead and accept that it will always be classmap based?

...this is the exact reason why I initially proposed to use Composer as a build-time tool in the first place.

My statement about staleness is now moot since nobody has taken any action on it. So I have decide to move forward with a much more incremental approach, hence my numerous comments and additional planned comments today.

Instead I plan on offering patches that either won't be stale or will be generated via a shell script.

We will not optimize the WordPress Core in one or two releases, and I'm surprised you would think such a thing to be feasible.

And I am surprised that you think it is such an overwhelming job. Sure, to get it to 100% will take a while. But based on the analysis I have been doing I see no reason why we can't get to 80% in short order. The only thing holding us back is will and needless bikeshedding.

The goal with using Composer Autoloader Generator as a build-time tool was always to just have a very quick, first step to make autoloading even possible, to get to the real problem: needing to shuffle classes, files and folders around for several releases to optimize the Core for autoloading.

So that seems to me to be a very small benefit for all the intensity of debate. Generating a classmap is almost trivial, as I have demonstrated.

In the meantime, with every change, a simple re-run of the Grunt build will fix whatever you broke in the autoloader. And I've said it before: the actual autoloader that is generated by Composer can be freely adapted.

But the autoloader generated by Composer will need to be modified as as Ryan McCue points out. Thus we really get no "it just works" benefit from using the Composer autoloader and we get the overhead that it generates too.

So, that being said, I would be relieved if we could stop talking about how optimized a specific autoloader implementation is, as that is completely besides the point at this time. Also, we don't need to rename or move files at this point, Composer will just take care of it.

Then why don't we just debate the specific pros and cons of naming conventions -- as I was proposing we debate -- instead of getting back on the Composer soapbox right now?

One of the benefits of having files named for their class is that it makes it performant to load the classmap dynamically, as would likely be the case for files in /wp-content/. Being able to scan a directory and grab the filenames as classnames means their is no potential breaking ambiguity in the conversion of the classname.

That said, I don't consider the file naming part critical, just a nice-to-have I would prefer that we adopt.

What I do consider critical is that we stop having circular debates and move things forward by finding things we can work on that do not require resolving the Composer debate, and that get us closer to seeing an autoloader included, no matter which autoloader it is.

https://github.com/staylor/develop.wordpress/tree/autoloader

This is news to me, thanks.

  • eliminates the advantage that initially brought us to Composer (hence your need to rename files and move stuff around),

I think you keep saying this has an advantage but from my vista is seems all those proposed advantages have turned out to not be real advantages. If the main benefit is to save time then in the time we've debated this we could do that work you are trying to bypass three times over, and do a much better job.

Why debate a one-time process at the level of a fundamental architecture decision?

  • eliminates all other advantages that Composer would add in the future,

Which is why I said let up bypass this Composer-or-Not and instead work on making core classes autoloadable.

But since you bring it up again, as Ryan said I think this is a solution looking for a problem. So please detail your expected advantanges as they related to WordPress. This far all I have read is that Composer's advantages are "because it is standard" and "using is is a best practice", but I have not read about any specific tangible benefits that using Composer with WordPress core would provide WordPress.

No, wait. That will only delay moving things forward. But please do give those explanation later when the Composer-or-Not decision is back on the table.

  • only seemingly adds one perceived advantage of its own (tightly optimized custom autoloader), which is not really an advantage, because you can generate this exact same autoloader with Composer Autoloader Generator.

But even if we could, why work with a tool that writes code -- a tool that you have to work to tweak -- when the code is trivial to write by hand?

As an aside, you cannot generate this exact same autoloader with Composer. But I digress.

Unless I'm missing something crucial, and don't see in what ways this solution would be preferable.

And I don't see how Composer is preferable either, so let us first focus on what we can move forward, okay?

#211 in reply to: ↑ 209 @MikeSchinkel
7 months ago

Replying to schlessera:

But if we ... start to discuss implementation details, file naming conventions, folder layout, etc... we will just end up with a "spaces-vs-tabs" debate and this will go nowhere.

Will we really? My opinions will be driven by the factual benefits of any given approach. Hopefully others will do the same. Or said another way, I don't really care about what they are as long as they empower autoloading and ideally dynamic discovery.

That said, the file naming conventions, folder layout are not super important other than that we have something so that we can refactor the existing classes to make them autoloadable. We can always change the file naming conventions folder layouts before code is shipped.

I have my own ideas about what layout and naming schemes would make sense for WordPress. But this ticket is complicated enough as it is, we don't need to add more factors.

Then please tell me what your proposal is to move things forward to address the things we all can agree on?

Without a viable alternative or condemnation from the core contributors here the best thing to do is move forward and start creating patches because actual code is more actionable than circular debates.

#212 follow-ups: @wonderboymusic
7 months ago

move any discussion to PRs over here - here is the present code diff:
https://github.com/aaronjorbin/develop.wordpress/compare/master...staylor:autoloader

If you want to contribute code, send a PR with a novella of opinions. this ticket is losing its usefulness.

#213 in reply to: ↑ 212 @MikeSchinkel
7 months ago

Replying to wonderboymusic:

move any discussion to PRs over here - here is the present code diff:
https://github.com/aaronjorbin/develop.wordpress/compare/master...staylor:autoloader

If you want to contribute code, send a PR with a novella of opinions. this ticket is losing its usefulness.

Perfect.

To clarify; to which should we submit PRs to?

And thanks.

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#215 in reply to: ↑ 210 @schlessera
7 months ago

Replying to MikeSchinkel:

Replying to schlessera:
Generating classmaps is trivial. My ~50 line classmap generator proof-of-concept that was included in my huge patch illustrates this.
So we can please refrain from using "Composer can generate a classmap" as a reason to use Compser and instead move on to focus on any other potential benefits Composer can provide that I am evidently missing?

Well, you went ahead and renamed all of the class files just to make this possible (or better: to make coding your generator trivial).

But not without having to using is_file() a lot. Or are you suggesting that we go ahead and accept that it will always be classmap based?

You can use whatever you want. The generated autoloader "is one or more arbitrary PHP file(s) that we let it generate". Whether you want to have is_file() or not, whether you want to have unicorn ASCIIs in it, all possible!

My statement about staleness is now moot since nobody has taken any action on it. So I have decide to move forward with a much more incremental approach, hence my numerous comments and additional planned comments today.

Yes, exactly, but that is a very obvious and logical result. We are talking about a major paradigm shift here, this won't be done in a handful of days...

So that seems to me to be a very small benefit for all the intensity of debate. Generating a classmap is almost trivial, as I have demonstrated.

No, you demonstrated the contrary. You needed to refactor the entire codebase to get your generation working and your update went stale and useless in a matter of days.

But the autoloader generated by Composer will need to be modified as as Ryan McCue points out. Thus we really get no "it just works" benefit from using the Composer autoloader and we get the overhead that it generates too.

No, we modify the generator to generate the exact autoloader we need. That work is already done, btw (or at least the parts we could already agree on): https://github.com/staylor/develop.wordpress/pull/1

Then why don't we just debate the specific pros and cons of naming conventions -- as I was proposing we debate -- instead of getting back on the Composer soapbox right now?

Because the ticket is about the autoloader, not about the coding standards.

One of the benefits of having files named for their class is that it makes it performant to load the classmap dynamically, as would likely be the case for files in /wp-content/. Being able to scan a directory and grab the filenames as classnames means their is no potential breaking ambiguity in the conversion of the classname.
That said, I don't consider the file naming part critical, just a nice-to-have I would prefer that we adopt.

Yes, totally agree, but completely irrelevant and unnecessary right now. We can generate whatever autoloader we need.

What I do consider critical is that we stop having circular debates and move things forward by finding things we can work on that do not require resolving the Composer debate, and that get us closer to seeing an autoloader included, no matter which autoloader it is.

We were already moving forward, and most of the work is done already in the GitHub repo for the feature project: https://github.com/staylor/develop.wordpress/tree/autoloader . People agreed that it is too early to use Composer as a dependency manager, but they also agreed to use it as a build-time tool to simplify building an autoloader and keeping everything in sync. This is done in a feature project.

I think you keep saying this has an advantage but from my vista is seems all those proposed advantages have turned out to not be real advantages. If the main benefit is to save time then in the time we've debated this we could do that work you are trying to bypass three times over, and do a much better job.

We are actively working on this, in the GitHub repo. This was decided in a Slack meeting.

But even if we could, why work with a tool that writes code -- a tool that you have to work to tweak -- when the code is trivial to write by hand?

Because you end up with a stale autoloader that does not notice that classes have changed.

As an aside, you cannot generate this exact same autoloader with Composer. But I digress.

Yes, I can. Composer passes a list of classes, you then run it through whatever you want to generate a PHP file. Heck, you could even generate an animated GIF of your classes if you so want...

And I don't see how Composer is preferable either, so let us first focus on what we can move forward, okay?

Yes, I'll just stop discussing here and continue working on the feature project. If you still want to try persuading people to move to something else, I'm fine with it, but I will (try to) not proceed further with this specific discussion.

#216 in reply to: ↑ 212 @MikeSchinkel
7 months ago

Replying to wonderboymusic:

move any discussion to PRs over here - here is the present code diff:
https://github.com/aaronjorbin/develop.wordpress/compare/master...staylor:autoloader

If you want to contribute code, send a PR with a novella of opinions. this ticket is losing its usefulness.

I would have added an issue to your Git repo but issues are turned off. Sooo...

How would you prefer to see a build script added to Grunt? Can you call a PHP script, or does it need to be written in Javascript? (I have not previously used Grunt as other people on our team handle front-end build.) Any direction on your preferences would be appreciated.

#217 in reply to: ↑ 202 @MikeSchinkel
7 months ago

Replying to TJNowell:

I'm also concerned that the generation code relies on global variables and isn't very testable, it wouldn't be possible to write an easy unit test.

Since you mentioned this, you can see a more production ready version in this PR.

#218 follow-ups: @TJNowell
7 months ago

I believe I've been misinterpreted by both sides regarding performance. It's not wether autoloader X or autoloader Y is faster. It's wether autoloading vs no autoloading is faster.

Right now, with no optimisations autoloading slows down WordPress due to the greater number of files being loaded. It's a small difference but we won't get an autoloader if it slows down core. We need to demonstrate with hard facts that an improvement is possible. Right now all we have is theoretical, and a number of senior developers have already chimed in with that's not good enough.

I'd also like to raise some questions:

  • I don't understand why we need to rearrange most of core, this sounds like a recipe for disaster and failed auto-updates. It screams fragile
  • I don't see why we need to rename our files, the part were we find the classes and their filenames is meant to happen at buildtime, it's just a PHP array 'class' => 'filename', why overcomplicate things?
  • There may be things we don't want to autoload that are always loaded, or too critical to let a plugin override
  • Nobody has ruled out the composer autoloader, discussion on wether it should be used is premature as the case for an autoloader has not been made

As for benchmarks, install Query Monitor and run a vanilla install with and without these changes, and measure 10 common page loads 10 times. That should give crude timings. Better yet use a cli tool

#219 in reply to: ↑ 218 @MikeSchinkel
7 months ago

Replying to TJNowell:

It's not wether autoloader X or autoloader Y is faster. It's wether autoloading vs no autoloading is faster.
We need to demonstrate with hard facts that an improvement is possible.

100% agreed.

I don't see why we need to rename our files, the part were we find the classes and their filenames is meant to happen at buildtime, it's just a PHP array 'class' => 'filename', why overcomplicate things?

Great question. If we use a classmap autoloader there is absolutely no reason we would be required to rearrange files. We can get a working autoloader without rearranging files.

The reason I have been thinking it would be good to rearrange files for the benefits I will mention next but maybe I jumped the gun with this idea. Maybe baby steps are better.

So I fully admit that my interest in renaming them are based on assumed concerns and not valid benchmarks, and I agree we should benchmark it.

So we can discuss after benchmarking my concern was adding to the memory footprint from loading the full classmap. Loading the classmap is probably fine on 90+% of sites, but large traffic sites that can't cache everything might be negatively affected by loading a classmap on every page.

How? We could store a number instead of the actual class paths where the number represents the root for the autoload directories, e.g.:

  • 1 for ABSPATH . 'wp-admin/autoload/' and
  • 2 for ABSPATH . 'wp-includes/autoload/'.

From there we could just:

require "{$root_dir}/{$class_name}";

This might make a tangible difference in memory usage for high traffic sites, but very admittedly it might not. I am(/had?) planned to rework the autoloader and classmap generator so we can benchmark this. Should I (not)?

I don't understand why we need to rearrange most of core, this sounds like a recipe for disaster and failed auto-updates. It screams fragile

  1. I do not understand how this can cause failed updates. I do not mean that I debate you on this, I mean I honestly am not aware of how this might break things. Can you elaborate?
  1. One of the strategies I have been thinking about is for us to leave the files in place for all the classes we move to an autoload directory. Then in those files we change them to all include only one line, a require() that would call _deprecated_file(). That way I think there would be almost zero chance that we could break anything, but please check my logic on this.
  • There may be things we don't want to autoload that are always loaded, or too critical to let a plugin override

And there is no problem here as autoloading is orthogonal to explicit loading. Simply leave the hardcoded require() in place for those files, they will be loaded exactly as before and having those files are in the classmap would have zero effect.

I could even enhance the classmap generator to omit from the classmap any classes you explicitly blacklist, e.g.

// wp-classmap-generator.php

require __DIR__ . '/tools/classmap/class-classmap-generator.php';

$generator = new WP_Classmap_Generator( __DIR__ . '/src' );
$generator->add_files( 'wp-admin' );
$generator->add_files( 'wp-includes' );
$generator->omit_classes( array(
	'WP',
	'WP_Query',
	'WP_Post',
	'WP_Rewrite',
	'wpdb',
));
$classmap = $generator->get_classmap();
file_put_contents( __DIR__ . '/src/wp-classmap.php', $classmap );

Matter of fact, I updated the PR to include a omit_classes() method with the above listed classes omitted.

As for benchmarks, install Query Monitor and run a vanilla install with and without these changes, and measure 10 common page loads 10 times. That should give crude timings. Better yet use a cli tool

Great suggestion. I can't promise I will get these done immediately, but I will definitely tackle them once my priority client work is out of the way.

#220 in reply to: ↑ 218 ; follow-up: @MikeSchinkel
7 months ago

A quick follow up. I did decide to do some benchmarks. I compared the current trunk with the code in the autoloader branch of this repo using Query Monitor with inclusion of my autoloader.

So I compared Trunk to Composer to Custom. Up front I will say setting up a valid test is very challenging. All setups were running in WPLib Box making it likely they would be equal, but the database in all cases was for an initial install database with almost no records as I am not sure what would be a good DB to test with.

That said I am honestly I am not seeing conclusive difference in performance between any of the three options. The problem is the timing numbers jump around all over the place and also seem dependent on how long since the last request of the same page, so without a scripted test any data I generate would likely not be valid.

However, I am seeing a difference in memory usage: Trunk was the worst, Custom was the best, and Composer was almost as good as Custom. Best I can tell Custom used between 5% and 10% less memory, and using Custom is about 1% less memory than with Composer.

The following is the mu-plugin I used to create the timing and then I just ran multiple side-by-side terminal windows running tail -f timing-log.txt so I could easily she the results after each page load:

<?php

add_action( 'shutdown', 'log_qm_stats', 11 );
function log_qm_stats() {
    $data = QM_Collectors::get( 'overview' )->get_data();
    if ( ! empty( $data['time_taken'] ) ) {
        file_put_contents(
            ABSPATH . 'timing-log.txt',
            sprintf(
                "'%s', %s, %s, '%s', '%s'\n",
                defined( 'USE_COMPOSER_AUTOLOADER' ) && USE_COMPOSER_AUTOLOADER
                    ? 'composer'
                    : ( 'wptrunk.dev' === $_SERVER[ 'HTTP_HOST' ]
                        ? 'trunk___'
                        : 'custom__'
                    ),
                $data['memory_usage'],
                $data['time_taken'],
                $_SERVER['REQUEST_URI'],
                isset( $_REQUEST ) ? str_replace( "'", "\\'", http_build_query( $_REQUEST ) ) : 'null'
            ),
            FILE_APPEND
        );
    }
}

That said, my methodology could very easily have been flawed and I would be happy if anyone would like to validate it.

One this is that Query Monitor does not keep track of data for certain Customizer page loads which I had expected might show a difference, but as is I could not verify.

#221 in reply to: ↑ 220 @schlessera
7 months ago

Replying to MikeSchinkel:

That said I am honestly I am not seeing conclusive difference in performance between any of the three options. The problem is the timing numbers jump around all over the place and also seem dependent on how long since the last request of the same page, so without a scripted test any data I generate would likely not be valid.

I suspected so. The issue is that the code is still basically loaded procedurally, only through the additional detour of an autoloader. As long as we don't do the work of actively optimizing for loading on-demand, autoloader benchmarks will be arbitrary and pretty meaningless. The only think you can compare at this point is the actual implementations of the generated autoloaders, which is an exercise in futility, as both can be freely adapted to match each other.

However, I am seeing a difference in memory usage: Trunk was the worst, Custom was the best, and Composer was almost as good as Custom. Best I can tell Custom used between 5% and 10% less memory, and using Custom is about 1% less memory than with Composer.

The difference in memory usage probably comes from the fact that your approach has relative paths in the classmap, and the Composer one has absolute paths in the classmap. Again, this can be changed into whatever we want for both, so that point is moot as well.

A note regarding your comparison: the one currently merged into the feature project GitHub repo has excluded most of the third-party/legacy classes for now: https://github.com/staylor/develop.wordpress/blob/autoloader/src/composer.json#L49-L68

And I would once again make you aware that you're now slowly building, feature-for-feature, a replica of the Composer Generator, but without the unit tests, without the community and maturity, and without the standardized configuration. This last point is very important, as it allows to reconcile several autoloaders into a single coherent, optimized autoloader. I still fail to see what the exact advantage is of reinventing the wheel...

#222 follow-up: @schlessera
7 months ago

@wonderboymusic: Please enable Issues on the Feature Project's GitHub repo so we can have a more structured discussion.

#223 in reply to: ↑ 222 ; follow-up: @MikeSchinkel
7 months ago

Replying to schlessera:

The difference in memory usage probably comes from the fact that your approach has relative paths in the classmap, and the Composer one has absolute paths in the classmap. Again, this can be changed into whatever we want for both, so that point is moot as well.

The key point I was trying to convey is that autoloading has a lower memory footprint than what is currently in core, no matter which autoloader we use. After all, ~1% is within the margin of error.

A note regarding your comparison: the one currently merged into the feature project GitHub repo has excluded most of the third-party/legacy classes for now: https://github.com/staylor/develop.wordpress/blob/autoloader/src/composer.json#L49-L68

Similarly we can optimize the custom autoloader by including these classes to exclude here or we could easily add an omit_files() method to omit them by filename. And I would like to get rid of the filepaths from the classmap entirely, but that would require moving the classes to files with new names in a few known directories. Together that would lower the memory footprint even more.

BTW, having files listed in an autoloader that do not ever need to be loaded does not break anything -- as you know -- it only takes up memory.

And I would once again make you aware that you're now slowly building, feature-for-feature, a replica of the Composer Generator, but without the unit tests, without the community and maturity, and without the standardized configuration.

And without the bulk and overhead of handling all use-cases.

We do not need to handle all use-cases -- constraints can be beneficial, especially to enable simplification -- so we only need to handle the small subset that applies to core.

Further it also means we do not have to deal with Composer making breaking changes in the future which anyone on core who has dealt with breaking changes from jQuery can almost certainly attest to.

This last point is very important, as it allows to reconcile several autoloaders into a single coherent, optimized autoloader. I still fail to see what the exact advantage is of reinventing the wheel...

If Composer were the right tool for the job, I might agree, ignoring the potential for future breaking changes. But Composer is not the right tool for WordPress as several people have recently said.

And if the Composer autoloader can end up being written to do everything exactly as the custom autoloader -- with the same optimizations we are capable of with the custom autoloader -- than what actual benefit is there to using the Composer autoloader other than introducing a lot more code to core and adding an external dependency? (Yeah, I know. Those are not benefits.) I mean, I thought the argument for it was that it was already written?

Composer is a square peg and WordPress has a round hole. It is a really, really nice square peg, but square pegs and round holes don't mix.

Replying to schlessera:

@wonderboymusic: Please enable Issues on the Feature Project's GitHub repo so we can have a more structured discussion.

Yes. Please!

(P.S. I will be so glad when a core committer makes a decision on this -- no matter what the outcome -- so that this pissing match will finally stop.)

Last edited 7 months ago by MikeSchinkel (previous) (diff)

#224 follow-up: @schlessera
7 months ago

For all those watching this ticket, I am truly sorry for the onslaught of notifications! I was foolish enough to reason about arguments with someone who saw this as a "pissing match", so as a result this ticket went to waste.

As it had been decided in Slack to attack this as a feature project (https://wordpress.slack.com/archives/core/p1472675314000588), I recommend the following steps to move forward:

  • Create a Slack channel #feature-autoloader
  • Create an official announcement post by the owner of the feature project (@wonderboymusic), stating the scope and goals of the project, and pointing to the corresponding Slack channel and GitHub repo
  • Create separate issues for any debatable points

#225 in reply to: ↑ 224 @lkraav
7 months ago

Replying to schlessera:

For all those watching this ticket, I am truly sorry for the onslaught of notifications! I was foolish enough to reason about arguments with someone who saw this as a "pissing match", so as a result this ticket went to waste.

I didn't mind at all. Learned a metric ton following the ideas and discussion, discovered a bunch of useful repos and projects from you guys, finally figured out some autoloading stuff for my own projects etc. All for which I thank everyone involved.

I'd reason that the quantity of people knowing the surgeon level guts here is so low overall, that having this out on the interwebs serves as a bigger picture educational tool to hopefully lift the quantity of such knowledgeable people, for glorious future benefit of all.

#226 in reply to: ↑ 223 ; follow-up: @Rarst
7 months ago

Replying to MikeSchinkel:

If Composer were the right tool for the job, I might agree, ignoring the potential for future breaking changes. But Composer is not the right tool for WordPress as several people have recently said.

The degree of this misquote of me is so horrible I can't even. I spent literally years advocating for Composer use with WordPress.

#227 in reply to: ↑ 226 ; follow-up: @MikeSchinkel
7 months ago

Replying to Rarst:

The degree of this misquote of me is so horrible I can't even. I spent literally years advocating for Composer use with WordPress.

It was not my intent to misquote you. I am well aware of your composer advocacy and your website devoted to such but I also believed based on your writings that you did not advocate Composer as a tool for every job.

There are multiple contexts in which Composer can be used and as I read your comments I came away thinking you advocated for Composer in some contexts but recognized and acknowledged it not to be a fit in others. Specifically that Composer was great for managing the code base for WordPress websites but not appropriate for managing the dynamic installation of plugins and themes.

Please tell me if I got that wrong, and if so then consider my quoting retracted and please accept my apology.


BTW, before anyone points out that the proposal on the table is to only to use Composer to build WordPress and to include a Composer-generated classmap and Composer-generated autoloader into core I understand that but have also seen that at least some of the advocates for this approach have indicated that they saw this as a first step to having Composer fully integrated into and shipping with WordPress. Hence some of my strong objection on this ticket.

To be explicit, let me bullet point my arguments for where to use Composer and where not to use Composer:

  1. To build WordPress core - Wonderful, no problem with this at all. Knock yourself out.
  1. To generate a classmap for core - This might or might not be a good idea, but it seems trivial to implement (as you can see here) and I question the wisdom of including a tool in the buildchain when the use-case is trivial and including Composer adds a dependency we do not control.
  1. To use a Composer-generated autoloader - This is what I have been objecting to. The Composer autoloader is designed to handle effectively any possible scenario, and for core and even plugins we have a very finite set of use-cases. Adding an autoloader that runs a lot of code for each class it loads adds unnecessary overhead on page load and makes XDEBUGging annoying. Plus, adding the autoloader is the trojan horse for the next point (#4) and I am objecting upstream rather than later wish I had objected sooner. Constraints are your friends, let them be friendly here rather than throw in the kitchen sink that comes with Composer.
  1. To bundle Composer with WordPress core - I also strongly object to this. This adds an external dependency to WordPress that can break backward compatibility in the future. Remember the fun that jQuery has given us? (And Rarst, this is the scenario which I believed you had written that Composer was not a good fit for.)
  1. To manage plugins and themes with Composer - This I probably do not need to object to because this is clearly the wrong tool for the job. Composer is a static build tool, and plugins and themes are added and removed dynamically.
  1. By WordPress professionals to build and manage website projects - Yes! This is where Composer really shines. (And Rarst this is the use-case for which I understood you to be an advocate.) But it is better IMO to allow the developer to incorporate the latest version of Composer than to bundle a version with WordPress that might be out-of-date.
  1. By plugin and theme developers to manage their dependencies - Yes and No. If their dependencies are the developer's own libraries, then yes (But even here, not a WordPress core-bundled version but instead let the plugin/theme developer build their own toolchain.) OTOH if the dependencies are PHP libraries from Packagist.org then no because there is no way to be sure that some other plugin is not also using the same PHP libraries and pulling in incompatible versions between the plugins.

Hopefully this numbered list of use-cases clarifies and can provide a basis for which to discuss moving forward.

#228 in reply to: ↑ 227 ; follow-up: @Rarst
7 months ago

Replying to MikeSchinkel:

Replying to Rarst:

The degree of this misquote of me is so horrible I can't even. I spent literally years advocating for Composer use with WordPress.

It was not my intent to misquote you. I am well aware of your composer advocacy and your website devoted to such but I also believed based on your writings that you did not advocate Composer as a tool for every job.

There are multiple contexts in which Composer can be used and as I read your comments I came away thinking you advocated for Composer in some contexts but recognized and acknowledged it not to be a fit in others. Specifically that Composer was great for managing the code base for WordPress websites but not appropriate for managing the dynamic installation of plugins and themes.

Please tell me if I got that wrong, and if so then consider my quoting retracted and please accept my apology.

I had told you the use of my quote was horrible, there is no "if" or discussion about it. I have no interest in being summoned into another wreck of a thread and having to explain myself.

There is a vibrant, robust, and by now (relatively) popular practice to use Composer to manage WordPress and its extensions. This was made possible by hard work of people from PHP and WP communities who saw the value that progress delivers and pursued it.

Meanwhile people on trac had been throwing continuous fit dedicated to maintain the worldview of WordPress core being center of the universe and Composer somehow having to justify itself to it.

Composer is now objective reality of modern PHP development. WP can progress towards that or it can not, it's hardly relevant or interesting to the issue anymore.

#229 in reply to: ↑ 228 @MikeSchinkel
7 months ago

Replying to Rarst:

I had told you the use of my quote was horrible, there is no "if" or discussion about it. I have no interest in being summoned into another wreck of a thread and having to explain myself.

Oh come now. I did not summon you here to explain yourself.

I acknowledged your claim of misquote, I explained it was not my intention to misquote you, I then clarified what I (mis-?)understood your position to be, I asked you to simply confirm or deny my understanding (but only because you came here to comment on it), and then I offered you a sincere apology in case I did misunderstand.

Can you not simply be gracious and just accept the apology?

Last edited 7 months ago by MikeSchinkel (previous) (diff)

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


7 months ago

#231 @jorbin
7 months ago

  • Milestone changed from 4.7 to Future Release

I'm moving this out of the 4.7 milestone as I think this isn't going to make it in before the merge deadline for feature projects (which is about 2 weeks out). When this lands in core, it should come in with a good amount of consensus and we aren't quite there yet.

Please continue working on this. I'm excited to see where it goes.

#232 @dd32
6 months ago

In 38896:

Revert the svn:ignore changes in [38394] which was missed in the original revert.

See #36335

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


5 months ago

#234 @pento
5 months ago

In 39449:

Load: Re-add class-feed.php.

class-feed.php is occasionally included directly by custom external code, so should continue to be available. This is deprecated in favour of calling fetch_feed(), however..

Partial revert of [38374].
See #36335, #39027.

#235 @pento
5 months ago

In 39450:

Load: Re-add class-wp-upgrader-skins.php.

class-wp-upgrader-skins.php is occasionally included directly by custom external code, so should continue to be available. This is deprecated in favour of including class-wp-upgrader.php, instead.

Revert of [38379].
See #36335, #39027.

#236 @pento
5 months ago

In 39453:

Load: Re-add session.php.

session.php is occasionally included directly by custom external code, so should continue to be available. This behaviour is deprecated.

Partial revert of [38373].
See #36335, #39027.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 months ago

#239 @lukecavanagh
4 months ago

In relation to actual benchmarking down the line, WebPageTest has script options so set the wp-login.php as the first navigate, then define the login and password id in the script, then set the last target to be wp-admin. So it should be of use for showing that autoloading in core improved backend load time by being able to benchmark admin load time.

https://sites.google.com/a/webpagetest.org/docs/using-webpagetest/scripting

Note: See TracTickets for help on using tickets.