WIP: Add events

[Deuchnord]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2167, api-platform/api-platform#734
License	MIT
Doc PR	later

Adding events to make API Platform’s actions tweaking easier.

Todos:

• Add Pre- and Post- event for each action (based on the \ApiPlatform\Core\EventListener\* listeners)
• Wire the actions to the events
• Wire GraphQL to the events
• Write unit tests
• Write Behat tests

[alanpoulain]

Thanks for working on this 🙂

[Deuchnord]

Guess there will be an issue here, since the $operationName from GraphQL (create, update, delete) is not the same as the HTTP methods used in REST

[antograssiot]

Would it be useful too to have PreValidate and PostValidate events to keep the same logic than in REST ?

core/src/GraphQl/Resolver/Factory/ItemMutationResolverFactory.php

Lines 130 to 134 in d678481

	try {
	$this->validator->validate($item, ['groups' => $validationGroups]);
	} catch (ValidationException $e) {
	throw Error::createLocatedError($e->getMessage(), $info->fieldNodes, $info->path);
	}

[antograssiot]

wouldn't it be better to change this block to

try {
    if (null !== $this->dispatcher) {
        $this->dispatcher->dispatch(PreReadEvent::NAME, new PreReadEvent(null));
    }
    $item = $this->iriConverter->getItemFromIri($args['id'], $baseNormalizationContext);
} catch (ItemNotFoundException $e) {
    $item = null;
}

if (null !== $this->dispatcher) {
    $this->dispatcher->dispatch(PostReadEvent::NAME, new PostReadEvent($item));
}

if (null === $item) {
    return null;
}

to ensure that the PostReadEvent is always sent like in the ReadListener ?

[teohhanhui]

I don't think it should be dispatched when Read has failed.

[dunglas]

I think we can restrict the events to:

• read
• deserialize
• write
• validate
• serialize

Other ones are Symfony specific and should be handled using Symfony kernel events. WDYT?

[Deuchnord]

I think we can restrict the events to:

• read
• deserialize
• write
• validate
• serialize

Other ones are Symfony specific and should be handled using Symfony kernel events. WDYT?

@dunglas Sound good, but I think we should keep the pre- and post- events.
NB: Since you requested changes on (Pre|Post)AddFormatEvent classes, I suppose they should be kept too, though.

[teohhanhui]

These events should be removed as they're not related to the resource.

[teohhanhui]

Please remove the "Add Format" events.

[teohhanhui]

This is a really weird thing to do. Did I miss the discussion on why this is done this way?

[Deuchnord]

You mean, defining a NAME constant in the classes?
I did it mainly for comfort purpose, actually: it makes more sense to me to use a constant attached to the concerned class, so I kept it after @dunglas asked me to create the separate Events class.
Do you want me to remove the NAME constants?

[teohhanhui]

I don't see the value of these constants. Just use e.g. Events::PRE_WRITE directly.

[teohhanhui]

You need to add the same on-invalid="null" in this file too.

[teohhanhui]

It's really weird to have data in Pre Read. I don't think it makes sense...

[teohhanhui]

Perhaps what we could do is to keep a reference to the event object, then check if the data is already set on the event. If it is set, we should not override it.

[jamesisaac]

Very glad this is being worked on.

