getherbert / herbert

The WordPress Plugin Framework:
http://getherbert.com/
632 stars 95 forks source link

[Proposal] Splitting framework into its own component. #7

Closed mdwheele closed 9 years ago

mdwheele commented 9 years ago

Hey Herberts!

I just found this and I think it's awesome. Love the organization / public API on a lot of the different components. Awesome work folks!

I was curious if there had been any thought to separate the framework out away from the "skeleton" plugin such that a developer could still use the skeleton (this repository) as a starting place, but also had the option of simply including the framework as a dependency to an existing plugin and using only the pieces they require.

Laravel does something similar with laravel/laravel being the skeleton application that is using the framework and laravel/framework being the framework packages. Symfony does something similar at symfony/framework-standard-edition

To be honest, I haven't gone that much in-depth with Herbert yet and I usually try to prepare more before submitting questions like this; but just curious.

We've also run into issues where shared code autoloaded by WordPress caused PHP errors from resulting namespace conflicts that I was curious if you've encountered. An example that might cause this would be:

Like I said, I haven't done enough research with Herbert here and will be playing around with it tonight.

Alsoooooo... are you guys amenable to contributions of tests? :) Would be happy to go down that road, myself.

johnReeve commented 9 years ago

I have been evaluating the framework for a project, and the name conflict thing is something that I've also been wondering about, but I'm not sure what a good answer is. I think that this would be an issue in any plugin that uses the composer autoloading... for instance using this with a theme that uses Timber has been brought up. It's something I have been wondering about.

jasonagnew commented 9 years ago

Thanks for the feedback. I like the Laravel approach as it also allows you to update the framework using composer. Hoping to some work it on at the weekend.

I feel a little silly not even thinking about naming conflicts when having two plugins using Herbert. I’ll put a comment on here tomorrow with plan for this - see what people think.

It’d be great if you want to contribute tests :)

mdwheele commented 9 years ago

Don't feel silly, haha. It's an interesting problem with (I'm sure) many "interesting" solutions. I'll try to find some time to play around with it. I've got a few plugins that I think are using different versions of Guzzle for interacting with internal APIs and I honestly can't remember why that's not exploding everywhere (as it should have kinda the same problem being described here)

Probably 6-8 months ago, we monkeyed with the idea of having a "framework plugin" that was installed separately and shot that down as a solution pretty quick because it felt super awkward. We settled (at the time) on having our build process inject a unique value into the namespace of our common code (that we brought in with composer) at build-time... would not recommend that; feels super hacky, but it worked.

I haven't had to touch any of that for a few months as they're ticking along, but Herbert has me curious to revisit, for sure.

mdwheele commented 9 years ago

So I went and set up a little sample rig to attempt to simulate what happens when WordPress loads plugins that have Composer dependencies:

Plugin 1 composer.json

{
    "name": "plugin/one",
    "require": {
        "php": ">=5.3.0",
        "mockery/mockery": "0.9"
    }
}

Plugin 2 composer.json

{
    "name": "plugin/one",
    "require": {
        "php": ">=5.3.0",
        "mockery/mockery": "0.7.2"
    }
}

Project structure

P1/...
P2/...
driver.php

class Mockery
{
    const VERSION = 'P1'; // Added line for demonstration
    const BLOCKS = 'Mockery_Forward_Blocks';
...

In my "driver" file representing WordPress loading all plugins (which would then call composer autoloaders):

<?php

require 'P1/vendor/autoload.php';
require 'P2/vendor/autoload.php';

echo Mockery::VERSION . PHP_EOL;

Output (when P2 loaded after P1):

[vagrant@kraken playspace]$ php driver.php 
P2

Output (when P1 loaded after P2):

[vagrant@kraken playspace]$ php driver.php 
P1

Thus, in this case... it doesn't explode everywhere... but if plugins use different versions of components, it is suspected that the order that WordPress decides to load plugins matters greatly.

I ran similar tests using require_once on the composer autoloaders, but it doesn't really matter. Composer uses require under the hood and on top of that is actually registering 2 separate autoloaders with PHP, so that's why the "last registered" is winning... is because it's the first to discover a successful match for Mockery\Mockery.

I'll go try to get this PoC into WordPress and see if anything changes, but more-or-less just dropping this here as experimental data.

johnReeve commented 9 years ago

Regardless of the load order, if there are multiple autoloads of the same dependency (even of the same version), won't there be a name problem between those two namespaces? I might be wrong, but I don't think you can redeclare a class.

I'm totally unsure of how composer build the dependencies of things that have dependencies, but I wonder if there is some way to create a unified composer install for all plugins and themes... you'd have a single composer.json maybe and then the individual plugins would declare their dependencies, and the vendor directory would happen in, say /wp-content?

Is that wholly misguided? It would certainly make it difficult for most folks to install something based on that way of working, but maybe its possible as a way of working with multiple composer-related installs across a single wordpress application.

johnReeve commented 9 years ago

Something along these lines? I'm still digesting how it works, but I think that is the general idea:

http://roots.io/using-composer-with-wordpress/

mdwheele commented 9 years ago

There isn't risk of redeclaring the class. In the example above two PHP auto loaders are registered, Composer1 and Composer2. At runtime, if PHP cannot resolve a class, it delegates to the first registered auto loader that can handle the request before failing with a PHP_Error. So it's only going to ever load one version of the class. The roots link is interesting, but places responsibility on system administrators or developers that have a dependency on your plugin. Basically your plugin, as a dependency, probably reaches too far outside its "sandbox". Roots gets away with this as it is its own platform of sorts.

The risk here is that if there are multiple auto loaders registered at the same time, clients of auto loaded code can't be sure they are getting the version they expect. I'm going to put together a demo WordPress install together today with pretty much the same example above and see what happens. I'll report back here.

mdwheele commented 9 years ago

Check this out! I've put together a demonstrative WordPress installation that simulates two plugins using composer and having a dependency on two different "versions" of the same package. The readme has all supporting docs, so I'll leave that out.

https://github.com/mdwheele/wp-composer-autoload-demo

I put everything in the repository, including the vendor directories for composer; just so it could be cloned down and ran with no setup other than needing a blank WordPress database.

ConnorVG commented 9 years ago

@mdwheele this is pretty much how I'm currently going about doing things but Herbert has it's own autoloading sequence (to keep one instance loading in all plugins). I'm currently undergoing no serious issues with this (nor any slowdown) but it may take a little while for me to show my progress with Herbert as the framework needs to be adapted to have a pluggable sort of setup.

One big influence of this is Laravel, I've dug into the Laravel framework to find out how it handles certain things (such as Service Providers) and I'm looking forward to see where it takes Herbert. What are you thoughts on this sort of approach?

bitdagger commented 9 years ago

To expand on what @mdwheele is saying:

Although there isn't a risk of direct namespace collisions (because the autoloader will only load the class in that namespace once, and ignore subsequent includes), you start running into issues when you update the underlying dependency and start to utilize new features of the dependency.

As an example, if you have a plugin PluginA installed from Wordpress.org (you don't manage this plugin yourself). PluginA runs on Herbert and works great. It revolutionizes your website, and everyone is happy.

Now you see that someone has released PluginB, which does something cool that you can't live without. Some time has passed between when PluginA was released, and so PluginB (which also uses Herbert) is running a newer version of the dependency. The new version of Herbert allows PluginB to use the Foo API to make your website do something cool. You load up PluginB, and one of two things could happen.

1.) Everything works great and you smile to yourself at how wonderful your site looks.

2.) PluginB blows up.

In the case of 2.) PluginA was run through the autoloader first, and so when PluginB requests Herbert it gets the old version that was already loaded. The old version doesn't have the API call that PluginB expects, so errors blow up.

I've given this a lot of thought in the past, having run into this exact problem while working on a simple plugin framework of my own, and I haven't really come up with a workable solution. It seems like the only options are splitting the dependency out into its own plugin (which is a bit silly, confusing, also also means you can never have backwards compatibility breaking changes ever), or somehow modifying the autoloader to take the version of loaded classes into account and dynamically modify the namespace somehow such that both versions are loaded at once, but without namespace conflicts (no idea how to manage that off the top of my head).

ConnorVG commented 9 years ago

Hey contributers!

We've just pushed this idea's initial implementation to the rewrite branch. It'd be great if we could get your response to this. We're currently working on some information to help getting started. The actual framework can be found at https://github.com/getherbert/framework.

Thanks! :smile:

jasonagnew commented 9 years ago

@mdwheele @johnReeve @bitdagger Hey everyone the rewrite branch is here. The README gives an overview, with feedback to be posted here. Thanks :)

jasonagnew commented 9 years ago

The rewrite is now live and working with example plugin coming this weekend.