search

flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.39k stars 835 forks source link

Finish PHP extenders #1891

Closed franzliedke closed 3 years ago

franzliedke commented 5 years ago

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:

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).

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

luceos commented 5 years ago

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.

franzliedke commented 5 years ago

It feels like adding extenders for all of them feels like we're bloating core.

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.

We could potentially even make the Events themselves smarter to allow for easier extensibility.

Not sure I understand - can you expand on this? Maybe with an example?

Looking at Toby's gist I see a lot of possible unnecessary duplication

Yeah. We can probably implement them as one. Unless something comes up that shows why Toby proposed them the way he did. :wink:

luceos commented 5 years ago

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;
    }
}
cmcjacob commented 5 years ago

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
are the only option going forward.

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.

franzliedke commented 5 years ago

@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).

franzliedke commented 4 years ago

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. :smiley:

askvortsov1 commented 4 years ago

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?

franzliedke commented 4 years ago

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)?

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.

Essentially, I'm wondering how the extenders to have were selected...

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.

...and to what extend this list is set in stone?

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.

franzliedke commented 4 years ago

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. :wink:

askvortsov1 commented 4 years ago

It's not, see above. Especially the exact interfaces of the concrete extenders are up to discussion - best kept to concrete sub-tickets, though.

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.

franzliedke commented 4 years ago

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.

askvortsov1 commented 4 years ago

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 https://github.com/flarum/core/issues/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?