Finish PHP extenders #1891
Comments
|
Looking at your list I am unsure how you want to move forward. It feels like adding extenders for all of them feels like we're bloating core. We could potentially even make the Events themselves smarter to allow for easier extensibility. Looking at Toby's gist I see a lot of possible unnecessary duplication, eg: new Extend\DiscussionGambit(TagGambit::class),Why not: new Extend\Gambit(Discussion::class, TagGambit::class),Maybe I'm completely oblivious to the exact plans. |
I think the more focused public API and the greater freedom to make internal changes without having to change the API for extensions is absolutely worth it. Once we have the public API, we can get rid of the events as implementation detail in many cases, which should pretty much reduce the bloat by the same amount we increased it.
Not sure I understand - can you expand on this? Maybe with an example?
Yeah. We can probably implement them as one. Unless something comes up that shows why Toby proposed them the way he did. 😉 |
|
Here's an example from tenancy where we've added a few helper methods on the event directly to simplify implementation for the developer: use Illuminate\Routing\RouteCollection;
use Illuminate\Routing\Router;
use Tenancy\Identification\Events\Switched;
class ConfigureRoutes
{
/**
* @var Switched
*/
public $event;
/**
* @var Router
*/
public $router;
public function __construct(Switched $event, Router $router)
{
$this->event = $event;
$this->router = $router;
}
/**
* Flush all tenant routes for this request.
*
* @return $this
*/
public function flush()
{
$this->router->setRoutes(new RouteCollection());
return $this;
}
/**
* Adds routes from a routes.php file to the current request.
*
* @param array $attributes
* @param string $path
*
* @return $this
*/
public function fromFile(array $attributes, string $path)
{
$this->router->group($attributes, $path);
return $this;
}
} |
|
I think there's a heavy boilerplate with the current state of relying so much on event listeners. They are implemented well, but come with a degree of importance that it can give the impression they Most of the helpers in this list sound beneficial as extenders, but it seems like some of them should be decoupled as officially enabled extensions, so kind of confused how core would be considered bloated from a supplicant feature. For example, why are there so many Post listeners described here? If all listeners of one type are handled in these extender helpers, one would expect other events to be as well be (unless I'm not visualizing the implementation correctly). Furthermore, if (hypothetically) 90% of events are left as blank containers and the other 10% are enhanced with app/logic helpers - could provide an unwanted layer of inconsistency that doesn't contrast well with the existing extender methods. I can understand why it's easier, afterall many of these helpers will rely on events themselves, but I also think these extenders would be more the more appealing option because of simplicity in design. |
|
@cmcjacob Most extenders will rely on the existing events for implementation in the beginning. But as soon as the extenders are there, extension developers will be expected to use those - events will then be considered private API. As you mention, some of the events are quite clunky for some of their use-cases, which is why we will likely change the implementation of extenders to something more direct (e.g. storing an array of middleware per "frontend" in the IoC container directly). |
|
For my own reference: I noticed that this GitHub code search is very useful to find out what extension developers are currently building custom extenders for. 😃 |
|
Please forgive me if this question is a bit uninformed, but is the goal of this to shift the vast majority of extension-flarum interaction to the extender interface (in effect, removing the necessity of listening into events)? Also, if that's the case, I noticed that there's a lack of extenders that have to do with logging in / registering. There are valid cases for actions to be executed after login (see https://github.com/askvortsov1/flarum-auth-sync), so I think that's something we should plan for. Essentially, I'm wondering how the extenders to have were selected, and to what extend this list is set in stone? |
Yes, pretty much. Whether they use events internally or not, is going to be an implementation detail and considered private API. Basically, the extenders (and what other classes the extenders reference and expose) will make up the public API of Flarum's backend.
Basically, I want all bundled extensions to be implemented fully in terms of extenders. They cover a variety of use-cases (experimenting with those use-cases was one of the reasons why the extensions were implemented so early), so we are hoping to cover enough ground with these. Additional input / requests on top are, of course, welcome, but I'd consider them in nice-to-have category.
It's not, see above. Especially the exact interfaces of the concrete extenders are up to discussion - best kept to concrete sub-tickets, though. Baseline for discussion would be this Gist by Toby, Flarum's founder, where he envisioned what the tags extension would look like if implemented using extenders. |
|
P.S.: I want extensions to mostly use extenders, not build their own. The latter has become a bit too common for my personal taste, but that's mostly due to our slow pace in creating more official extenders. 😉 |
I think that to organize work on this, it might be best to start this discussion and decide which ones we definitely want, as well as any kind of priority we want to do these in. |
|
We'll organize a call / chat meeting in early March, after beta.12 is released. Everybody who's interested, let us know and we'll give you a chance to join. |
|
For the event listener ones, what if we refactored all the created/deleted/saving/etc events out into model-agnostic ones, and created a general use AbstractLifecycleHandler, which extension devs could override and register via Extend\Model->addLifecycleHandler? The lifecycle handler could have methods ie onDeleting, onDeleted, onCreating, on Created, etc which, if defined by extension devs, would be subscribed onto the new model-agnostic events. This should simplify/add flexibility to the extender system, allow the per-model extenders (e.g. the User or Group or Post or Notification extenders) to focus on model-specific situations, and also fix #1223. Additionally, models could define the set of events that they support (e.g. User currently has no soft delete), which would tie everything together brilliantly. Additionally, do we want to somehow validate input into extender methods? |
Since introducing the extender API in beta.8, a bit of time has passed and we still need to add extenders for the following use-cases.
This list should cover what we need to implement all official, bundled extensions' backends only using existing extenders.
Remaining Post-Beta 15 Steps:
subscribeto event extender, but register subscribers only after application is fully booted to avoid early resolution.Extenders To Replace Events
At this stage, our goal is to replace the current event-based extension system for non-domain events, not to introduce new extensibility functionality (except where we can break current ambiguous events into more granular, use-driven extenders).
Extend\ApiSchema (PHP Extender: Serializing / forum api attributes #1624)
Extend\Console
Extend\Formatter (Refactor formatter extender #2098) @askvortsov1
Extend\Floodgate (Move floodgate to middleware, add extender + integration tests #2170) @askvortsov1
Extend\User
Extend\Policy
Extend\Search (Search Refactor and Extenders #2104) @askvortsov1
Extend\Routes
Extend\Locales
Extend\Middleware
Extend\Model
Extend\Notification
Extend\Post
Extend\Validation (Validator extender #2102) @askvortsov1
Events to Drop
Events To Keep
Discussion
Extension
Group
Post
Settings
User
OLD LIST
Most of them come from Toby's original gist.
Other related issues: #1223
The text was updated successfully, but these errors were encountered: