Core: New PHP Extension API

[tobyzerner]

In every extension, we have an AddClientAssets listener which is basically the same:

class AddClientAssets
{
    public function subscribe(Dispatcher $events)
    {
        $events->listen(ConfigureClientView::class, [$this, 'addAssets']);
    }

    public function addAssets(ConfigureClientView $event)
    {
        if ($event->isForum()) {
            $event->addAssets([
                __DIR__.'/../../js/forum/dist/extension.js',
                __DIR__.'/../../less/forum/extension.less'
            ]);
            $event->addBootstrapper('flarum/auth/facebook/main');
        }

        if ($event->isAdmin()) {
            $event->addAssets([
                __DIR__.'/../../js/admin/dist/extension.js',
                __DIR__.'/../../less/forum/extension.less'
            ]);
            $event->addBootstrapper('flarum/auth/facebook/main');
        }
    }
}

Given that this asset file-structure is a best practice, we could reduce this duplication by providing an instantiable AddDefaultClientAssets listener. In bootstrap.php, instead of:

return function (Dispatcher $events) {
    $events->subscribe(Listener\AddClientAssets::class);
};

you would use:

return function (Dispatcher $events, Extension $extension) {
    $events->subscribe(new AddDefaultClientAssets($extension));
};

This would add the default asset file paths if they exist, along with JS bootstrappers using the Extension's ID as a prefix.

We could also extend this idea of shortcut listeners to other things, e.g.:

return function (Dispatcher $events) {
    $events->subscribe(new AddForumRoute($extension, 'get', '/auth/facebook', 'auth', FacebookAuthController::class));

    $events->subscribe(new AddPostType(DiscussionStickiedPost::class));
};

This needs discussion because to me it's a little unclear how far we would want to go with providing these helpers. Where do we draw the line?

[luceos]

I only see new "shortcuts" popup, with easier adoption and faster development as a reason. But isn't this pointing us at a bigger problem? Endlessly adding helpers will only increase the size of the platform, which is not something we should do lightly.

How can we optimize the code to make extension development easier, without losing any flexibility at the same time?

[franzliedke]

I agree with Toby's suggestion.

But yes, we have to be careful with all of these. But here, just like with my migration helpers, I'd say it's warranted. Since we're doing this only by adding new helper classes, the changes are very un-intrusive, so I don't see any big problems with that approach.

[franzliedke]

In fact, it's awesome. :+1:

[franzliedke]

I discussed this with @tobscure, here's a quick summary:

• Toby is currently refactoring the asset management, so these helpers will look slightly different
• These are the kind of helpers that can - in certain circumstances - _decrease_ our public API surface. The APIs being used by such helpers can be considered private, and implementing sensible helpers will allow us to change the underlying implementation without breaking BC of the public API for extensions.
• As for guidelines on which helpers to add: We'll likely add a section to the extension docs that will explain how certain typical tasks can be achieved - a sort of "cookbook". The use cases explained there will be primary candidates for such shortcuts, anything else will need good legitimation.

[tobyzerner]

One deal-breaker with the proposed syntax I just noticed is that since these helper subscribes are instantiated explicitly within the bootstrapper, we lose our way to inject dependencies. For example:

$events->subscribe(new LoadLanguagePackFrom(__DIR__));

But part of the "load language pack from" code involves using an injected instance of the LocaleManager. It's definitely not nice to have to inject these things into the bootstrapper and then pass them along, so we need to rethink.

Toying with some ideas:

// ExtensionApi is an object which is specific to the current extension being booted,
// so we can inject vendor/package prefix where appropriate.
return function (ExtensionApi $api) {
    // Contains a bunch of shortcut methods. Problem with this is that the
    // ExtensionApi class would end up with a huge number of dependencies.
    $api->addDefaultClientAssets();
    $api->loadLanguagePackFrom(__DIR__);

    // Register an event subscriber.
    $api->subscribe(SomeCustomSubscriber::class);

    // Register an event listener (use Reflection to eliminate the first argument).
    $api->listen(function (SomeEvent $event) {
        // ...
    });
};

// Hmm, so essentially helpers need to be injected into the bootstrap function
// in order for this to work and be clean. Something like this...
return function (LoadLanguagePackFrom $loadLanguagePackFrom) {};

// But obvious that's horrible. What about a more declarative
// command-bus-like system? Each of these would be a "command" DTO,
// and would be self-handling (we would inject any dependencies using the
// Container's "call" method). Reminds me of the old Extend API ...
// https://github.com/flarum/core/tree/a577910d04f466ad69df0e420858e3518718ade2/src/Extend
// ... looks like we might come full-circle!
return [
    new Extend\AddDefaultClientAssets(),
    new Extend\LoadLanguagePackFrom(__DIR__),
    new Extend\EventSubscriber(SomeCustomSubscriber::class),
    new Extend\EventListener(function (SomeEvent $event) {})
];

[franzliedke]

