-
-
Notifications
You must be signed in to change notification settings - Fork 834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New PHP Extension API #851
Comments
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. 👍 |
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: Event listener boilerplateIMO, 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 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 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. ShortcutsHow 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 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:
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
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:
Thoughts? (I hope that was understandable.) |
A small counterpoint, just for the record – I see two advantages to only dealing with events conceptually:
Anyway, back to the real discussion at hand... Static Factory MethodsI 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 ( 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 FilesThis is a nice idea, but I have a couple questions:
Traits RevisitedThe 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.
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 // bootstrap.php
return [
new Flarum\Extend\Listener(function (DiscussionWasStarted $event) {
// do something with $event->discussion
})
]; For the 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 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 |
@luceos Sure! |
Latest proposal: https://gist.github.com/tobscure/5e514a700dcb471180369903ab4562c3 |
@tobscure Slightly confused by your last comment... Do you want the instance- or class-based API now? |
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?
The text was updated successfully, but these errors were encountered: