Allow subresource items in the iri converter #1877

soyuka · 2018-04-19T13:03:03Z

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

This enables subresources in the IRIConverter so that subresources can get denormalized as well.
I thought that it'd be better to use the ReadListener (avoid code duplication) instead of injecting another data provider. Let me know if you'd prefer the later (it's why I didn't added any tests yet).

Done:

Move the ReadListener code to a Trait (OperationDataProviderTrait)
Initialize a new OperationDataProvider class (uses the trait, used only in IriConverter, has a getDataFromRouteParameters method)
Refactor IriConverterTests (removed duplication code)
ReadListener uses the trait as well
Add behat subresource denormalizeRelation test

dunglas · 2018-04-20T08:49:26Z

src/Bridge/Symfony/Routing/IriConverter.php


-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ItemDataProviderInterface $itemDataProvider, RouteNameResolverInterface $routeNameResolver, RouterInterface $router, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $identifiersExtractor = null, ChainIdentifierDenormalizer $identifierDenormalizer = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ItemDataProviderInterface $itemDataProvider = null, RouteNameResolverInterface $routeNameResolver, RouterInterface $router, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $identifiersExtractor = null, ChainIdentifierDenormalizer $identifierDenormalizer = null, ReadListener $readListener = null)


It introduces a dependency to a Symfony full stack related class in the low level infrastructure. I don't think it's a good idea, it will be very painful to support other frameworks (or PSR-7) in the future.

Okay WDYT about this:

Move private methods to a trait

Introduce a new class (OperationDataProvider that uses the trait)

Inject the OperationDataProvider in the IriConverter so that it can call item and subresource operations

This way:

no changes in read listener

de-duplicate code in IriConverter

improves future updates in the IriConverter concerning data retrieval (IriToItem)

dunglas · 2018-04-20T09:56:40Z

src/DataProvider/OperationDataProvider.php

+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+
+final class OperationDataProvider


Shouldn't we introduce an interface?

We may but I'm also to mark this as @internal unless we can find a global use for it? For now it'll be only used by the IriConverter and I really doubt it'll be used for anything else :p.
I just want to avoid injecting everything the OperationDataProviderTrait depends on in the IriConverter.

dunglas · 2018-04-20T09:58:15Z

src/DataProvider/OperationDataProviderTrait.php

+        }
+
+        if (null === $data) {
+            throw new NotFoundHttpException('Not Found');


We shouldn't throw a HTTP error here, but a domain exception.

totally should be in ReadListener only

dunglas · 2018-04-20T09:59:10Z

src/DataProvider/OperationDataProviderTrait.php

+     *
+     * @return array|\Traversable|null
+     */
+    private function getCollectionData(Request $request, array $attributes, array $context)


Don't you think we can avoid to depend of the request, but extract the relevant data from the controller/event listener and pass it through the context? It will be easier to support PSR-7 this way.

Yes we can totally do this will be cleaner.

soyuka · 2018-04-20T14:57:34Z

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

@@ -58,7 +58,7 @@
            <argument type="service" id="api_platform.router" />
            <argument type="service" id="api_platform.property_accessor" />
            <argument type="service" id="api_platform.identifiers_extractor.cached" />
-            <argument type="service" id="api_platform.identifier.denormalizer" />
+            <argument type="service" id="api_platform.operation_data_provider" />


Not released yet == not BC break.

soyuka · 2018-04-20T15:25:17Z

tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php

+            'api_platform.identifier.date_normalizer',
+            'api_platform.identifier.uuid_normalizer',
+            'api_platform.identifiers_extractor',
+            'api_platform.identifiers_extractor.cached',


alphabetical order :D

dunglas · 2018-04-25T09:27:06Z

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

@@ -26,6 +26,13 @@
            <argument type="tagged" tag="api_platform.subresource_data_provider" />
        </service>
        <service id="ApiPlatform\Core\DataProvider\SubresourceDataProviderInterface" alias="api_platform.subresource_data_provider" />
+
+        <service id="api_platform.operation_data_provider" class="ApiPlatform\Core\DataProvider\OperationDataProvider">


public="false"?

dunglas · 2018-04-25T09:34:18Z

src/Bridge/Symfony/Routing/IriConverter.php

        }