Hmm, good point. I'll think about it, too.

[tobyzerner]

Some brainstorming on the Extenders idea:

• Contrary to what I think I was saying back in early 2015, I think I quite like the "declarative" approach to writing extensions. It will allow us to do some potentially cool things, like map out exactly what an extension does without running any of its code. (Not sure if that will have any real-world application though.)
• Instead of using event subscribers to organise code, we can allow nesting of arrays, so that you can put more extender declarations in other files and include them:

// bootstrap.php
return [
    new Extend\AddDefaultClientAssets(),
    include 'addDiscussionsTagsRelationship.php'
];

// addDiscussionsTagsRelationship.php
return [
    // more extenders here...
];

• Instead of using event subscribers to inject dependencies, we could have an extender which does exactly that:

return [
    new Extend\Inject(function (SettingsRepositoryInterface $settings) {
        return new Extend\EventListener(function (DiscussionWasSaved $saved) use ($settings) {
            // Do something with $saved->discussion and $settings
        });
    })
];

Alternatively, we could do the injection on the outside, still allowing for nesting:

return function (SettingsRepositoryInterface $settings) {
    return [
        new Extend\EventListener(function (DiscussionWasSaved $saved) use ($settings) {
            // Do something with $saved->discussion and $settings
        }),
        include 'addDiscussionsTagsRelationship.php'
    ];
};

• Even better, since most of the time the injected dependency is actually the SettingsRepositoryInterface, we can make an Extension instance available in the bootstrap file and get a prefixed SettingsRepository from that:

return [
    new Extend\EventListener(function (DiscussionWasSaved $saved) use ($extension) {
        // Do something with $saved->discussion
        $whatever = $extension->getSettings()->get('whatever');
    })
];

• We can come up with all kinds of cool declarative shortcuts:

return [
    // Simply add relationships to models/serializers
    (new Extend\ModelRelationship(Discussion::class))->belongsToMany(Tag::class),
    (new Extend\SerializerRelationship(DiscussionSerializer::class))->hasMany('tags', TagSerializer::class),

    // Add a "canTag" attribute to serialized discussions based on the actor's permission
    (new Extend\SerializerAttributes(DiscussionSerializer::class))->permission('tag')
];

[tobyzerner]

