bolt / core

🧿 Bolt 5 core
https://boltcms.io
MIT License
539 stars 159 forks source link

[RFC] Discussion starter: how could we best tackle Extensions? #492

Closed doenietzomoeilijk closed 4 years ago

doenietzomoeilijk commented 5 years ago

This relates to #134

A couple of weeks ago I asked Bob how he figured Extensions would fit into B4, and he figured

That's still up for debate.. so far i've only made some preliminary tests, and i'm leaning towards going with the paradigm i've been using for most things with Bolt 4: Use what's there.

In this case: We have composer (and packagist) to handle installation, versioning, distribution and all

We have SF 4 (and its awesome autowiring) and autoloading to make sure stuff gets autoloaded.

after which I hinted at having implemented something similar in an application somewhere, and promising to do a little write-up about how that works. That got me thinking that the way I used in that application might not actually fullfil the entire need Bolt has, but at least it could get things started, people might have some ideas, and so on. So without further ado:

How to get Symfony to do special things to classes implementing a given Interface

Actually the whole thing is quite simple, and consists of the following parts:

The Interface

In fact, the interface doesn't need to do anything to be useful. Bob mentioned that the Interface could define an initialize function to be run (possible); what I did in my project was make sure the implementor can identify itself.

All code below is barely above pseudocode level, in real life this should all be properly namespaced (probably under \Bolt\Extension) et cetera.

<?php

interface ExtensionInterface
{
    /** Defines the tag that Symfony will get to know our Extensions by */
    public const CONTAINER_TAG = 'bolt.extension';

    /** Let's say that all Extensions should be able to identify them with a polite name */
    public function getName(): string;
}

That's all.

The registry

For the registry, we need to be able to add implementors, retrieve them, etc.

<?php
class ExtensionRepository
{
    /** @var ExtensionInterface[] **/
    protected $extensions = [];

    public function addExtension(ExtensionInterface $extension): void
    {
        $this->extensions[\get_class($extension)] = $extension;
    }

    /** @return ExtensionInterface[] */
    public function getExtensions(): array
    {
        return $this->extensions;
    }
}

Getting the implementors into a repository

Finally, at compile time, we need to grab all our implementing classes and have them stuffed into the repository. This is done using a CompilerPass:

<?php
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

class ExtensionCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if (!$container->has(ExtensionRepository::class)) {
            return;
        }

        $repository = $container->findDefinition(ExtensionRepository::class);
        // The important bit: grab all classes that were tagged with our specified CONTAINER_TAG, and shove them into our Repository
        foreach ($container->findTaggedServiceIds(ExtensionInterface::CONTAINER_TAG) as $id => $tags) {
            /** @see ExtensionRepository::addExtension() */
            $repository->addMethodCall('addExtension', [new Reference($id)]);
        }
    }
}

The CompilerPass is called from within kernel.php:

<?php
use Bolt\Kernel as BoltKernel;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class Kernel extends BoltKernel
{
    public function build(ContainerBuilder $container)
    {
        // Tag all implementors
        $container
            ->registerForAutoconfiguration(ExtensionInterface::class)
            ->addTag(ExtensionInterface::CONTAINER_TAG);

        // Process all our implementors through our CompilerPass
        $container->addCompilerPass(new ExtensionCompilerPass());
    }
}

Create an extension

<?php
class MyExtension implements ExtensionInterface
{
    public function getName(): string
    {
        return 'My fantastic extension';
    }
}

Boom, done.

At this point we can inject the ExtensionsRepository and Do Things™ with extensions. Of course, now that I'm typing all this "from a distance" the registry starts smelling like a service locator, which some people might take offence at. Of course it's still very possible to do something else in the CompilerPass, like run initialise() on all the tagged implementors as Bob envisioned.

The things I'm somewhat less than happy with:

That feels less than clean to me as a user - this feels like giving up on a lot of control. I'd prefer nothing in the extensions space to be autowired, except for the implementing "main" extension class, which can then do the things it needs to do.

This is where the "discussion" bit comes in.

The idea is that Bolt ends up with a simple, safe and elegant way for extension developers to extend it, and for Bolt users to install those extensions and use them. I'm sure there's some folks with an even better idea than me about how to keep this solution clean enough for a situation where we can have an arbitrary number of extensions from arbitrary sources - a scenario that my application doesn't encounter, I am the sole creator of modules (as they're called there), so I know what I can and cannot (and should and should not) do in them.

I also see some relation with the Bundle system of old, that was removed from Symfony - I know the system was there, I roughly know what it did, but since I didn't do a whole lot of Symfony back then I'm not sure if they'd be a great fit for this task, or what the exact reason was for them being axed from Symfony. If someone has some relevant info on that, I'd personally love to hear it.

Thoughts, ideas and critique welcome!

bobdenotter commented 5 years ago

A repository (not the Doctrine kind) or registry to keep all these implementors in, which can be injected. Come to think of it, maybe you could call this a container within the container;

I think this should be saved as a file in bolt/config. If a new extension is found, it can add a my-extension: 1 to this file, and that can be used to toggle the extension on or off. (either from the cli, by editing the file, or from the UI)

this "runs" all extension code at kernel boot

Methinks that's pretty much unavoidable. It'll be up to the extension developers to make sure the initialisation is as simple as possible (as in: Try to do nothing, but register the extension) SF 4 does something similar with hundreds of files, and it's still very snappy.

