fix #1980 user defined property metadata takes precedence #1997

soyuka · 2018-06-03T15:12:46Z

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

See issue.

soyuka · 2018-06-03T15:13:43Z

ping @oxan @bendavies

oxan · 2018-06-03T15:32:54Z

I don't think this fixes #1994, as the SerializerPropertyMetadataFactory still overwrites information from the annotations. I think a check needs to be added in that factory to avoid overwriting existing information. I'm not sure that won't break anything though, as it also restricts the serializer factory from overwriting data coming from Doctrine and PropertyInfo.

oxan · 2018-06-03T15:40:58Z

Maybe it's better to leave the serializer factory alone and lower the priority of the annotation & extractor factories so that they run last (well, before the cache). Along with this PR to let them overwrite the present metadata, that should work fine. I checked and none of the existing factories seem to read from the metadata passed to them anyway (except to avoid overwriting), so I don't think it's a problem if they don't get the metadata configured by the user.

(Actually, where I'm talking about the serializer factory, we should really think about all factories that set metadata based on heuristics. It's just that in my case the metadata I defined was overwritten by the serializer factory, but that could happen with others as well).

soyuka · 2018-06-04T07:53:58Z

Maybe it's better to leave the serializer factory alone and lower the priority of the annotation & extractor factories so that they run last (well, before the cache).

Yes I think that this is safer :p. I added a commit that does this.

oxan · 2018-06-04T19:39:48Z

Does Symfony guarantee an order of services with the same priority? Because the ValidatorPropertyMetadataFactory also has priority 20, so if no order is guaranteed the validator can still overwrite user-defined metadata.

dunglas · 2018-06-04T20:43:20Z

src/Metadata/Property/Factory/AnnotationPropertyMetadataFactory.php

