[RFR] Allow to specify formats per resources/operations #1798

antograssiot · 2018-03-29T07:50:31Z

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

As suggesting in the issue, this PR will allow to use the following:

/**
 * @ApiResource(attributes={
 *     "formats"={"jsonld", "json", "html", "csv"={"text/csv"}}
 * }, itemOperations={
 *     "put", "delete",
 *     "get"={"formats"={"jsonld", "json", "html", "pdf"={"application/pdf"}}}
 * })
 */
class Foo

todo:

Create a FormatsProvider service
Add more PHPUnit tests
Add integration tests
Update the swagerUi available formats
Add formats fallback attributes in the @ApiResource annotation

ping @vincentchalamon, @Toflar

antograssiot · 2018-03-29T19:16:30Z

Playing a bit more with this and when adding test I think I need to introduce a new service like a FormatProvider that will be called by every service that actually receives %api_platform.formats%...

dunglas · 2018-03-30T13:22:13Z

I'm for keeping only "pdf"={"application/pdf"}, it's expliciter

antograssiot · 2018-03-30T15:24:50Z

PR updated to only support the simplest declaration form.

dunglas

To me there is a flaw in the logic. The service shouldn't depend of the request, it should use the attributes directly to find the accepted formats for every resource class, then the Swagger documentation and the listener should use this new service to respectively display the available formats or throw an exception when a non-supported format is requested.

dunglas · 2018-04-01T17:30:20Z

src/Api/FormatsProvider.php

+    private $formats;
+    private $resourceMetadataFactory;
+
+    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, array $formats)


I suggest to renamed formats in configuredFormats, it will be mor explicit

dunglas · 2018-04-01T17:33:44Z

src/Api/FormatsProvider.php

