Add a new `getIriFromPlainIdentifier` method and remove ItemDataProvider from AbstractItemNormalizer #1837

Simperfit · 2018-04-09T13:51:31Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	multiples need to be sure
License	MIT
Doc PR	todo

As discussed with @soyuka in #1834 we should not use ItemDataProvider in the normalizer and the the process works totally with IRIs.

So I'm adding a new method to convert from plainIdentifiers to IRIs.

soyuka · 2018-04-09T16:13:48Z

src/Bridge/Symfony/Routing/IriConverter.php

+     */
+    public function getIriFromPlainIdentifier($id, string $resourceClass, array $context = []): string
+    {
+        return $this->getIriFromItem($this->itemDataProvider->getItem($resourceClass, $id, null, $context + ['fetch_data' => true]));


Like I said on slack, we should use the router to rebuild the route not actually do a request. Code is almost the same as getIriFromItem

dunglas · 2018-04-09T16:20:04Z

src/Api/IriConverterInterface.php

+     *
+     * @return string
+     */
+    public function getIriFromPlainIdentifier($id, string $resourceClass, array $context = []): string;


This is a BC break, and not something we want in this interface btw. Maybe can we introduce a new one to deal with plain identifiers (a bad practice, according to me).

This is definitely a bad practice but people asked for it a lot so let's try to make everyone happy :D.

👍 to add a new service. It'll be better then injecting ItemDataProvider in the normalizers...

By a new service do you mean I should not implement the PlainIdentifierConverterInterfce in the IriConverter ? or like this it's ok ?

dunglas · 2018-04-09T16:20:56Z

src/Serializer/AbstractItemNormalizer.php

@@ -57,6 +57,9 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
        $this->iriConverter = $iriConverter;
        $this->resourceClassResolver = $resourceClassResolver;
        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
+        if (null !== $itemDataProvider) {
+            @trigger_error(sprintf('Passing a %s is deprecated since 2.3.0 and will not be possible in 3.0.0.', ItemDataProviderInterface::class));


you can drop the patch part

soyuka · 2018-04-10T07:37:48Z

src/Serializer/AbstractItemNormalizer.php

                try {
-                    return $this->itemDataProvider->getItem($className, $value, null, $context + ['fetch_data' => true]);
+                    return $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromPlainIdentifier($value, $className), $context + ['fetch_data' => true]);


👍 I really like the fix now.

soyuka · 2018-04-10T07:39:11Z

Could you also add the #1716 test + fix ? (best would be a cherry pick from that author to keep his contribution)

Simperfit · 2018-04-10T08:04:53Z

test from #1716 cherry-picked.

Taluu

I think that would actually solve the problem I have on #1840, so 👍

Taluu · 2018-04-10T10:21:29Z

src/Serializer/AbstractItemNormalizer.php

@@ -57,6 +61,9 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
        $this->iriConverter = $iriConverter;
        $this->resourceClassResolver = $resourceClassResolver;
        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
+        if (null !== $itemDataProvider) {
+            @trigger_error(sprintf('Passing a %s is deprecated since 2.3 and will not be possible in 3.0.', ItemDataProviderInterface::class));
+        }
        $this->itemDataProvider = $itemDataProvider;


why not remove the property ? Is it still used ?

my bad, it's a protected one...

We should but it'd be a backward compatibility break :|.

Taluu · 2018-04-10T10:22:23Z

src/Serializer/AbstractItemNormalizer.php

@@ -57,6 +61,9 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
        $this->iriConverter = $iriConverter;
        $this->resourceClassResolver = $resourceClassResolver;
        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
+        if (null !== $itemDataProvider) {
+            @trigger_error(sprintf('Passing a %s is deprecated since 2.3 and will not be possible in 3.0.', ItemDataProviderInterface::class));


missing a E_USER_DEPRECATED

src/Serializer/AbstractItemNormalizer.php

                try {
-                    return $this->itemDataProvider->getItem($className, $value, null, $context + ['fetch_data' => true]);
+                    return $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromPlainIdentifier($value, $className), $context + ['fetch_data' => true]);
                } catch (ItemNotFoundException $e) {
                    throw new InvalidArgumentException($e->getMessage(), $e->getCode(), $e);
                } catch (InvalidArgumentException $e) {


soyuka · 2018-04-10T10:31:04Z

What about merging #1834 in this one and use the same method (ie getIriFromPlainIdentifier)?

Taluu · 2018-04-10T10:37:15Z

Nevermind, it doesn't solve #1840, as the first check done in the \is_string() calls prevent from using plain identifiers...

soyuka · 2018-04-10T12:18:38Z

Nevermind, it doesn't solve #1840, as the first check done in the \is_string() calls prevent from using plain identifiers...

What is your identifier if not a string? In your issue it seems to be one?
Best would be a behat test case that identifies your problem. As far as I can tell, the tests from #1547 have been maintained in #1762 ?

Taluu · 2018-04-10T13:12:20Z

What is your identifier if not a string? In your issue it seems to be one?

Let's discuss that in the related issue. :}

teohhanhui · 2018-04-11T18:22:32Z

src/Api/IriPlainIdentifierAwareConverterInterface.php

+ *
+ * @author Hamza Amrouche <hamza.simperfit@gmail.com>
+ */
+interface IriPlainIdentifierAwareConverterInterface extends IriConverterInterface


I don't think it needs to extend the existing interface. We could just keep the type hint as IriConverterInterface, but check if it is also an IdentifierToIriConverterInterface. ("Plain" is redundant here.)

Mainly because I prefer small, single-purpose interfaces.

Edit: Uhh, it's one of the SOLID principles... https://en.wikipedia.org/wiki/Interface_segregation_principle

I agree with you, but in fact this interface says, "Ok i'm an IriConverter but I can manage plain identifier too".
I don't mind not extending the IriConverterInterface ;).

teohhanhui · 2018-04-12T08:30:16Z

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog

+## 2.3.0
+
+* Introduce a new interface `IriPlainIdentifierAwareConverterInterface`


This hasn't been updated to reflect the changes. 😜

and btw we never change the changelog in merge requests it's done when releasing only

Not sure if that's a good idea, to be honest. For example, Symfony requires updating the changelog: https://raw.githubusercontent.com/symfony/symfony/master/.github/PULL_REQUEST_TEMPLATE.md

That's why I updated it, I've contributing some PRs to symfony lately and add to update it ;).

And I think it's a good practice too to update the changelog when adding new things or deprecating things so we gain time when releasing

soyuka · 2018-04-18T12:51:22Z

src/Serializer/AbstractItemNormalizer.php

@@ -313,9 +320,9 @@ protected function denormalizeRelation(string $attributeName, PropertyMetadata $

        if (!\is_array($value)) {
            // repeat the code so that IRIs keep working with the json format
-            if (true === $this->allowPlainIdentifiers && $this->itemDataProvider) {
+            if (true === $this->allowPlainIdentifiers && ($this->iriConverter instanceof IriToIdentifierConverterInterface && $this->iriConverter instanceof IriConverterInterface)) {


Shouldn't we support the other path? I mean what if IRIConverter is not an instance of IriToIdentifierConverterInterface (case where someone extends the class)?

Simperfit · 2018-05-04T08:50:05Z

PR rebased with master

dunglas · 2018-05-05T13:19:22Z

src/Api/IriToIdentifierConverterInterface.php

+ *
+ * @author Hamza Amrouche <hamza.simperfit@gmail.com>
+ */
+interface IriToIdentifierConverterInterface


IriToPlainIdentifier? Because an IRI by definition is already an identifier (Internationalized Resource Identifier).

see #1837 (comment)

dunglas · 2018-05-05T13:20:24Z

src/Api/IriToIdentifierConverterInterface.php

+     *
+     * @return string
+     */
+    public function getIriFromPlainIdentifier($id, string $resourceClass, int $referenceType = UrlGeneratorInterface::ABS_PATH): string;


I suggest to only allow arrays for the $id parameter, it will be more in sync with the work of @soyuka on complex identifiers.

dunglas · 2018-05-05T13:20:58Z

src/Bridge/Symfony/Routing/IriConverter.php

@@ -59,7 +60,7 @@ public function __construct(PropertyNameCollectionFactoryInterface $propertyName
        $this->identifierDenormalizer = $identifierDenormalizer;

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


Why this change?

IIRC, this change deprecation has been reverted in another PR.

need to rebase, ill see

It still there in master

dunglas · 2018-05-05T13:22:16Z

src/Serializer/AbstractItemNormalizer.php

@@ -43,6 +44,9 @@

    protected $propertyNameCollectionFactory;
    protected $propertyMetadataFactory;
+    /**
+     * @var IriToIdentifierConverterInterface|IriConverterInterface


I would fully split both, and add a new optional constructor parameter instead.

You mean, splitting the classes too ?

maybe, but at least inject it 2 times

dunglas · 2018-05-05T13:23:53Z

src/Serializer/AbstractItemNormalizer.php

+                    return $this->iriConverter->getItemFromIri($this->iriConverter->getIriFromPlainIdentifier($value, $className), $context + ['fetch_data' => true]);
+                }
+
+                if (true === $this->allowPlainIdentifiers && $this->itemDataProvider) {


This test can be factorized with the one line 320 (store the result in a variable).

…DataProvider from AbstractItemNormalizer

… different than the IriConverterInterface

Simperfit · 2018-05-09T12:59:28Z

@dunglas done.

soyuka · 2018-06-14T07:53:47Z

can you rebase this @Simperfit ?

flug · 2019-11-14T14:54:20Z

@Simperfit Up is this pull request still valid?

soyuka · 2020-11-28T17:25:14Z

We will eventually deprecate plain identifiers, closing this.

Simperfit requested review from dunglas and soyuka April 9, 2018 13:51

Simperfit added [RFR] Ready for Review enhancement labels Apr 9, 2018

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from f1bbf03 to 6c77a8d Compare April 9, 2018 14:38

Simperfit added [WIP] Work In Progress and removed [RFR] Ready for Review labels Apr 9, 2018

soyuka reviewed Apr 9, 2018

View reviewed changes

dunglas requested changes Apr 9, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch 2 times, most recently from 6cf7329 to 83b783a Compare April 9, 2018 18:48

Simperfit added [RFR] Ready for Review and removed [WIP] Work In Progress labels Apr 9, 2018

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from 83b783a to cd24e28 Compare April 10, 2018 07:05

soyuka reviewed Apr 10, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from cd24e28 to bf51bfa Compare April 10, 2018 07:54

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from 33190b0 to 8a8c4eb Compare April 10, 2018 08:13

Simperfit requested a review from meyerbaptiste April 10, 2018 09:12

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from 8a8c4eb to a53464f Compare April 10, 2018 09:24

Taluu approved these changes Apr 10, 2018

View reviewed changes

soyuka reviewed Apr 10, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch 2 times, most recently from 725091a to 13f91d5 Compare April 10, 2018 14:00

soyuka approved these changes Apr 10, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from 08e6ed2 to 24f91c1 Compare April 11, 2018 16:07

teohhanhui reviewed Apr 11, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch 4 times, most recently from b164e7e to 3d633d1 Compare April 12, 2018 08:13

teohhanhui reviewed Apr 12, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch 3 times, most recently from 021b954 to 1bc04f9 Compare April 12, 2018 10:06

Simperfit added this to In progress in 2.3.x Apr 13, 2018

Simperfit moved this from In progress to Done in 2.3.x Apr 13, 2018

Simperfit moved this from Done to In progress in 2.3.x Apr 13, 2018

soyuka reviewed Apr 18, 2018

View reviewed changes

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch 3 times, most recently from 6052490 to 949c198 Compare May 4, 2018 08:49

dunglas requested changes May 5, 2018

View reviewed changes

Amrouche Hamza and others added 3 commits May 9, 2018 14:32

feature: add a new getIriFromPlainIdentifier method and remove Item…

b9e80ca

…DataProvider from AbstractItemNormalizer

throw NotFound exception when use plain identifiers

6480222

feature: allow an implementation of IriToIdentifierConverterInterface…

0b2c793

… different than the IriConverterInterface

Simperfit force-pushed the feature/remove-item-data-provider-form-abstract-item-normalizer branch from 949c198 to 0b2c793 Compare May 9, 2018 12:58

soyuka mentioned this pull request May 11, 2018

[RFC] Refactor population of object in serializer #1939

Closed

soyuka mentioned this pull request Jun 14, 2018

Fix allow creating resources with custom identifers #2022

Closed

soyuka closed this Nov 28, 2020

Add a new getIriFromPlainIdentifier method and remove ItemDataProvider from AbstractItemNormalizer #1837

Add a new getIriFromPlainIdentifier method and remove ItemDataProvider from AbstractItemNormalizer #1837

Conversation

Simperfit commented Apr 9, 2018 • edited

Choose a reason for hiding this comment

dunglas Apr 9, 2018 • edited

Choose a reason for hiding this comment

soyuka Apr 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Apr 10, 2018

Simperfit commented Apr 10, 2018

Taluu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

soyuka commented Apr 10, 2018

Taluu commented Apr 10, 2018

soyuka commented Apr 10, 2018

Taluu commented Apr 10, 2018

Choose a reason for hiding this comment

teohhanhui Apr 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented May 9, 2018

soyuka commented Jun 14, 2018

flug commented Nov 14, 2019

soyuka commented Nov 28, 2020

Add a new `getIriFromPlainIdentifier` method and remove ItemDataProvider from AbstractItemNormalizer #1837

Add a new `getIriFromPlainIdentifier` method and remove ItemDataProvider from AbstractItemNormalizer #1837

Simperfit commented Apr 9, 2018 •

edited

dunglas Apr 9, 2018 •

edited

soyuka Apr 10, 2018 •

edited

dunglas Apr 9, 2018 •

edited

teohhanhui Apr 11, 2018 •

edited

dunglas May 9, 2018 •

edited