@@ -134,10 +134,6 @@ private function createMetadata(ApiProperty $annotation, PropertyMetadata $paren
    private function createWith(PropertyMetadata $propertyMetadata, array $property, $value): PropertyMetadata
    {
        $getter = $property[0].ucfirst($property[1]);
-        if (null !== $propertyMetadata->$getter()) {


This shouldn't be dropped. It's a BC break.

dunglas · 2018-06-04T20:44:14Z

src/Metadata/Property/Factory/ExtractorPropertyMetadataFactory.php

@@ -117,7 +117,7 @@ private function update(PropertyMetadata $propertyMetadata, array $metadata): Pr
        ];

        foreach ($metadataAccessors as $metadataKey => $accessorPrefix) {
-            if (null === $metadata[$metadataKey] || null !== $propertyMetadata->{$accessorPrefix.ucfirst($metadataKey)}()) {


bendavies · 2018-06-04T20:46:28Z

So I'd the factory can not be changed, the only possible fix is to alter the priority?

dunglas · 2018-06-04T20:47:36Z

Yes. It's cleaner anyway. And it allows anybody to customize the order (by changing priorities).

bendavies · 2018-06-04T20:48:42Z

But the factories are still weird in that some will overwrite, but some won't. Guess this can't be changed until 3.0?

dunglas · 2018-06-04T20:49:36Z

None should...

bendavies · 2018-06-04T20:52:12Z

I'll check tomorrow but I was under the impression that that wasn't the case.

soyuka · 2018-06-05T07:38:35Z

I'm really unsure that changing priorities will be enough to fix this...

oxan · 2018-06-05T09:03:33Z

Yeah, at least the SerializerPropertyMetadataFactory currently does overwrite existing data. Since the annotation factory doesn't, only changing the priority doesn't fix this.

teohhanhui · 2018-06-05T09:31:54Z

@oxan

Does Symfony guarantee an order of services with the same priority?

It should be in the reverse order of service registration:

https://github.com/symfony/symfony/blob/v4.1.0/src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php#L28

https://github.com/symfony/symfony/blob/v4.1.0/src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php#L35

soyuka · 2018-06-05T10:11:58Z

So I should change SerializerPropertyMetadataFactory so that they don't overwrite? But will it not be a BC break?

bendavies · 2018-06-05T10:28:42Z

that seems like it would be a BC break yes. seems like a catch 22 situation here to me.

bendavies · 2018-06-05T10:40:43Z

copying @Nek- nice image which shows the current state of things:

Nek- · 2018-06-05T11:31:57Z

Btw I can update it on demand.

soyuka · 2018-06-05T12:11:19Z

Anyway, if you guys need a resolution to this issue, you can always add your own PropertyMetadataFactory that overrides everything (with the lowest priority before cache)!

To me this whole issue looks like a wontfix because I don't see any viable solution that doesn't break the current state of things...

teohhanhui · 2018-06-05T12:42:04Z

@bendavies There is no "ambiguous order" :)

bendavies · 2018-06-05T12:53:40Z

@teohhanhui it's not my image, but anyway, it's more a "non obvious" order.

oxan · 2018-06-05T17:07:05Z

Is there any actual situation where autogenerated metadata overwiriting user-defined metadata is actually desired? Yes, preventing that is a BC break, but every bugfix is a BC break in a way. I can't think of any real-world situations where the current situation would be relied upon. The behaviour would only change in places where people purposefully have manually declared wrong metadata (in fact, @ApiProperty is barely documented, so I doubt it has much usage at all).

oxan · 2018-06-05T17:10:50Z

@soyuka Yes, I'm currently using a custom metadata factory to be able to override the readableLink and writableLink properties, but it's ugly and it'd be nice to have an easier solution (for future reference, code is here: https://gist.github.com/oxan/5a9ba44e72bbeae5fa1aee7d0be0b337)

bendavies · 2018-06-05T17:40:33Z

agree with @oxan really.

Nek- · 2018-06-05T21:19:41Z

I think a failing test case is needed to show how obviously wrong is "state of things". I'll do it next week if nobody is faster than me.

But indeed the best solution is to drop the behavior of voting on null only for some of theses "voters".

soyuka · 2018-06-06T07:08:20Z

In the mean time we could always introduce a new annotation that overrides metadata (like the one from @oxan), feels dirty but at least it's non-breaking.

soyuka · 2018-07-10T10:25:53Z

src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php

@@ -117,7 +114,7 @@ private function transformLinkStatus(PropertyMetadata $propertyMetadata, array $
        $relatedClass = $type->isCollection() && ($collectionValueType = $type->getCollectionValueType()) ? $collectionValueType->getClassName() : $type->getClassName();

        if (null === $relatedClass) {
-            return $propertyMetadata;
+            return $propertyMetadata->withReadableLink(true)->withWritableLink(true);


Kinda prevents the break, to me these should stay nullable but it leads to bugs (essentially regarding docs and context - should I fix those there instead?).

Yeah, I don't think this is correct, as this class is only supposed to fill in the metadata based on the Symfony Serializer mapping.

I'm totally 👍 to remove this but this has some impacts down the road.
Should we default these to true or should we test for null || true in the normalizers?

Hmm... Now I see we were already doing this from before. 😱 I guess let's keep it then. Haha...

soyuka · 2018-07-10T10:26:31Z

ping @oxan @Nek- @bendavies

Could you guys give a shot at the latest version of this patch?

Thanks!

soyuka · 2018-07-25T06:06:24Z

@dunglas wdyt?

bendavies · 2018-08-22T21:19:58Z

🚢

dunglas · 2018-08-22T21:21:53Z

Should be merged in master, not in 2.3 because the priority change may break some apps.

Change priority of user-defined metadata services to 20 User-defined metadata always overrides previous metadata

dunglas requested changes Jun 4, 2018

View reviewed changes

soyuka force-pushed the fix-metadata branch 3 times, most recently from f5c0fca to f230bc4 Compare June 5, 2018 07:37

soyuka force-pushed the fix-metadata branch from f230bc4 to 67d9b45 Compare July 10, 2018 07:33

soyuka changed the base branch from 2.2 to 2.3 July 10, 2018 07:34

soyuka force-pushed the fix-metadata branch 4 times, most recently from 22535af to deb3e07 Compare July 10, 2018 10:11

soyuka commented Jul 10, 2018

View reviewed changes

soyuka force-pushed the fix-metadata branch 2 times, most recently from 5114c1b to 2f30e44 Compare July 10, 2018 12:52

dunglas approved these changes Aug 21, 2018

View reviewed changes

fix api-platform#1980 user defined property metadata takes precedence

f7b9079

Change priority of user-defined metadata services to 20 User-defined metadata always overrides previous metadata

soyuka force-pushed the fix-metadata branch from 2f30e44 to f7b9079 Compare August 23, 2018 07:11

soyuka changed the base branch from 2.3 to master August 23, 2018 07:11

soyuka merged commit 3fdb58c into api-platform:master Aug 23, 2018

soyuka deleted the fix-metadata branch August 23, 2018 07:42

soyuka mentioned this pull request Sep 7, 2018

Swagger Documentation ignore @ApiProperty(writableLink=true) #2192

Closed

soyuka mentioned this pull request Oct 24, 2018

Embed objects and serialization groups #1001

Closed

fix #1980 user defined property metadata takes precedence #1997

fix #1980 user defined property metadata takes precedence #1997

Conversation

soyuka commented Jun 3, 2018

soyuka commented Jun 3, 2018

oxan commented Jun 3, 2018

oxan commented Jun 3, 2018

soyuka commented Jun 4, 2018

oxan commented Jun 4, 2018

dunglas Jun 4, 2018

Choose a reason for hiding this comment

dunglas Jun 4, 2018

Choose a reason for hiding this comment

bendavies commented Jun 4, 2018

dunglas commented Jun 4, 2018

bendavies commented Jun 4, 2018

dunglas commented Jun 4, 2018

bendavies commented Jun 4, 2018 • edited Loading

soyuka commented Jun 5, 2018

oxan commented Jun 5, 2018

teohhanhui commented Jun 5, 2018 • edited Loading

soyuka commented Jun 5, 2018

bendavies commented Jun 5, 2018

bendavies commented Jun 5, 2018

Nek- commented Jun 5, 2018

soyuka commented Jun 5, 2018

teohhanhui commented Jun 5, 2018

bendavies commented Jun 5, 2018

oxan commented Jun 5, 2018

oxan commented Jun 5, 2018

bendavies commented Jun 5, 2018

Nek- commented Jun 5, 2018 • edited Loading

soyuka commented Jun 6, 2018

soyuka Jul 10, 2018

Choose a reason for hiding this comment

teohhanhui Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

soyuka Jul 19, 2018

Choose a reason for hiding this comment

teohhanhui Jul 23, 2018

Choose a reason for hiding this comment

soyuka commented Jul 10, 2018 • edited Loading

soyuka commented Jul 25, 2018

bendavies commented Aug 22, 2018

dunglas commented Aug 22, 2018

bendavies commented Jun 4, 2018 •

edited

Loading

teohhanhui commented Jun 5, 2018 •

edited

Loading

Nek- commented Jun 5, 2018 •

edited

Loading

teohhanhui Jul 16, 2018 •

edited

Loading

soyuka commented Jul 10, 2018 •

edited

Loading