So ultimately I think the advantage of the declarative API is that it makes it easy to automate prefixes and whatnot (as in the second-to-last dot-point above; related #850). We should also consider changing migrations to use a similar syntax, for the same purpose. Something like this:

return [
    new Migrate\CreateTable('whatever', function (Blueprint $table) {
        // ...
    }),

    new Migrate\AddColumns('discussions', [
        'foo' => ['string'],
        'bar' => ['dateTime', 'nullable' => true]
    ])
];

[tobyzerner]

Spent another few hours thinking about this, and boy it's a tough problem.

The "extender" API I've proposed seems really slick on the surface, but I think it quickly breaks down when you start to translate it into a real world use case. Take a look at this annotated version of how the tags extension would look, for example:

<?php

namespace Flarum\Tags;

use Flarum\Core\Discussion;
use Flarum\Core\User;
use Flarum\Extend;
use Flarum\Event;
use Flarum\Extension\Extension;

return function (Extension $extension) {
    return [
        // Extend the web app with default assets. So far so good!
        Extend\WebApp::defaultAssets(),

        // Define some forum routes. Sure.
        Extend\ForumRoute::get('/t/{slug}', 'tag'),
        Extend\ForumRoute::get('/tags', 'tags'),

        // Define a model relationship. OK.
        Extend\Model::discussion()
            ->belongsToMany(Tag::class),

        // And define a serializer relationship, as well as a permission attribute. Nice.
        Extend\Serializer::discussion()
            ->hasMany('tags', Api\Serializer\TagSerializer::class)
            ->permission('tag'),

        Extend\Serializer::forum()
            ->hasMany('tags', Api\Serializer\TagSerializer::class)

            // Add attributes to a serializer. This example is OK too... but what happens if
            // you want to do something even the slightest bit more complex, like inject
            // a TagRepository or anything else? It's starting to look like a *class* would
            // be a good way to organise that logic.
            ->attributes(function ($model) use ($extension) {
                return [
                    'minPrimaryTags' => $extension->getSetting('min_primary_tags'),
                    'maxPrimaryTags' => $extension->getSetting('min_primary_tags'),
                    'minSecondaryTags' => $extension->getSetting('min_primary_tags'),
                    'maxSecondaryTags' => $extension->getSetting('min_primary_tags'),
                ];
            }),

        // Same thing here... if preloading the tags involved the TagRepository (as it
        // probably should), then it'd make sense to extract this logic into a class.
        Extend\ApiController::showForum()
            ->include([
                'tags' => function ($actor) {
                    return Tag::whereVisibleTo($actor)->with('lastDiscussion')->get();
                },
                'tags.lastDiscussion',
                'tags.parent'
            ]),

        Extend\ApiController::listDiscussions()->include('tags'),
        Extend\ApiController::showDiscussion()->include('tags'),
        Extend\ApiController::createDiscussion()->include(['tags', 'tags.lastDiscussion']),

        Extend\ApiRoute::get('/tags', 'tags.index', Api\Controller\ListTagsController::class),
        Extend\ApiRoute::post('/tags', 'tags.create', Api\Controller\CreateTagController::class),
        Extend\ApiRoute::post('/tags/order', 'tags.order', Api\Controller\OrderTagsController::class),
        Extend\ApiRoute::patch('/tags/{id}', 'tags.update', Api\Controller\UpdateTagController::class),
        Extend\ApiRoute::delete('/tags/{id}', 'tags.delete', Api\Controller\DeleteTagController::class),

        Extend\PostType::register('discussionTagged', Post\DiscussionTaggedPost::class),

        // And of course, for most event listeners we'll want to extract into a subscriber anyway,
        // because they'll probably have dependencies or enough code to warrant organisation
        // within a separate class.
        Extend\Event::subscribe(Listener\CreatePostWhenTagsAreChanged::class),

        Extend\DiscussionGabmit::register(Gambit\TagGambit::class),
        Extend\Event::subscribe(Listener\FilterDiscussionListByTags::class),

        Extend\Event::subscribe(Listener\SaveTagsToDatabase::class),
        Extend\Event::subscribe(Listener\UpdateTagMetadata::class),

        // Policies are a great example of too much code for the bootstrap file...
        Extend\Policy::discussion()
            ->can('tag', function (User $actor, Discussion $discussion) use ($extension) {
                if ($discussion->start_user_id == $actor->id) {
                    $allowEditTags = $extension->getSetting('allow_tag_change');

                    if ($allowEditTags === '-1'
                        || ($allowEditTags === 'reply' && $discussion->participants_count <= 1)
                        || ($discussion->start_time->diffInMinutes(new Carbon) < $allowEditTags)
                    ) {
                        return true;
                    }
                }
            })
    ];
};

So overall with extenders, you'll end up with a bootstrap.php that half contains some basic definitions, and half delegates to event subscribers anyway. Contrast that with the old tags bootstrap.php:

return function (Dispatcher $events) {
    $events->subscribe(Listener\AddClientAssets::class);
    $events->subscribe(Listener\AddDiscussionTagsRelationship::class);
    $events->subscribe(Listener\AddForumTagsRelationship::class);
    $events->subscribe(Listener\AddTagsApi::class);
    $events->subscribe(Listener\CreatePostWhenTagsAreChanged::class);
    $events->subscribe(Listener\FilterDiscussionListByTags::class);
    $events->subscribe(Listener\SaveTagsToDatabase::class);
    $events->subscribe(Listener\UpdateTagMetadata::class);
};

Even if it's not as "easy", it feels a lot cleaner and more organised. Each different "task" that the extension performs (via a set of listeners) is grouped into a descriptively-named subscriber. Every task is implemented this way, no matter how big or small or simple or complex. I think this kind of code organisation and consistency is a good thing to encourage. Not to mention there is no limitation to power/flexibility with this setup.

So now I am thinking we need to stick with event subscribers as our primary extension API. But we can still certainly make the argument that the extension API is very verbose for basic repetitive tasks, and should be simplified. How about we try and do this without losing the power/flexibility/cleanliness of event subscribers. For example:

// bootstrap.php
// Simplify by allowing an array of subscriber classes to be returned (instead of a closure).
// Provide some default subscriber implementations for common tasks. Arguments cannot
// be passed, but the Extension instance will be injected which is sufficient in most cases.
return [
    \Flarum\Listener\AddDefaultAssets::class,
    AddDiscussionTagsRelationship::class
];

// AddDiscussionTagsRelationship.php
// For common tasks which require more input, require that the extension defines a new
// subscriber class (encourage descriptive class name) and then work out a way to make
// that common task easy within the subscriber. One way would be extending a parent
// class:
use Flarum\Extend\AddRelationship;
class AddDiscussionTagsRelationship extends AddRelationship
{
    protected $model = Discussion::class;
    protected $name = 'tags';
    protected $related = Tag::class;
    protected $type = AddRelationship::BELONGS_TO_MANY;
}

// Drawback with this is that it limits a subscriber like this to doing just one thing (i.e.
// specifying a single relationship). Maybe this isn't such a bad thing? Would need to
// play with it a bit more. An alternative would be to make use of some kind of utility
// class/trait within a subscriber:
use Flarum\Extend\AddRelationship;
class AddDiscussionTagsRelationship
{
    use AddRelationship;

    /**
     * @param Dispatcher $events
     */
    public function subscribe(Dispatcher $events)
    {
        $this->belongsToMany($events, Discussion::class, 'tags', Tag::class);
    }
}

use Flarum\Extend\AddRelationship;
class AddDiscussionTagsRelationship
{
    /**
     * @param Dispatcher $events
     */
    public function subscribe(Dispatcher $events)
    {
        AddRelationship::belongsToMany($events, Discussion::class, 'tags', Tag::class);
    }
}

// For reference, here's the current way we have to do it:
class AddDiscussionTagsRelationship
{
    /**
     * @param Dispatcher $events
     */
    public function subscribe(Dispatcher $events)
    {
        $events->listen(GetModelRelationship::class, [$this, 'addRelationship']);
    }

    /**
     * @param GetModelRelationship $event
     * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany|null
     */
    public function addRelationship(GetModelRelationship $relationship)
    {
        if ($relationship->is(Discussion::class, 'tags')) {
            return $relationship->model->belongsToMany(Tag::class, 'discussions_tags', null, null, 'tags');
        }
    }
}

In any case, the tags bootstrap.php could end up looking something like this:

<?php

use Flarum\Tags\Listener;

return [
    \Flarum\Listener\AddDefaultAssets::class,

    Listener\AddDiscussionTagsModelRelationship::class,
    Listener\AddDiscussionTagsApiRelationship::class,
    Listener\AddDiscussionApiAttributes::class,

    Listener\AddForumTagsApiRelationship::class,
    Listener\AddForumApiAttributes::class,

    Listener\AddTagsApiRoutes::class,

    Listener\AddDiscussionTaggedPostType::class,
    Listener\CreatePostWhenTagsAreChanged::class,

    Listener\AddTagGambit::class,
    Listener\HideTagsFromDiscussionList::class,

    Listener\SaveTagsToDatabase::class,
    Listener\UpdateTagMetadata::class
];

/cc @franzliedke

[franzliedke]

Howdy, thanks for putting so much thought into this!

After reading it, I must agree that changing this is probably for no good. So, to reach the original goal on reducing the boilerplate that's needed for common task, that mostly leaves us with two questions from my perspective:

Event listener boilerplate

IMO, this one slipped through the cracks a little bit. It really irks me that no matter what kind of concept you extend in the bootstrap file, you're always dealing with some kind of event, and listening to it. Of course that is exactly what happens under the hood, but it is confusing because it's not the kind of interface somebody extending a certain concept (e.g. adding a relationship to a model, exposing a new endpoint in the router etc.) would expect to deal with.

Now that I'm writing these words, though, I notice that your suggestion of returning an array of class names would alleviate one part of this, so I guess I'm all for that (as an additional option parallel to the existing ones).

For the actual listener classes, your subclassing approach would deal with this problem, too, as the actual event instances and the calls to listen would be hidden in the base classes.

That's probably about how far we have to go. I wouldn't want to add another layer of abstraction just to hide this event listening interface, but I've been wondering how to change the current approach to read a bit less confusing... Anyway, this approach seems to do it well enough for me.

Shortcuts

How do we implement the shortcuts? As already explained above, I prefer the straight subclassing approach, mostly because it hides the events and the calls to listen. To continue with your example of adding a relationship, that would mean something like the following:

class AddDiscussionTagsRelationship extends AddRelationship
{
    protected $model = Discussion::class;
    protected $name = 'tags';
    protected $related = Tag::class;
    protected $type = AddRelationship::BELONGS_TO_MANY;
}

Note that using traits would mean we'd have to pass the event instance to the trait's method(s) always, which significantly pollutes the interface.

An alternative would be simple static factory methods (like we do with the refactored migrations). Both approaches could achieve the same thing. There's really not much reason to dislike inheritance here (slighly more boilerplate being the worst thing). The devil is in the details:

// This would be in the bootstrap file
$dispatcher->subscribe(
  AddRelationship::belongsToMany(
    Discussion::class,
    'tags',
    Tag::class
  )
);

Benefits:

• Less repetitive creation of classes that only overwrite two or three properties.
• Easy to understand what a listener is doing (as opposed to the natural limitations to the expressiveness of a class name such as AddTheTagsRelationshipToTheDiscussionModel).

Disadvantages:

• We couldn't return an array of class names anymore.
• The bootstrap file could get out of hand.
• More complex things (like your example of adding a policy) would still be to complex to grasp with a quick glance.
• We're dealing with the dispatcher again.

Open questions:

• Does it make any difference that we're always dealing with instances rather than class names?

Hmm...

---

Looks like I ended just where you did. Again, you thought this through very well. ;)