+     */
+    public function fromRequest(Request $request): array
+    {
+        if ($this->formats) {


It looks buggy: if a different format can be configured per resource, then you must test for $this->formats[$resourceClass] or the bad formats will be returned (for instance the ones of the previously requested resource class).

I'm not storing key/values just the format for one operation right now

antograssiot · 2018-04-01T19:09:27Z

Ok I'll think about your comment @dunglas. I'm the impression that you expect this service to register all acceptables format per resource, but as the format could be defined at the operation level, that seems a bit weird to me.
I could still remove the request and only use attributes, that's what I've been starting with but when I saw the number of places where attributes were not already presents/extracted, I decided to it in a single point in the service, ok to change back if you prefer. I'll work on that next week

Toflar · 2018-04-03T09:13:56Z

Sorry for coming late to the party and thanks for pinging me @antograssiot. I'm +1 on the fact that it should go into its own service and become part of the resource metadata rather than the request as @dunglas stated. Even if you allow to specify the formats on operation level, it is still resource meta data that can have different sources, not only annotations.

antograssiot · 2018-04-04T19:54:42Z

@dunglas it has been updated and doesn't rely on the SF Request but only on attributes.

antograssiot · 2018-04-04T19:57:23Z

@Toflar it already leverages the getOperationAttribute() method from the resource metadata so already doesn't rely on annotation only but would work with Yaml or XML like every other attributes. Did I missed something ?

Toflar

@Toflar it already leverages the getOperationAttribute() method from the resource metadata so already doesn't rely on annotation only but would work with Yaml or XML like every other attributes. Did I missed something ?

I was just mentioning again 😄 PR looks already a lot better now, excellent work! I left a few comments, mainly about improving DX when upgrading to v3 later on but also on the general concept about how the configuration should behave. I hope it helps, keep up the work!

Toflar · 2018-04-05T08:24:22Z

src/Api/FormatsProvider.php

+    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, array $formats)
+    {
+        $this->resourceMetadataFactory = $resourceMetadataFactory;
+        $this->configuredFormats = $formats;


Could you also rename $formats to $configuredFormats to be consistent here?

Toflar · 2018-04-05T08:26:50Z

src/Api/FormatsProvider.php

+    /**
+     * Finds formats from a Resource class.
+     */
+    public function getFormatsFromAttributes(array $attributes): array


Not sure if I like this concept here. I mean it's a matter of how we want it to work. Right now the specified formats on the resource would override the generally configured formats. But if I think about it, the generally configured formats are general. So what I expect this annotation to do is to configure additional formats for this specific resource. So imho it should merge the configured formats with the ones defined on the resource. I understand that it can be a use case to say everything should be available in json except for this one resource so maybe how it behaves should be a configuration option too?

That's indeed one of the concerns I have too. it could be painful if you add a general format, you'll then need to update all annotation.

Yeah for me the general ones are the ones that should be available for all endpoints. So merging seems more appropriate to me.

I like the idea to override the default formats, it's pretty useful (at the operation level at least) I think. IMHO, If we merge the configured formats, we also have to find a way to be able to completely override the default formats.

Yeah, thinking about additionalFormats and formats. The first just adds and the other overrides and you're done.

ok let's try to get this merged with formats only and I'll add additionalFormats in a following PR if @api-platform/core-team agrees. That will be pretty easy anyway

Toflar · 2018-04-05T08:33:06Z

src/EventListener/AddFormatListener.php

    {
        $this->negotiator = $negotiator;
        $this->formats = $formats;
+        $this->formatsProvider = $formatsProvider;


You could remove the typehint and do something like this:

public function __construct(Negotiator $negotiator, $formatProvider) { // BC if (\is_array($formatProvider)) { // deprecated notice but set anyway return; } if (!$formatProvider instanceof FormatsProviderInterface) { // throw new \InvalidArgumentException...... } }

Toflar · 2018-04-05T08:33:27Z

src/EventListener/AddFormatListener.php

@@ -51,6 +55,10 @@ public function onKernelRequest(GetResponseEvent $event)
            return;
        }

+        if (null !== $this->formatsProvider) {


Would need a comment to be removed once BC is removed for Version 3.

Toflar · 2018-04-05T08:34:17Z

src/EventListener/DeserializeListener.php


-    public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, array $formats)
+    public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, array $formats, FormatsProviderInterface $formatsProvider = null)


Same here with the constructor. I just want to omit we add another 5 arguments to the constructor because it makes migration from 2 to 3 harder.

Toflar · 2018-04-05T08:36:31Z

src/Annotation/ApiResource.php

@@ -129,6 +130,13 @@
     */
    private $forceEager;

+    /**
+     * @see https://github.com/Haehnchen/idea-php-annotation-plugin/issues/112


In my PR (#1685) it was mentioned that the core team does not want to add more root annotation configuration values but instead put it into attributes. Just saying that this may need to be modified but I let @dunglas decide on this. I don't remember the exact reason for this anymore because I actually like when things are specific 😄

this just follows #1788, nothing more

PR has been updated, new review to come

meyerbaptiste · 2018-03-30T17:05:12Z

src/Api/FormatsProvider.php

+    {
+        $this->resourceMetadataFactory = $resourceMetadataFactory;
+        $this->configuredFormats = $formats;
+        $this->formats = [];


public $formats = [];

meyerbaptiste · 2018-04-05T08:54:48Z

src/Api/FormatsProvider.php

+            if (!is_numeric($format)) {
+                $resourceFormats[$format] = (array) $value;
+                continue;
+            } elseif (array_key_exists($value, $this->configuredFormats)) {


if instead of elseif.

meyerbaptiste · 2018-04-05T08:55:45Z

src/Api/FormatsProvider.php

+                continue;
+            }
+
+            throw new InvalidArgumentException(sprintf("You either need to add the format '%s' to your project configuration or declare a mime type for it in your annotation.", $value));


Can you add the @throws annotation in the PHPDoc?

meyerbaptiste · 2018-04-05T09:26:39Z

src/Api/FormatsProvider.php

+
+        $resourceMetadata = $this->resourceMetadataFactory->create($attributes['resource_class']);
+
+        if (!$formats = (array) $resourceMetadata->getOperationAttribute($attributes, 'formats', [], true)) {


About (array), is there a way to get something different from an array?

Yes I guess by using "formats"="json" instead of "formats"={"json"}

But as the key is plural, the user could easily understand it expects an array. Otherwise if the value is not an array, maybe throw an exception? WDYT @meyerbaptiste?

I can if you all prefer, it was almost costless

on a second thought, I buy the 'plural' argument, I've removed the typehint, added an exception and test.

meyerbaptiste · 2018-04-05T09:29:50Z

src/Api/FormatsProvider.php

+            return $this->configuredFormats;
+        }
+
+        $resourceMetadata = $this->resourceMetadataFactory->create($attributes['resource_class']);


What if resource_class is not set?

antograssiot · 2018-04-26T18:52:04Z

I'm not sure why circle-ci would try to test the code on PHP5.6 🤔 ...
@teohhanhui is it because of #1888 and 2.2 not being merged in master yet ?

soyuka · 2018-04-26T20:06:54Z

try to force push

antograssiot · 2018-06-08T19:01:16Z

I've rebased and updated with the last fixer rules. Anything else to be done on this one ?

dunglas · 2018-06-08T20:07:26Z

src/Bridge/Symfony/Bundle/Resources/config/swagger.xml

@@ -58,7 +58,7 @@
            <argument>%api_platform.title%</argument>
            <argument>%api_platform.description%</argument>
            <argument>%api_platform.version%</argument>
-            <argument>%api_platform.formats%</argument>
+            <argument type="service" id="api_platform.formats_provider"/>


missing space before />

updated 👍

Add unit tests Update service injection with deprecation error and exception Fix missing use of FormatsProvider into DocumentAction Only support the shortest mime types declaration Update service injection in documentation to trigger deprecation warning Avoid another way to badly declare format overrding. update phpdoc

dunglas · 2018-06-09T09:30:19Z

Thanks!

api-platform/core#1798

* Add documentation on enabling format for specific operation api-platform/core#1798 * Update content-negotiation.md

antograssiot force-pushed the format-by-resource branch from dad282d to 9c17235 Compare March 29, 2018 07:54

vincentchalamon requested review from dunglas and vincentchalamon March 29, 2018 08:15

antograssiot force-pushed the format-by-resource branch from 9c17235 to b80fc12 Compare March 29, 2018 08:22

antograssiot changed the title ~~Allow to specify formats per resources/operations~~ WIP Allow to specify formats per resources/operations Mar 29, 2018

antograssiot force-pushed the format-by-resource branch 4 times, most recently from c9fb1f1 to 9fc3c53 Compare March 30, 2018 12:04

antograssiot changed the title ~~WIP Allow to specify formats per resources/operations~~ [RFR] Allow to specify formats per resources/operations Mar 30, 2018

vincentchalamon previously approved these changes Mar 30, 2018

View reviewed changes

antograssiot force-pushed the format-by-resource branch from e345956 to 98997fa Compare March 30, 2018 19:21

dunglas requested changes Apr 1, 2018

View reviewed changes

soyuka added [RFR] Ready for Review and removed [RFR] Ready for Review labels Apr 3, 2018

antograssiot force-pushed the format-by-resource branch from 98997fa to d8f6ea2 Compare April 4, 2018 19:53

Toflar suggested changes Apr 5, 2018

View reviewed changes

meyerbaptiste requested changes Apr 5, 2018

View reviewed changes

Simperfit added the [WFC] Waiting for Change label Apr 6, 2018

antograssiot force-pushed the format-by-resource branch from d8f6ea2 to b026b6c Compare April 6, 2018 15:09

antograssiot changed the title ~~[RFR] Allow to specify formats per resources/operations~~ [WIP] Allow to specify formats per resources/operations Apr 6, 2018

antograssiot changed the title ~~[WIP] Allow to specify formats per resources/operations~~ [RFR] Allow to specify formats per resources/operations Apr 6, 2018

antograssiot force-pushed the format-by-resource branch from fa62570 to e7aca3c Compare April 10, 2018 18:39

Simperfit added [RFR] Ready for Review and removed [WFC] Waiting for Change labels Apr 11, 2018

Simperfit approved these changes Apr 11, 2018

View reviewed changes

antograssiot force-pushed the format-by-resource branch 2 times, most recently from 1a75d55 to d19daa2 Compare April 16, 2018 09:20

antograssiot force-pushed the format-by-resource branch from d19daa2 to 031cbf3 Compare April 26, 2018 17:16

antograssiot force-pushed the format-by-resource branch 4 times, most recently from 199c17d to 2201faa Compare May 3, 2018 13:48

antograssiot force-pushed the format-by-resource branch from 2201faa to 615746f Compare May 5, 2018 19:32

antograssiot force-pushed the format-by-resource branch 2 times, most recently from 01e3b11 to 60706c9 Compare June 8, 2018 18:59

dunglas approved these changes Jun 8, 2018

View reviewed changes

antograssiot force-pushed the format-by-resource branch from 60706c9 to ab48edb Compare June 8, 2018 21:19

soyuka approved these changes Jun 9, 2018

View reviewed changes

dunglas merged commit bd2eadb into api-platform:master Jun 9, 2018

antograssiot deleted the format-by-resource branch June 11, 2018 14:40

jdeniau mentioned this pull request Jun 16, 2018

add a specific "documentation_formats" that may differ from the API "… #1891

Closed

antograssiot added a commit to antograssiot/docs-1 that referenced this pull request Jun 16, 2018

Add documentation on enabling format for specific operation

bf46ca0

api-platform/core#1798

antograssiot added a commit to antograssiot/docs-1 that referenced this pull request Jun 16, 2018

Add documentation on enabling format for specific operation

36e65b3

api-platform/core#1798

antograssiot mentioned this pull request Jun 16, 2018

Add documentation on enabling format for specific operation api-platform/docs#507

Merged

antograssiot added a commit to antograssiot/docs-1 that referenced this pull request Jun 16, 2018

Add documentation on enabling format for specific operation

3eaa928

api-platform/core#1798

dunglas pushed a commit to api-platform/docs that referenced this pull request Jun 18, 2018

Add documentation on enabling format for specific operation (#507)

6d4428b

* Add documentation on enabling format for specific operation api-platform/core#1798 * Update content-negotiation.md


		public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, array $formats)
		public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, array $formats, FormatsProviderInterface $formatsProvider = null)


		$resourceMetadata = $this->resourceMetadataFactory->create($attributes['resource_class']);

		if (!$formats = (array) $resourceMetadata->getOperationAttribute($attributes, 'formats', [], true)) {

Uh oh!

[RFR] Allow to specify formats per resources/operations #1798

[RFR] Allow to specify formats per resources/operations #1798

Uh oh!

Conversation

antograssiot commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antograssiot commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Mar 30, 2018

Uh oh!

antograssiot commented Mar 30, 2018

Uh oh!

dunglas left a comment

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!

antograssiot commented Apr 1, 2018

Uh oh!

Toflar commented Apr 3, 2018

Uh oh!

antograssiot commented Apr 4, 2018

Uh oh!

antograssiot commented Apr 4, 2018

Uh oh!

Toflar left a comment

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!

meyerbaptiste Apr 5, 2018 • 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!

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

antograssiot commented Mar 29, 2018 •

edited

Loading

antograssiot commented Mar 29, 2018 •

edited

Loading

meyerbaptiste Apr 5, 2018 •

edited

Loading