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

[Proposal] Internal events #2506

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@alanpoulain
Copy link
Member

commented Feb 10, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? ?
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Alternative to #2329.

The aim is the same as the other PR but also to decouple the event system from Symfony as much as possible (to replace it easily with Laravel for instance).

The listeners in ApiPlatform\Core\EventListener listen to our internal events instead of the Symfony ones.

The Symfony events are listened by the EventDispatcher in the Bridge and dispatched as our events. It implies that the symfony/event-dispatcher dependency is now mandatory.

It will also simplify the part about priorities (https://api-platform.com/docs/core/events/#the-event-system) since the user has now to only listen to our events.

For existing applications, it should not change anything since listening to Symfony events (with priority) is equivalent to listening to our internal ones. For the listeners should we add deprecations about using Symfony events?

WDYT? @api-platform/core-team?

final class Events
{
public const PRE_READ = 'api_platform.pre_read';
public const READ = 'api_platform.read';

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 10, 2019

Author Member

This event will not be used by GraphQL and is internal. Should we expose it elsewhere? Should we call it something else (api_platform.rest.read)?

This comment has been minimized.

Copy link
@vincentchalamon

vincentchalamon Feb 10, 2019

Contributor

WDYT about keeping this event name, and dispatch a api_platform.query event for GraphQL?

This comment has been minimized.

Copy link
@vincentchalamon

vincentchalamon Feb 10, 2019

Contributor

I don't understand how the api_platform.read event could be only internal and not listened by any listener in a project

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 10, 2019

Author Member

For GraphQL, there will be no event like this dispatched (only pre and post). GraphQL is working differently. Using query does not work: there is a read step in the mutation resolver for example.
api_platform.read could be listened, but it shouldn't since it is used by our ReadListener.

@@ -52,9 +51,9 @@ public function __construct(CollectionDataProviderInterface $collectionDataProvi
*
* @throws NotFoundHttpException
*/
public function onKernelRequest(GetResponseEvent $event)
public function handleEvent(EventInterface $event)

This comment has been minimized.

Copy link
@vincentchalamon

vincentchalamon Feb 10, 2019

Contributor

This is a BC break, as you change a public method definition (for example, if any project overrides this service using service decoration)

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 10, 2019

Author Member

Yes sure. Maybe I will add a deprecation. It's too related to Symfony right now IMO.

@vincentchalamon

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

I like the idea of this bridge of the EventDispatcher, it makes API Platform more flexible and compatible with other systems (such as Laravel as you said).

For the listeners should we add deprecations?

Do you mean adding deprecations about listening to Symfony events in an API Platform project?

@Taluu

Taluu approved these changes Feb 10, 2019

switch ($event) {
case $event instanceof GetResponseEvent:
$internalEvent = new $this->eventClass(null, ['request' => $event->getRequest()]);

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 10, 2019

Contributor

How about putting the original event in the internal event ?

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 10, 2019

Author Member

Not sure about this. I want to decouple from Symfony as much as possible. What is the use case?

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

The GetResponseEvent from Symfony has a response part which is not in the internal event, it could be useful to have it here.

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 11, 2019

Author Member

Yes but it's only to set the response and bypass everything else. I'm not sure we want this in API Platform, do we? If there are a "response" (controller result) and a request (in GetResponseForControllerResultEvent for instance), I will add them both to the context.

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

Yup, but then what if we get an exception event (GetResponseForExceptionEvent I think ?), and we want to get the thrown exception ? Same for all the other sub events.

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 11, 2019

Author Member

I will add an exception field in the context then. IMO we want as much information as the Symfony event but we don't want to use the event itself since it's too tight to Symfony.

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

Then the case should be split accordingly depending on the event it handles (exception, controller result, ... and so on)

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

@vincentchalamon yes I mean that for the deprecations.

@ragboyjr

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

@alanpoulain this is huge 👍 from me. One of the main things that confused me when first looking over API platform was that it felt a bit hackish if we wanted to extend specific API platform events because it was based on ensuring priority after api platform event listeners on SF events.

This seems like it will not only make API Platform easier to fit on other popular frameworks, but will also make extending API platform specific events easier for users.

$internalEvent = new $this->eventClass(null, ['request' => $event->getRequest()]);
}
$this->dispatcher->dispatch($this->eventName, $internalEvent);

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

if the internal event is null, how about dispatching the given event instead ?

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 11, 2019

Author Member

Why would it be null? I'm thinking of throwing an exception instead (it would mean the service is not configured correctly).

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

As this service is kinda decorating the event dispatcher, if you want to dispatch other events, you will dispatch a null event instead. I think this can be a huge source of bugs or miscomprehension.

Unless this service is really for internal...

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Feb 11, 2019

Author Member

Yes I think it's really internal. If a user want to do something like this, it can do it itself. Maybe adding a @internal for the class would be a good idea.

Show resolved Hide resolved src/Bridge/Symfony/Bundle/EventListener/EventDispatcher.php Outdated
switch ($event) {
case $event instanceof GetResponseEvent:
$internalEvent = new $this->eventClass(null, ['request' => $event->getRequest()]);

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

Then the case should be split accordingly depending on the event it handles (exception, controller result, ... and so on)

private $eventClass;
private $dispatcher;
public function __construct(string $eventName, string $eventClass, EventDispatcherInterface $dispatcher)

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

so this dispatcher can handle only one event ? you would have for X events (read, write, ... and so on) X declared services ? Wouldn't that be a mess ? :O

public function __construct(string $eventName, string $eventClass, EventDispatcherInterface $dispatcher)
{
$this->eventName = $eventName;
$this->eventClass = $eventClass;

This comment has been minimized.

Copy link
@Taluu

Taluu Feb 11, 2019

Contributor

not having any check if the event class given is valid is a foot gun IMO

@dunglas

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

I'm wondering if we the latest addition in the library, we still need an event system. If we look at the diagram created with @soyuka (we'll provide an updated version soon), we already have extension point everywhere we need.

image

Maybe could we just remove the event system from the docs (it will become purely internal), and document how to hook custom logic during the execution flow:

  1. custom data provider
  2. custom deserializer
  3. custom validator
  4. custom data persister
  5. custom serializer

It's simple, covers all use cases, and is compatible with Laravel, PSR-7 middleware and so on. WDYT?

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@dunglas Why not. But we will need to keep the event listeners anyway, don't we? And it's better if they are decoupled from Symfony.

And I'm not sure if creating a custom persister for instance is a good idea for having a side-effect (send a mail for example). Are you sure it's easier than creating an event listener?

Also I don't think it's only documentation. We need a ChainValidator for example don't we?

@dunglas

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

But we will need to keep the event listeners anyway, don't we?

We'll always need to plug on the Symfony kernel, anyway.

Are you sure it's easier than creating an event listener?

I'm not sure, it's just some thought to start the discussion :)

We need a ChainValidator for example don't we?

Not really, the user can create custom validation constraints by using the Symfony component, or implement the ValidatorInterface himself if he (or she) wants too.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

We'll always need to plug on the Symfony kernel, anyway.

Sure but it will be done in the bridge, and could be easily replaced with another listener. Not the case for the current event listeners because there are too tight to Symfony.

I think it's really easier to use events for side-effets than having to deal with custom services. I think both methods are complementary.

@soyuka

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

And I'm not sure if creating a custom persister for instance is a good idea for having a side-effect (send a mail for example). Are you sure it's easier than creating an event listener?

To me this is a job for the messenger.

I think it's really easier to use events for side-effets than having to deal with custom services. I think both methods are complementary.

Yes but using too many events can become a mess pretty fast.

Updated schema with transformers:

api-platform-put-i-o

IMO we should try to deprecate the use of events as much as possible. Of course we'd have to keep symfony events but I'm not in favor of adding internal events like this. If we look at the above schema, the end developper should be able to hook using our concepts (persister, provider, transformer) for most of the use cases.
If the use case is really specific, events may be a good solution but they should not be the developer first choice, which is what internal events tend to give.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

To me this is a job for the messenger.

All projects don't always use the Messenger component whereas events are pretty common.

Of course we'd have to keep symfony events but I'm not in favor of adding internal events like this.

If we keep the Symfony events, it's always better to decouple our event listeners from Symfony. This is also the aim of this PR.

If the use case is really specific, events may be a good solution but they should not be the developer first choice, which is what internal events tend to give.

Why? Listening to our internal events or listening to the Symfony events, I don't really see the difference. If a developer wants to do it, it will always find a way. We can outline in the documentation that this shouldn't be the first choice, but that's all.

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch from 2e69d85 to 3357370 Feb 15, 2019

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch from 3357370 to 586872d Feb 27, 2019

@jamesisaac

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

And I'm not sure if creating a custom persister for instance is a good idea for having a side-effect (send a mail for example). Are you sure it's easier than creating an event listener?

To me this is a job for the messenger.

This is over complicating things IMO. I think it's great that with API Platform it's so simple to get a REST and/or GraphQL API up and running by simply defining the entities and a bit of configuration via annotations. But unlike similar tools which also provide these capabilities (Prisma, PostGraphile, etc), you can also very easily hook in custom business logic (e.g. side effects like sending email) through Symfony's events system. Deploying to something like Heroku is then just a few clicks away.

If I understand the Messenger component correctly, you're now talking about orchestrating multiple projects/services, replacing function calls with inter-process message queues. Sure, there's a place for that (bigger projects with lots of devops resources), but for rapidly shipping MVPs, the single project "monolith" approach greatly simplifies things. IMO, API Platform is currently the best in class for this use-case, and GraphQL events is one of the only gaps in offering full flexibility (as I experienced in #2167).

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch from 586872d to 4c54984 Mar 2, 2019

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch from 4c54984 to b7dbb8f Mar 2, 2019

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

I'd suggest we stick to semantical events, in this case CRUD.

The goal should not be to replicate our existing event listeners / priorities, but to provide hooks where they are valuable and have well-defined semantics.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

@teohhanhui I don't fully grasp what you say. My aim is to have our own events (to be decoupled from Symfony). And with them, users can add hooks if they want.

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

What I mean is, if we're going to have custom events, then those events should have well-defined semantics, not something like "add format" / "deserialize" which is merely an implementation detail. Having CRUD events would be good.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

I'm not sure what you mean by CRUD events. We need a way for the user to modify things before or after our listeners. And we need these events for ourselves too.

@soyuka

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

He probably means that the user should be able to hook to:

PRE_CREATE
POST_CREATE
PRE_READ
POST_READ
PRE_UPDATE
POST_UPDATE
PRE_DELETE
POST_DELETE

This is independent from the implementation, and by the way I've reviewed my thoughts about having internal events and I think it might be good to detach these from symfony.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

Adding these events later should be really easy with this implementation (just more events to dispatch).
But they can't replace the already existing events.

{
public function getData();
public function setData($data): void;

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 11, 2019

Member

If we introduce our own event system, I would like to use immutability (as in our metadata system).

public function handleEvent(/*EventInterface */$event)
{
if ($event instanceof EventInterface) {
$request = $event->getContext()['request'];

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 11, 2019

Member

This is a hard coupling to Symfony. We should let the user injecting RequestStack instead of populating this key.

public const FORMAT_ADD = 'api_platform.format_add';
public const PRE_READ = 'api_platform.pre_read';
public const READ = 'api_platform.read';

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 11, 2019

Member

Couldn't we only have the PRE ad POST events?

This comment has been minimized.

Copy link
@alanpoulain

alanpoulain Mar 11, 2019

Author Member

I was thinking that PRE and POST events were for the user whereas the other ones were for our internal use only.

@ragboyjr

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@jamesisaac Regarding using messenger, it's just used as a command bus in here and doesn't necessarily need to be hooked up to an actual async sending system.

@Deuchnord Deuchnord referenced this pull request Apr 1, 2019

Closed

WIP: Add events #2329

4 of 5 tasks complete
@teohhanhui

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

We need a way for the user to modify things before or after our listeners. And we need these events for ourselves too.

These listeners are supposed to be the integration with the Symfony framework. We should extract out all logic from the listeners, instead of going the other way round. So I'm strongly -1 for having implementation-specific events.

It will really tie our hands in the future, as we will need to keep supporting events that make no sense anymore, as our implementation evolves. Leaking implementation details is always a bad thing, and this includes having events that are so closely tied to our current implementation. That's what I meant by "semantics". These events are meaningless beyond a specific implementation.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Not sure about your position. For me, API Platform is based on events and will always relies on an event system. No need to extract the logic from the listeners, we only need them to be decorrelated from the Symfony framework.

It will tie our hands in the future.

I don't see why. Our hands are tied today because we are too coupled to Symfony.

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

The way to decouple from Symfony framework, is to extract all logic out from listeners - "thin listeners". Same thing that we do for controllers - "thin controllers". Then we can integrate with different frameworks by reusing the services that we have. Listeners are just glue code.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

You can consider this PR as a first step to do this then? First we decouple, then we extract the logic?

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

No, we decouple by extracting the logic, not by creating implementation-specific events. This is completely going in the wrong direction. I've explained in detail above. So I have nothing more to add.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I'm not convinced then and I think this PR is worth finishing. And with it GraphQL will have an event system.

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I'm not against having our own events. In fact, it'll be wonderful. If the events have well-defined semantics and decoupled from implementation, then I have no objections.

I gave the example of CRUD events, as those are the most obvious (and probably already will provide enough flexibility anyway). But we could come up with other well-defined events. Taking our existing listeners implementation and creating events from those is the opposite of that.

@alanpoulain

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

You're probably right but I don't want to refactor the planet with this PR. Couldn't it be done in another PR once this one is merged?

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch 3 times, most recently from 890ce6d to 6c8b29f Apr 6, 2019

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch 5 times, most recently from fc607d8 to cdc9608 Apr 7, 2019

Internal events
Co-authored-by: ArnoudThibaut <thibaut.arnoud@gmail.com>

@alanpoulain alanpoulain force-pushed the alanpoulain:internal-events branch from cdc9608 to a0ecc42 Apr 7, 2019

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