-        if (null === $identifierDenormalizer) {
-            @trigger_error(sprintf('Not injecting "%s" is deprecated since API Platform 2.2 and will not be possible anymore in API Platform 3.', ChainIdentifierDenormalizer::class), E_USER_DEPRECATED);
+        if (null === $operationDataProvider) {


It will make very difficult to use API Platform without a framework or with another framework than Symfony. For instance, if I want to use API Platform's classes without a framework, and I don't bother to support subresources (I've a project like that), I'm stuck.

dunglas · 2018-04-25T09:35:52Z

src/DataProvider/OperationDataProvider.php

+use ApiPlatform\Core\Util\RequestAttributesExtractor;
+
+/**
+ * @internal


Has it's a mandatory dependency of IriConverter, and there is no related interface, it cannot be @internal.

Also, please add some PHPDoc.

dunglas · 2018-04-25T09:36:10Z

src/DataProvider/OperationDataProvider.php

+/**
+ * @internal
+ */
+class OperationDataProvider


I would just get rid of this class, it introduces unneeded complexity and indirection. Can we not include the trait directly in the data provider? It looks cleaner to me (and allows to keep subresources support optional).

dunglas · 2018-04-25T09:37:29Z

src/DataProvider/OperationDataProvider.php

+        $this->identifierDenormalizer = $identifierDenormalizer;
+    }
+
+    public function getDataFromRouteParameters(string $iri = null, array $parameters, array $context = [])


Why is $iri nullable?

dunglas · 2018-04-25T09:40:02Z

src/Util/RequestAttributesExtractor.php

+        return self::extractAttributesFromParameters($request->attributes->all());
+    }
+
+    public static function extractAttributesFromParameters(array $attributes)


I would introduce a new class, as it's not tight with the Request nor Symfony anymore.

Also, please copy/adapt the PHPDoc.

dunglas · 2018-04-25T09:41:07Z

src/DataProvider/OperationDataProviderTrait.php

+use ApiPlatform\Core\Exception\InvalidIdentifierException;
+use ApiPlatform\Core\Identifier\Normalizer\ChainIdentifierDenormalizer;
+
+trait OperationDataProviderTrait


@internal?

dunglas · 2018-04-25T09:41:41Z

src/DataProvider/OperationDataProviderTrait.php

+    /**
+     * @var CollectionDataProviderInterface
+     */
+    private $collectionDataProvider;


Please add a blank line between props (btw, it's often better to have traits without props).

dunglas · 2018-04-25T09:42:09Z

src/DataProvider/OperationDataProviderTrait.php

+    /**
+     * Retrieves data for a collection operation.
+     *
+     * @return array|\Traversable|null


iterable|null?

dunglas · 2018-04-25T09:44:48Z

src/EventListener/ReadListener.php


    public function __construct(CollectionDataProviderInterface $collectionDataProvider, ItemDataProviderInterface $itemDataProvider, SubresourceDataProviderInterface $subresourceDataProvider = null, SerializerContextBuilderInterface $serializerContextBuilder = null, ChainIdentifierDenormalizer $identifierDenormalizer = null)
    {
        $this->collectionDataProvider = $collectionDataProvider;
        $this->itemDataProvider = $itemDataProvider;
        $this->subresourceDataProvider = $subresourceDataProvider;
        $this->serializerContextBuilder = $serializerContextBuilder;
+        $this->identifierDenormalizer = $identifierDenormalizer;

        if (null === $identifierDenormalizer) {


Same here, I would not throw a deprecation if it's not provided. If you don't inject it, you'll just loose support for complex identifiers (not a big deal in many projects).

dunglas · 2018-04-25T09:56:16Z

It's a big change for a patch release. I would be more confortable to revert the previous fix and merge the new system in master. WDYT @soyuka?

soyuka · 2018-04-25T15:06:34Z

It's a big change for a patch release. I would be more confortable to revert the previous fix and merge the new system in master. WDYT @soyuka?

I removed some deprecations and the added code will work without subresources and without identifier denormalizer. It's really your call though, I don't mind! Still, this may be a big improvement for those who want to use subresources in the denormalization process (closes a bunch of issues)!

dunglas

Good job, thanks for handling that!

dunglas requested changes Apr 20, 2018

View reviewed changes

soyuka force-pushed the allow-subresource-iri branch from 5e70e35 to c6218e5 Compare April 20, 2018 09:35

dunglas requested changes Apr 20, 2018

View reviewed changes

soyuka force-pushed the allow-subresource-iri branch 2 times, most recently from 6045f20 to 6d5ff4b Compare April 20, 2018 14:56

soyuka changed the base branch from master to 2.2 April 20, 2018 14:56

soyuka changed the title ~~Proposal: Allow subresource items in the iri converter~~ WIP: Allow subresource items in the iri converter Apr 20, 2018

soyuka commented Apr 20, 2018

View reviewed changes

soyuka force-pushed the allow-subresource-iri branch from 6d5ff4b to 74f1eb6 Compare April 20, 2018 15:23

soyuka commented Apr 20, 2018

View reviewed changes

soyuka changed the title ~~WIP: Allow subresource items in the iri converter~~ Allow subresource items in the iri converter Apr 20, 2018

soyuka force-pushed the allow-subresource-iri branch from 74f1eb6 to 5fb07c7 Compare April 20, 2018 15:30

soyuka added the [RFR] Ready for Review label Apr 20, 2018

soyuka force-pushed the allow-subresource-iri branch 3 times, most recently from 636d989 to a823bcf Compare April 23, 2018 07:50

dunglas requested changes Apr 25, 2018

View reviewed changes

soyuka force-pushed the allow-subresource-iri branch 3 times, most recently from aa0b43a to 8d89a7f Compare April 25, 2018 14:52

dunglas approved these changes Apr 25, 2018

View reviewed changes

soyuka requested a review from meyerbaptiste April 25, 2018 16:18

Allow subresource items in the iri converter

985f0c1

soyuka force-pushed the allow-subresource-iri branch from 8d89a7f to 985f0c1 Compare April 26, 2018 07:26

soyuka merged commit 309e0ff into api-platform:2.2 Apr 26, 2018

soyuka deleted the allow-subresource-iri branch April 26, 2018 15:04

teohhanhui mentioned this pull request May 2, 2018

[Do not merge] Revert merge master into 2.2 #1923

Closed

19 tasks

soyuka restored the allow-subresource-iri branch May 2, 2018 16:08

soyuka mentioned this pull request May 2, 2018

Allow subresource items in the iri converter #1927

Merged


		public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ItemDataProviderInterface $itemDataProvider, RouteNameResolverInterface $routeNameResolver, RouterInterface $router, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $identifiersExtractor = null, ChainIdentifierDenormalizer $identifierDenormalizer = null)
		public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ItemDataProviderInterface $itemDataProvider = null, RouteNameResolverInterface $routeNameResolver, RouterInterface $router, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $identifiersExtractor = null, ChainIdentifierDenormalizer $identifierDenormalizer = null, ReadListener $readListener = null)

Uh oh!

Allow subresource items in the iri converter #1877

Allow subresource items in the iri converter #1877

Uh oh!

Conversation

soyuka commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dunglas commented Apr 25, 2018

Uh oh!

soyuka commented Apr 25, 2018

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soyuka commented Apr 19, 2018 •

edited

Loading