Closed tobyzerner closed 6 years ago
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?
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.
In fact, it's awesome. :+1:
I discussed this with @tobscure, here's a quick summary:
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) {})
];
Hmm, good point. I'll think about it, too.
Some brainstorming on the Extenders idea:
// bootstrap.php
return [
new Extend\AddDefaultClientAssets(),
include 'addDiscussionsTagsRelationship.php'
];
// addDiscussionsTagsRelationship.php
return [
// more extenders here...
];
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'
];
};
return [
new Extend\EventListener(function (DiscussionWasSaved $saved) use ($extension) {
// Do something with $saved->discussion
$whatever = $extension->getSettings()->get('whatever');
})
];
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')
];
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 flarum/issue-archive#353). 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]
])
];
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
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:
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 listen
ing 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.
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:
AddTheTagsRelationshipToTheDiscussionModel
).Disadvantages:
Open questions:
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. :)
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:
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:
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.)
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:
Flarum\Event
).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...
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.
This is a nice idea, but I have a couple questions:
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.
@franzliedke Do you have any further thoughts here?
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\Extend
ers. 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);
})
}
];
@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.
@Luceos Sure!
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
@tobscure Slightly confused by your last comment... Do you want the instance- or class-based API now?
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...
@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.
And Compat
is gone. :smile:
So extensions literally do not need to change now? Not even wrap their function in an array? ie. this should work:
// bootstrap.php
return function (Dispatcher $events) {
// ...
};
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).
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.
Turns out it doesn't work - Laravel's Container::call
does not work with invokables.
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 😬
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.)
@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?
@tobscure Fixed that, sorry for the mishap.
Oh I was worried this was my doing.. @franzliedke could you take a look at flarum/framework#1346, it would have caught this 😊
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);
}
];
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?
@luceos This is now done.
For your extension, this should now do the trick:
new Extend\Locales(__DIR__.'/resources/locale')
Still hoping for Extend\EventListener
to be added, I can do that as well. Lots of extension use a lot of listeners :)
@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.)
@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 Extend\Routes.
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.
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.
@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.
@tobscure Good thoughts. Done in d4a80ea. (Will push the changes to all extensions now.)
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?
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
.
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.
I didn't mean having duplicate code, I thought it's possible share codes between scopes, something similar to common folder.
How would we share JS source between core and an extension?
sorry, you're right, I didn't noticed embed is an extension.
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?
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.
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:
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.for 2:
create
make
instantiate
publish
In every extension, we have an AddClientAssets listener which is basically the same:
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:you would use:
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.:
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?