So, I'll have to sleep over this for a bit. Which I'll do now. :)

[franzliedke]

Oh, turns out I _can_ think at night.

What about turning into

If we can ensure that extensions only need event listeners, we could go the same route as the new migrations, and return listener instances from a bunch of files.

So, in a folder called extend in your extension (example from flarum-ext-mentions), you'd have the following files:

• add_assets.php
• add_mentioned_relationship.php
• format_post_mentions.php
• format_user_mentions.php
• update_post_mentions_metadata.php
• update_user_mentions_metadata.php
• add_filter_by_mentions.php

These would, when included, simply return a listener instance.

We'd get rid of the little bit of duplication in the bootstrap.php file, but would remain flexible, really. Custom listeners could still be created.

Remaining question:

• How to handle other logic that used to be in bootstrap.php? For example, flarum-ext-mentions adds a new namespace to the view factory. Possible ways around this: 1) Let these files return closures, which will be handled through App::call(), providing service-provider-like dependency injection like we had in bootstrap.php. 2) Provide "extenders" (factory methods) for all the tasks that were previously handled through custom injections in bootstrap.php.

Thoughts? (I hope that was understandable.)

[tobyzerner]

It really irks me that no matter what kind of concept you extend in the bootstrap file, you're always dealing with some kind of event, and listening to it. Of course that is exactly what happens under the hood, but it is confusing because it's not the kind of interface somebody extending a certain concept (e.g. adding a relationship to a model, exposing a new endpoint in the router etc.) would expect to deal with.