Is the idea with not attaching the Request to these events (unlike their Symfony counterparts) that the Request can be pulled out manually from the RequestStack service? I don't think the current events on their own contain enough data that you'd be able to determine whether to run most real-world side-effects (normally I'm checking the method, collection/item operation name, or route name).

Also, with the current events system they're executed in a very predictable linear order. Am I understanding that due to the way GraphQL resolvers work (one entity at a time), these events can be executed more like 1..n times (or 0..n) per request if the graphql request contains some nesting? Just trying to think if that could create any awkward conditions where side-effects get executed multiple times.

[Deuchnord]

Hi, sorry for the late, I forgot to answer when I saw your comment ^^

Is the idea with not attaching the Request to these events (unlike their Symfony counterparts) that the Request can be pulled out manually from the RequestStack service? I don't think the current events on their own contain enough data that you'd be able to determine whether to run most real-world side-effects (normally I'm checking the method, collection/item operation name, or route name).

Actually, these events are supposed to be higher levelled than Symfony's ones, so they're meant to contain only the details about the data themselves. If you want to get details about the request itself, you'll probably want to intercept Symfony's events instead.

Also, with the current events system they're executed in a very predictable linear order. Am I understanding that due to the way GraphQL resolvers work (one entity at a time), these events can be executed more like 1..n times (or 0..n) per request if the graphql request contains some nesting? Just trying to think if that could create any awkward conditions where side-effects get executed multiple times.

That GraphQL part is actually quite cryptic to me, I need to study that more… if you have any tips to make my integration of the events better, feel free to share them 🙂

[jamesisaac]

No worries!

Actually, these events are supposed to be higher levelled than Symfony's ones, so they're meant to contain only the details about the data themselves. If you want to get details about the request itself, you'll probably want to intercept Symfony's events instead.

I thought one of the motivations here was that Symfony events aren't getting fired during the lifecycle of GraphQL requests (see #2167 )? So do you mean intercepting a combination of the 2? I.e. in a single listener, listen to Symfony's request event to store the route/method, then combine that with the data retrieved from this event, to execute the side effect?

I just think that individually, these events with "only the details about the data", and Symfony's events which don't have the controller result, don't contain enough information to add granular side effects in a lot of real world cases. Both are needed together most of the time.

[Deuchnord]

I thought one of the motivations here was that Symfony events aren't getting fired during the lifecycle of GraphQL requests (see #2167 )? So do you mean intercepting a combination of the 2? I.e. in a single listener, listen to Symfony's request event to store the route/method, then combine that with the data retrieved from this event, to execute the side effect?

Yes, that's what's planned ideally: an event that's risen, whenever the action comes from REST or GraphQL :)

I just think that individually, these events with "only the details about the data", and Symfony's events which don't have the controller result, don't contain enough information to add granular side effects in a lot of real world cases. Both are needed together most of the time.

I think I see what you mean, problem is, I'm not sure I can get that Symfony event, for the same reason we can't do it currently like described in #2167… We'll need to think more about it, maybe it would be a good thing to have an object that contain the information that may be useful and are available for both REST and GraphQL?

[jamesisaac]

When I wrote my own quick workaround for #2167 (master...jamesisaac:graphql-events), I just injected the RequestStack into the ItemMutationResolverFactory etc, and included the current request in the event object. so that it closely matched the REST events.

To me that solution added a lot of convenience, as the data alone doesn't tell you the context of how that data is being reached in the API (not even whether it's a query or a mutation). But I know you mentioned these events are meant to be higher level, so maybe there's a separation of concerns I'm missing out on.

[Deuchnord]

What about adding to all the events an object that would be common to both REST and GraphQL, and would contain the context of the events? This way, you would get as many information as possible, without having to take care about Symfony's events being actually risen or not.

Currently, I can see at least two things this event could store:

• the origin of the event (REST or GraphQL),
• the request type (creation, read, update or delete).

Plus, we could also append the old version of the data in the PostWriteEvent. Actually, it looks Doctrine doesn't allow getting the version before changing it from the Core (or maybe I didn't look well?)

WDYT? Do you see any other thing that could be interesting to add in such an object?

@dunglas and @teohhanhui, I'd be interested by your opinion about that too :)

[jamesisaac]

What about adding to all the events an object that would be common to both REST and GraphQL, and would contain the context of the events? This way, you would get as many information as possible, without having to take care about Symfony's events being actually risen or not.

That would be very helpful to me. In addition to your list, the things I make use of are:

• The entity being mutated (ControllerResult)
• The route name (e.g. when there are multiple POST actions defined with different semantics)

[Deuchnord]

Closing this PR since a better implementation is proposed on #2506.