[Proposal] Add Stages by alanpoulain · Pull Request #2978 · api-platform/core

alanpoulain · 2019-08-09T17:48:38Z

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

As I've done with GraphQL (#2959), I would like to do the same for the REST part.

The aim is to create Stage services.

They will be responsible for all the stages (or steps) API Platform is doing to resolve a request.

They will be independent of the request and will be used by the event listeners.

A lot of benefits:

instead of using events and having issues with priorities (https://api-platform.com/docs/core/events/), the services could be decorated instead;
since they will be independent of events and the request, they could be used in different contexts (I'm thinking of Laravel for instance).

Right now I've only done the read stage. Behat tests are passing.

WDYT @api-platform/core-team?

dunglas

Good first step! I definitely agree with the goals and it's a move in the good direction. I made some comments to go one step further. I've the feeling that we can still improve the design a lot.

src/Exception/NotFoundException.php

dunglas · 2019-08-10T05:37:20Z

src/Stage/ReadStage.php

+use ApiPlatform\Core\Metadata\Resource\ToggleableOperationAttributeTrait;
+
+/**
+ * Retrieves data from the applicable data provider and sets it as a request parameter called data.


This description seems outdated

You think? It's basically what this stage does (excepted the set part which I can remove).

dunglas · 2019-08-10T06:02:03Z

src/Stage/ReadStage.php

+    /**
+     * {@inheritdoc}
+     */
+    public function apply(array $attributes, array $parameters, ?array $filters, string $method, array $normalizationContext)


Instead of this long list of unstructured and untyped parameters, shouldn't we introduce a new ApiRequest or ApiQuery/ApiCommand class(es), that we'll construct from the (Symfony/Laravel/PSR-7) HTTP request, or from the GraphQL query, or from any other source of request regarless of the protocol. It will allow to have a more strict typing, and will improve the evolvability of API Platform (in case we need to add more data over the time).

Basically, the responsibility of the Symfony Event Listener, or of the Laravel middleware, or of the GraphQL resolver would be to create this ApiRequest instance.

I've always wondered if it would be interesting to introduce this kind of class instead of having a lot of parameters and a context array with more things inside 😅
I'm 👍 for it.

dunglas · 2019-08-10T06:04:05Z

src/Stage/ReadStage.php

+            if (isset($attributes['item_operation_name'])) {
+                $data = $this->getItemData($identifiers, $attributes, $context);
+            } elseif (isset($attributes['subresource_operation_name'])) {
+                // Legacy


Why legacy? It's a valid use case to not inject a SubResourceDataPprovider (especially if you use the library directly, without the Symfony bridge).

Not my code here but I can remove the comment. The SubresourceDataProvider is mandatory in the GraphQL read stage.

dunglas · 2019-08-10T06:04:33Z

src/Stage/ReadStageInterface.php

+namespace ApiPlatform\Core\Stage;
+
+/**
+ * Retrieves data from the applicable data provider and sets it as a request parameter called data.


dunglas · 2019-08-10T06:06:46Z

src/EventListener/ReadListener.php


        $request->attributes->set('data', $data);
-        $request->attributes->set('previous_data', \is_object($data) ? clone $data : $data);
+        $request->attributes->set('previous_data', \is_object($data) && (new \ReflectionClass(\get_class($data)))->isCloneable() ? clone $data : $data);


This should be backported on 2.4 as a bugfix.

This is not really a bugfix. It's because now this method is always called whereas before it was not called if the operation was a collection.

An issue has been reported for non-cloneable objects IIRC

In this case, without this change, the tests wouldn't pass.

src/EventListener/ReadListener.php

dunglas · 2019-08-10T06:08:45Z

src/Stage/ReadStageInterface.php

+    /**
+     * @return object|iterable|null
+     */
+    public function apply(array $attributes, array $parameters, ?array $filters, string $method, array $normalizationContext);


apply isn't very descriptive (I'm not a fond of "stage" too btw :D). Can't we more explicit names?

Which one do you have in mind? Something like read?
I know you would say that 😄 Naming is really hard and subjective. "Stage" was interesting because it was not used and its meaning was alright: it's a "step in a process".

Stage sounds like “util” or “service” to me: too generic and not very descriptive

But it is something kind of generic. It's a step or an operation. How would you call the read, serialize, validate, etc. things?

Make it a callable? __invoke?

Good idea! WDYT @dunglas?

dunglas · 2019-08-10T06:09:11Z

src/Stage/ReadStage.php

+ *
+ * @author Alan Poulain <contact@alanpoulain.eu>
+ */
+final class ReadStage implements ReadStageInterface


Why isn't this class also used by the GraphQL resolver?

I would like to merge both too but sadly they are very different. I don't think it will be doable easily.

But then it doesn’t fix the issue of having good extensions points working both for REST and GraphQL. Which is one of the goals.

Why not having different ones? The need to use both REST and GraphQL is not common. It would just mean having two different service names to use.

To me it's very common, implementing both. That's actually what drove me to API Platform in the first place, I thought it would provide a(nother) way to DRY my Controllers and Resolvers. If you feel it's too much work for this MR, that's of course your prerogative, but please don't sacrifice forever the goal of unification. Also, Stage feels okay ; it fits, and I can't find anything I deem more suitable.

We could introduce stage for both REST and GraphQL, but the service will be an empty shell with practically no code IMO. It will be just there to be decorated.

dunglas · 2019-08-10T06:11:29Z

src/Stage/ReadStageInterface.php

+ *
+ * @author Alan Poulain <contact@alanpoulain.eu>
+ */
+interface ReadStageInterface


I wonder if this new concept is really necessary. Does this logic belong to the DataProvider? Or should we create a ApiRequestFactory class instead (see my previous comment?). I'm not sure yet, I need to think about it, but I've the feeling that we're introducing unnecessary indirection layers.

I think their responsibility is clear:

allow or not to apply the stage

extract, collect and normalize the right parameters (mainly extract the identifiers and build the (de)normalization context)

call the good "solver"

We could add the second point inside an ApiRequestFactory. But IMO it will not necessarily be a good thing. It will have a lot of conditional clauses and could quickly become a monster.

And I don't know where we could put the other parts. Maybe having a ChainDataProvider and modify the ChainDataPersister to choose the right method (persist or remove to call based on the ApiRequest)?

Also IMHO, it's easier for the user to understand and decorate this kind of service than to use the data provider / persister and the serializer.

I’m really not sure about the last point! It’s more concepts to learn with no clear responsibilities.

I guess it depends of the developer and their way to think.
But a lot of developers are using the events already to add more logic so the need is there IMO.

dunglas · 2019-08-18T15:17:28Z

I need to find some time to dive deeper into this. I'm still not convinced that we need this new layer of complexity. I also don't like the fact that this new class isn't used by the GraphQL subsystem.

stale · 2022-11-05T00:03:29Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

soyuka · 2023-10-17T14:52:12Z

this was quite helpful towards building #5657 thanks !

alanpoulain added the RFC label Aug 9, 2019

dunglas requested changes Aug 10, 2019

View reviewed changes

alanpoulain force-pushed the add-stages branch from d89d62d to 3924aa8 Compare August 10, 2019 10:39

Add Read Stage

78a864c

alanpoulain force-pushed the add-stages branch from 3924aa8 to 78a864c Compare August 10, 2019 11:04

nitneuk mentioned this pull request Aug 19, 2019

Prevent cloning generator when instanciating previous_data #2990

Merged

jamesisaac mentioned this pull request Aug 26, 2019

[Proposal] Internal events #2506

Closed

Base automatically changed from master to main January 23, 2021 21:59

stale bot added the wontfix label Nov 5, 2022

soyuka added enhancement and removed wontfix labels Mar 17, 2023

soyuka mentioned this pull request Aug 18, 2023

refactor: use provider/processor instead of event listeners #5657

Merged

soyuka closed this Oct 17, 2023

Uh oh!

Comments

Conversation

alanpoulain commented Aug 9, 2019

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Aug 18, 2019

Uh oh!

stale bot commented Nov 5, 2022

Uh oh!

soyuka commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

alanpoulain Aug 10, 2019 •

edited

Loading

alanpoulain Aug 10, 2019 •

edited

Loading