A small counterpoint, just for the record – I see two advantages to only dealing with events conceptually:

1. API consistency. You don't have to switch between the two mindsets (declaration/listeners) if everything is an event. And you know that everything you can possibly do with the API is located and documented under the one namespace (Flarum\Event).
2. Power and flexibility. What if you want to add the same relationship to multiple models, or an API include to multiple endpoints, or some policy logic for multiple abilities? Using an event, you can have your own conditions in an if block to achieve these kinds of things. Rather than having to declare the same logic etc. for multiple cases.

Anyway, back to the real discussion at hand...

Static Factory Methods

I think we could certainly make this work with the array-returning bootstrap.php, we'd just need to check for an instance vs. a string (class name) when we loop through the array.

return [
    \Flarum\Listener\AddDefaultAssets::class,
    AddRelationship::belongsToMany(Discussion::class, 'tags', Tag::class)
];

_However_, does this not have the same problem as my initial suggestion ($events->subscribe(new LoadLanguagePackFrom(__DIR__))), in that we can't inject any dependencies into the created instances?

I also agree that the bootstrap file could get out of hand... Personally I prefer the cleanliness of the simple array of descriptive class names, even if it does result in some repetitive creation of very small classes.

Listeners as Files

This is a nice idea, but I have a couple questions:

• How does doing a directory scan for every extension on every pageload affect performance? (This is something we don't have to worry about for migrations.)
• How would you create custom listeners? Would you just define a class in your file (what namespace would it need to be under?) Or would to have to instantiate it and return the instance? How would you inject dependencies?
• Would the ability to disable a listener by commenting it out from the bootstrap file be missed?

Traits Revisited

The subclassing thing is still bugging me a tiny bit in that you can only do one thing. You're right that traits are less than ideal with having to pass the dispatcher around... But could we consider factoring that out by setting the event dispatcher as an instance variable in a parent class?

class AddDiscussionTagsRelationship extends AbstractListener
{
    use AddRelationship;

    public function boot()
    {
        $this->addBelongsToManyRelationship(Discussion::class, 'tags', Tag::class);
    }
}

I guess this doesn't really help us for shortcuts that need dependencies injected... for that, subclassing is the only way.

[tobyzerner]

@franzliedke Do you have any further thoughts here?

[tobyzerner]

OK, I think I've cracked it. Here's my proposal, inspired by the best parts from all of the above ideas. I've taken a top-down approach with the API design – aiming for it to be as easy/nice to use as possible.

---

bootstrap.php returns an array which is looped through recursively when the extension is loaded. In its simplest form, this is just a function or an array of functions which have their dependencies injected when they are called:

return [
    function (Dispatcher $events) {
        $events->listen(DiscussionWasStarted::class, function (DiscussionWasStarted $event) {
            // do something with $event->discussion
        });
    }
];

In order to abstract away common tasks, we have a whole bunch of Flarum\Extenders. These are classes with __invoke methods which can be used in place of plain ol' functions:

// bootstrap.php
return [
    new Flarum\Extend\Listener(function (DiscussionWasStarted $event) {
        // do something with $event->discussion
    })
];

For the Listener extender, we use the Reflection API to get the event class from the first argument in the passed closure, and then register the appropriate event listener with the dispatcher in __invoke.

And of course, since arrays are looped through recursively, you can structure your event extenders with includes:

return [
    Flarum\Extend\WebApp::defaultAssets(),
    include 'includes/extend_api.php'
];

... That's all there is to it, really! With this setup we get an API that is super simple/quick to get started with, and allows you to structure your code very cleanly. I've converted most of the Tags extension over as an example of how nice it is:

https://gist.github.com/tobscure/fad353fbe3da13cb83ba69de0b5d7cf4

However, it's still very flexible and powerful – there are multiple right ways of doing things. Like dependency injection – you could wrap your event listener in a function containing the dependencies. Or you could just use app(), since in most cases you won't really be unit testing your extender use. And if you _do_ need your code to be unit testable (eg. if there's some business logic in there), then you can just extract it:

return [
    new Flarum\Extend\Listener(function (DiscussionWasSaved $event) {
        app(BusinessLogic::class)->doSomething($event->discussion);
    }),

    // or...

    function (BusinessLogic $logic) {
        return new Flarum\Extend\Listener(function (DiscussionWasSaved $event) use ($logic) {
            $logic->doSomething($event->discussion);
        })
    }
];

[luceos]

@tobscure this proposal will also allow for service providers to be added, are you considering adding an Extend'er for that as well? That might be really helpful for the more complex extensions that want to stay close to the Laravel way of handling packages.

[tobyzerner]

@Luceos Sure!

[tobyzerner]

Latest proposal: https://gist.github.com/tobscure/5e514a700dcb471180369903ab4562c3
Except we won't have schema stuff until 0.2, so:
https://gist.github.com/tobscure/a07da0ca4e80ac7de640d368c3d36ce0
https://gist.github.com/tobscure/4d0449a9563675664294463423923659

[franzliedke]

@tobscure Slightly confused by your last comment... Do you want the instance- or class-based API now?

[tobyzerner]

I think this one will be cleaner to implement and document, because we're separating distinct functions into distinct classes.

But the new version of tobscure/json-api which automates API building with "Schema" won't be ready until 0.2. Maybe somehow we can abstract it away though so we don't break the API in-between...

[tobyzerner]

@franzliedke Okay, I've turned extenders into invokables, as you suggested. This basically renders the Compat extender obsolete, right?

Yay! Looks nice. Yes, the Compat extender is obsolete.

[franzliedke]

And Compat is gone. :smile:

[tobyzerner]

So extensions literally do not need to change now? Not even wrap their function in an array? ie. this should work:

```php
// bootstrap.php
return function (Dispatcher $events) {
// ...
};

[franzliedke]

Probably yes. That was not intended, was it? 😏

Let's not forget that most extensions will need to adapt to all the namespace changes, so there is plenty reason to switch to extenders right away as well (especially as they remove quite a lot of boilerplate).

[tobyzerner]

Yes I was hoping it would work, because I want to be able to say "at its most basic, an extension is just a function" rather than "an array of functions" :D

And then we can do cool one-liners like this:

return new Extend\Locale(__DIR__);

But agreed that we will want to convert to extenders right away, and encourage third-party extensions to do the same. This just eases the transition.

[franzliedke]

Turns out it doesn't work - Laravel's Container::call does not work with invokables.

[tobyzerner]

This makes me sad :( Do you think there any scope to PR support for Container::call and invokables into Laravel? It can be done.

Or can we just add a check when running the extenders ourselves? (can confirm that this works)

foreach ($extenders as $extender) {
    if (is_object($extender)) {
        $app->call([$extender, '__invoke']);
    } else {
        $app->call($extender);
    }
}

Sorry for the back-and-forth 😬

[franzliedke]

We can try sending a PR to Laravel.

However, I am quite happy with not calling call on every extender (only the compat one does it itself). I don't trust the performance of all that reflection jazz...

(And yes, such a statement should be backed by a benchmark.)

[tobyzerner]

@franzliedke Currently I'm getting this with the latest commit on everything:

Fatal error: Uncaught TypeError: Argument 1 passed to Flarum\Extension\ExtensionManager::{closure}() must implement interface Illuminate\Contracts\Events\Dispatcher, instance of Flarum\Foundation\Application given, called in /Users/toby/Projects/Flarum/packages/core/src/Extension/ExtensionServiceProvider.php on line 30 and defined in /Users/toby/Projects/Flarum/packages/flarum-ext-akismet/bootstrap.php:22

Is there something you haven't pushed?

[franzliedke]

@tobscure Fixed that, sorry for the mishap.

[luceos]

Oh I was worried this was my doing.. @franzliedke could you take a look at #1346, it would have caught this 😊

[luceos]

I'm preparing bazaar for this. But I can't seem to find a way to load the locale without it being seen as a language pack (Language pack locale must define "extra.flarum-locale.code" in composer.json. in core/src/Extend/Locale.php on line 52).

Each 3rd party extension has its own language file(s) because it's not part of core, we would need to support this as well, eg on the Assets extender. Here's my result so far:

<?php

namespace Flagrow\Bazaar;

use Flagrow\Bazaar\Api\Controllers;
use Flarum\Extend\Assets;
use Flarum\Extend\Locale;
use Flarum\Extend\Routes;
use Flarum\Foundation\Application;
use Illuminate\Contracts\Events\Dispatcher;

return [
    (new Routes('admin'))
        ->get('/bazaar/extensions', 'bazaar.extensions.index', Controllers\ListExtensionController::class)
        ->post('/bazaar/extensions', 'bazaar.extensions.install', Controllers\InstallExtensionController::class)
        ->patch('/bazaar/extensions/{id}', 'bazaar.extensions.update', Controllers\UpdateExtensionController::class)
        ->patch('/bazaar/extensions/{id}/toggle','bazaar.extensions.toggle', Controllers\ToggleExtensionController::class)
        ->post('/bazaar/extensions/{id}/favorite','bazaar.extensions.favorite',Controllers\FavoriteExtensionController::class)
        ->get('/bazaar/redirect/subscribe/{id}','bazaar.redirect.subscribe',Controllers\SubscriptionRedirectSubscribeController::class)
        ->get('/bazaar/redirect/unsubscribe/{id}','bazaar.redirect.unsubscribe',Controllers\SubscriptionRedirectUnsubscribeController::class)
        ->get('/bazaar/callback/subscription', 'bazaar.callback.subscription', Controllers\SubscriptionRedirectCallbackController::class)
        ->delete('/bazaar/extensions/{id}', 'bazaar.extensions.delete', Controllers\UninstallExtensionController::class)
        ->get('/bazaar/connect', 'bazaar.connect',  Controllers\ConnectController::class)
        ->get('/bazaar/tasks', 'bazaar.tasks.index', Controllers\ListTaskController::class)
        ->get('/bazaar/sync/composer-lock', 'bazaar.composer-lock', Controllers\RetrieveComposerLockController::class)
        ->get('/bazaar/sync/extensions/{id}/version', 'bazaar.extensions.version', Controllers\RetrieveExtensionVersionController::class),
    (new Assets('admin'))
        ->asset(__DIR__ . '/resources/less/extension.less')
        ->asset(__DIR__ . '/js/admin/dist/extension.js')
        ->bootstrapper('flagrow/bazaar/main'),
    (new Locale(__DIR__.'/resources/locale')),
    function (Dispatcher $events, Application $app) {
        $events->subscribe(Listeners\BazaarEnabled::class);
        $events->subscribe(Listeners\AddApiAttributes::class);
        $events->subscribe(Listeners\AddSatisConfiguration::class);
        $events->subscribe(Listeners\SyncWasSet::class);
        $events->subscribe(Listeners\SyncVersion::class);

        $app->register(Providers\ComposerEnvironmentProvider::class);
        $app->register(Providers\ExtensionProvider::class);
        $app->register(Providers\ConsoleProvider::class);
    }
];

[franzliedke]

I've finished the transition from ConfigureForumRoutes event (and the like for admin and API) to the (recently introduced) Routes extender in all bundled extensions.

On to making new extenders now! :smile:

@luceos Thanks for the heads-up. So this means we have two extender use-cases here: registering a locale (a language pack, basically) and translations (files that house locale data), right? I'll have to think whether we want this to be realized in the same extender, or in different ones. Any opinions?

[franzliedke]

@luceos This is now done.

For your extension, this should now do the trick:

~php
new Extend\Locales(__DIR__.'/resources/locale')
~

[luceos]

Still hoping for Extend\EventListener to be added, I can do that as well. Lots of extension use a lot of listeners :)

[franzliedke]

@luceos I'd like to avoid that. There are still lots of extenders that have to be implemented, which - hopefully - make all events obsolete. (They will still be there behind the scenes in some of the cases, but should be considered private API.)

[tobyzerner]

@flarum/core With the new way of hooking up frontend controllers, it might make sense to convert Extend\Assets into Extend\Frontend. Example:

Old:

return [
  (new Extend\Assets('forum'))
    ->js('file')
    ->css('file'),

  (new Extend\Routes('forum'))
    ->get('/foo', 'foo', Extend\Routes::toFrontend('flarum.forum.frontend'))
    ->get('/bar', 'bar', Extend\Routes::toFrontend('flarum.forum.frontend', Content\Discussion::class))
    ->post('/register', 'register', MyRegisterController::class)
];

New:

return [
  (new Extend\Frontend('forum'))
    ->js('file')
    ->css('file')
    ->route('/foo', 'foo')
    ->route('/bar', 'bar', Content\Discussion::class),

  (new Extend\Routes('forum'))
    ->post('/register', 'register', MyRegisterController::class)
];

Non-GET routes would still be defined via ExtendRoutes.

[luceos]

I dislike the route method here, especially because it only allows for get requests. Either continue this fluent setting by adding routes(), eg:

return [
  (new Extend\Frontend('forum'))
    ->js('file')
    ->css('file')
    ->routes(function ($routes) {
        $routes->get('/foo', 'foo');
        $routes->get('/bar', 'bar', Content\Discussion::class);
    })
];

Or stick to the old behavior, otherwise you would get too much confusion of what is the de-facto standard.

I am in favour of using Frontend vs Assets though, it sounds way more fitting in the context.

[tobyzerner]

In the context of a frontend (i.e. hooking up a FrontendController with Content, which will set up a frontend view), you are only able to make GET routes. The other methods don't have anything to do with the frontend, because you'll hook them up to your own controllers. Hence why a single route method on the Frontend extender would make sense. I have amended my example to try and make this clear.

[tobyzerner]

@franzliedke Would like your thoughts on this please. Currently hitting /tags results in an error because we don't have an API to hook up frontend routes.

[franzliedke]

@tobscure Good thoughts. Done in d4a80ea. (Will push the changes to all extensions now.)

[franzliedke]

Hmm, while working on the necessary changes to the embed extension as proposed by Toby here, I noticed that this new Frontend extender causes another type of conflict: it is currently used to register things like assets and routes for existing frontends. For the embed extension, we need to register a new frontend (or, at least, a new asset group and layout). The route, on the other hand, still needs to be registered with the "forum" frontend, whereas it renders the "embed" frontend. Quite confusing. :confused:

Can we clear this up by improving terminology?

[sijad]

I already pointed it out in chats, I also think it'd be better if embed get separated from forum (like admin), it will decrease size of embed bundle and API would be cleaner to developers how extend forum without being worry about side effects in embed.

[tobyzerner]

Well no, we want developers to be able to extend forum and have the same changes automatically show up in embed. The whole point is that embed is forum with minor modifications (hide header, sidebar, disallow navigation). Plus, it uses all of the DiscussionPage + related code (which is a lot), and having to duplicate that and keep it synced would be extremely un-DRY.

[sijad]

I didn't mean having duplicate code, I thought it's possible share codes between scopes, something similar to common folder.

[tobyzerner]

How would we share JS source between core and an extension?

[sijad]

sorry, you're right, I didn't noticed embed is an extension.

[sijad]

on second thought, it's possible to share Js between core and an extensions:

import { DiscussionPage } from 'flarum';

app.initializers.add('sijad-pages', app => {
  app.routes.embedDiscussion = {path: '/embed/d/:id', component: DiscussionPage.component()};
  // ...
});

am I missing something here?

[tobyzerner]

Importing from "flarum" relies on the flarum global being available, ie. forum.js must be included. Take a look at externals in flarum-webpack-config.

[franzliedke]

I noticed that this new Frontend extender causes another type of conflict: it is currently used to register things like assets and routes for existing frontends. For the embed extension, we need to register a new frontend (or, at least, a new asset group and layout).

@tobscure Can you share your thoughts on this? I believe having new Extend\Frontend($name) behave differently depending on whether the frontend identified by $name exists or not is a rather confusing API.

I see two possibilities here:

1. Split the two use-cases into separate extenders. How would we name them, though? And they would do a few things twice (registering concrete assets for the frontends). :confused:
2. (currently my favorite) Come up with two different named constructors for the extender. Extend\Frontend::existing($name) and Extend\Frontend::new($name, $extendsFrom). The problem here: How to name the second one? new is not allowed, and I'd like to see the name communicate that we are - optionally - extending another frontend, and which parameter describes the frontend that is being extended and which describes the new one.

[luceos]

for 2:

• create
• make
• instantiate
• publish

[tobyzerner]

I think we should maintain the status quo (new Extend\Frontend) for the most common use-case which is extending an existing frontend. Every extension developer will have to do this, while most will never have to even think about creating a new frontend.

To create a new frontend, I like the static method as suggested by @franzliedke. Since the inheritance is optional, we could make it a new method? Perhaps:

return [
    (new Extend\Frontend('forum')) // extend an existing frontend
        ->css('file'),

    Extend\Frontend::create('embed') // create a new frontend
        ->inherit('forum')
        ->css('file')
];

Hmm actually, I don't even know if the static method is necessary...

behave differently depending on whether the frontend identified by $name exists or not is a rather confusing API.

It wouldn't really would it? All it would do is setup a binding in the container if one doesn't already exist.

[tobyzerner]

Actually - not even that is going to work. If an extension gets installed after Flarum has been installed, all of its migrations will be run. So extension migrations need to be kept up-to-date to work with the current core schema at all times, and then run after core migrations. Therefore, the addPermissions helper shouldn't cause any problems when used by extensions. Does that make sense?

[franzliedke]

Your last comment refers to https://github.com/flarum/core/pull/1344#discussion_r212527432, right?

[sanjucta]

Hi ,

Leaving this comment here as advised by Franz in this issue

It seems that in beta7 we can't use ConfigureForumRoutes to overwrite and existing route. I was trying to do something like

public function reRouteLogin(ConfigureForumRoutes $event)
{
$event->post('/login', 'login', Controllers\MyLogInController::class);
}

That results in an exception -
FastRoute\BadRouteException: Cannot register two routes matching "/login" for method "POST" in C:\wamp64\wwwflarum\vendor\nikic\fast-route\src\DataGenerator\RegexBasedAbstract.php on line 55

[franzliedke]

Closing this, as the main infrastructure and quite a few extenders are already in place. I will create separate issues for the remaining extenders and assign them:

• to beta.8 if they're still needed to fix problems or because their corresponding events have been removed / deprecated
• to beta.9 otherwise.