it autowires everything inside the /src directory as per Symfony standard - that means that extensions can depend on one another and things can get tied up.

I'd prefer to use extensions/ for this. src/ is code for the current project. Not stuff meant to be shared between different projects. Also, if we'd put it in extensions/ the developer can decide for themselves if they think it's Best Practice™ or not to shove this folder into their git.

a badly coded extension can take down the entire system without an easy way to solve it, save for mucking about with files in a filesystem...

Can be prevented with this extensions.yaml, mentioned above.

The idea is that Bolt ends up with a simple, safe and elegant way for extension developers to extend it, and for Bolt users to install those extensions and use them.

Yes, that pretty much sums up the goal. :-)

JarJak commented 5 years ago

I'd prefer to use extensions/ for this. src/ is code for the current project. Not stuff meant to be shared between different projects.

For stuff meant to be shared between different projects there is vendor/. Any custom local extension or vendor overwrite usually sits under src/. But again, we can make it configurable.

doenietzomoeilijk commented 5 years ago

There's something to be said for both locations for extensions. Personally, I wouldn't make it configurable, since that's adding another variable to a user's setup that could make troubleshooting an issue harder, without a hard benefit.

If we decide to plonk extensions into /extensions like Bolt does it currently, can have a separate composer.json purely for extensions, the directory is - by default - sealed off from autowiring, and so on.

Of course, that might also be the biggest drawback: I could see autowiring and the DI you can do with it making a lot of extensions work far more Symfony-esque (or at least enable them to work that way), which is a plus in my book.

As for storing the found extensions in a config file: yes, it'll enable us to switch extensions off and on, as a matter of fact you could add an enabled key to the extension's config itself if you want to keep the amount of YAML files lower. Of course, storing a config file in the proper place is one of those things that get done on an extension's initialize / first run.

If you do go the separate which-plugins-are-enabled file route, I'd just have an array of FQCNs, no need to add a 1 or 0. Not being listed in that array means the extension is disabled. Using the FQCN means never having name collisions there.

bobdenotter commented 5 years ago

This might be interesting to take a peek at: https://github.com/heiglandreas/wp_talkToComposer

doenietzomoeilijk commented 5 years ago

I'd prefer to use extensions/ for this. src/ is code for the current project. Not stuff meant to be shared between different projects.

For stuff meant to be shared between different projects there is vendor/. Any custom local extension or vendor overwrite usually sits under src/. But again, we can make it configurable.

There's something to be said for that - just roll plugins into the regular /vendor structure. I'm not sure how this complicates things for people who don't use composer to start up their projects. Can someone weigh in on that?

Edit (because post bike-ride-to-work clarity): also, if we were to drop extensions into /vendor, we miss out on autowiring (which may or may not be a bad thing). How would we do discovery in that case? The config file Bob mentioned? Toss 'em in Kernel.php or something similar?

Doing that might make the whole discovery and tagging song and dance I outlined above superfluous, by the way. If we're going to explicitly point at stuff, there's no need for that.

bobdenotter commented 5 years ago

There's something to be said for that - just roll plugins into the regular /vendor structure.

It does have some benefits for sure, but it also has one mayor drawback that I can think of: Installing or removing extensions will force a composer update on the entire project. This is unavoidable, because it's simply how composer works.

We should keep in mind that we're dealing with different audiences here: the people who keep dependencies in check, like which exact bolt version and composer versions are being used, are not necessarily the same people that will add the "Bolt SEO" or "Add Facebook spyware"-extensions.

additionally, the autowiring looks much more straightforward if we keep them in extensions/, so I'm going to stick with that as my preferred way forward for now.

JarJak commented 5 years ago

Installing or removing extensions will force a composer update on the entire project. This is unavoidable, because it's simply how composer works.

Unless you call composer require --no-update

bobdenotter commented 5 years ago

Unless you call composer require --no-update

I didn't know about that flag. 🤔

I can't find much about it online, but my mind is too small to comprehend how that would work if the newly installed package comes with its own dependencies or version constraints.

JarJak commented 5 years ago

how that would work if the newly installed package comes with its own dependencies or version constraints

It will only install what is required for the new dependency and stop if some current deps needs an update to work with the new one.

https://getcomposer.org/doc/03-cli.md#require

bobdenotter commented 5 years ago

OK, if that's the case, let's go for vendor.. It's easier because it's already namespaced properly, and it'll be much nicer to have your dependencies in one place. For example, let's say your project has some functionality that requires ramsey/uuid, and you pull in an extension that requires the same. It'd be nonsense to have it installed twice in vendor/ramsey/uuid and in extensions/vendor/ramsey/uuid.

JarJak commented 5 years ago

It'd be nonsense to have it installed twice in vendor/ramsey/uuid and in extensions/vendor/ramsey/uuid.

Yes, it will also rise a namespace conflict between vendors. But there is https://getcomposer.org/doc/06-config.md#vendor-dir which could point extension's vendor to project's vendor dir, so it's not a problem.

However

it'll be much nicer to have your dependencies in one place

This is a very strong argument as Bolt and it's extensions are also dependencies for a project.

bobdenotter commented 4 years ago

I'd like to close this for now. So far, we have working extensions. If they need improvements, we'll open more focused issues.