Skip to content
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

Finish PHP extenders #1891

Open
franzliedke opened this issue Sep 23, 2019 · 6 comments
Open

Finish PHP extenders #1891

franzliedke opened this issue Sep 23, 2019 · 6 comments
Labels
Milestone

Comments

@franzliedke
Copy link
Member

@franzliedke franzliedke commented Sep 23, 2019

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.

  • API: Schema (Attributes / Relationships / Serialize) (#1624)
  • API: load data
  • Discussions: Deleted
  • Discussions: Started
  • Discussions: Saving
  • Event listeners for extension events
  • Formatter: rendering
  • Frontend: overwrite existing route (proposal on Discuss)
  • Middleware: registration (#1919)
  • Model: default attributes
  • Model: relationship
  • Model: date attributes
  • Notification: Sending
  • Notification: types
  • Policies: e.g. get permission on model
  • Posts: Revised
  • Posts: Query
  • Posts: Posted
  • Posts: Deleted
  • Posts: Restored
  • Posts: Hidden
  • Posts: Deleting
  • Posts: Saving
  • Posts: types
  • Search: reorder (custom logic)
  • Search: gambit
  • User: preferences
  • User: saving
  • User: prepare user groups
  • User: avatar (#1709)
  • View: Namespace
  • Core: Error handlers (#1781)

Where applicable, existing issues have been or will be linked - these will have examples and explain the full use-case.

Most of them come from Toby's original gist.

Other related issues: #1223

@franzliedke franzliedke added this to the 0.1.0-beta.11 milestone Sep 23, 2019
@franzliedke franzliedke pinned this issue Sep 23, 2019
@luceos

This comment has been minimized.

Copy link
Member

@luceos luceos commented Sep 25, 2019

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

This comment has been minimized.

Copy link
Member Author

@franzliedke franzliedke commented Sep 25, 2019

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

@luceos

This comment has been minimized.

Copy link
Member

@luceos luceos commented Sep 26, 2019

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

This comment has been minimized.

Copy link

@cmcjacob cmcjacob commented Oct 14, 2019

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

This comment has been minimized.

Copy link
Member Author

@franzliedke franzliedke commented Oct 26, 2019

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

@luceos luceos modified the milestones: 0.1.0-beta.12, 0.1 Dec 6, 2019
@franzliedke

This comment has been minimized.

Copy link
Member Author

@franzliedke franzliedke commented Jan 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.