Fix strict typing exception when the propertyName is a number #1055

jocel1 · Apr 13, 2017

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#282
License	MIT
Doc PR

soyuka · Apr 13, 2017

src/Metadata/Property/Factory/AnnotationPropertyNameCollectionFactory.php

@@ -63,7 +63,7 @@ public function create(string $resourceClass, array $options = []): PropertyName
        // Properties
        foreach ($reflectionClass->getProperties() as $reflectionProperty) {
            if (null !== $this->reader->getPropertyAnnotation($reflectionProperty, ApiProperty::class)) {
-                $propertyNames[$reflectionProperty->name] = true;
+                $propertyNames[$reflectionProperty->name] = (string)$reflectionProperty->name;


Can't we keep keys? $propertyNames[(string) $reflectionProperty->name] should work no?

I studied this a bit more, looks like the fix should be in PropertyNameCollection:

--- a/src/Metadata/Property/PropertyNameCollection.php +++ b/src/Metadata/Property/PropertyNameCollection.php @@ -30,7 +30,9 @@ final class PropertyNameCollection implements \IteratorAggregate, \Countable */ public function __construct(array $properties = []) { - $this->properties = $properties; + $this->properties = array_map(function($property) { + return (string) $property; + }, $properties); }

For the key unfortunately php is converting the key to int, even when (string) is forced

Indeed, still:

<?php $t = []; $t[118] = 'foo'; $t['bar'] = 'foo'; var_dump(array_map(function($v) { return (string) $v; }, array_keys($t)));

Outputs:

array(2) { [0]=> string(3) "118" [1]=> string(3) "bar" }

yup a centralized array_map is definitely better to handle this issue

I'm not a fond of the centralized array_map solution. It looks hackish... This is not the reponsibility of PropertyNameCollection to fix the data provided by friend classes. Other classes should provide data respecting the contract (an array of strings).

soyuka · Apr 13, 2017

For the test, I'd recommend to create a new fixture class (avoid editing Dummy, too many tests are based on it at one change will require lots of changes :p).
You can add a test in this class by using the new fixture, would be just fine!

dunglas

I like your solution. It's elegant and faster than array_map. I just left a minor comment regarding the useless cast. Thank you very much for fixing this!

dunglas · Apr 13, 2017

src/Metadata/Property/Factory/AnnotationPropertyNameCollectionFactory.php

@@ -63,7 +63,7 @@ public function create(string $resourceClass, array $options = []): PropertyName
        // Properties
        foreach ($reflectionClass->getProperties() as $reflectionProperty) {
            if (null !== $this->reader->getPropertyAnnotation($reflectionProperty, ApiProperty::class)) {
-                $propertyNames[$reflectionProperty->name] = true;
+                $propertyNames[$reflectionProperty->name] = (string)$reflectionProperty->name;


$reflectionProperty->name is a always a string, so the cast isn't necessary (same everywhere).

So we can make this even faster by using $propertyNames[] = $reflectionProperty->name; if we rely on values :).

Not really because we must ensure that properties are unique. If you use values, you'll need to call in_array and it's an O(n) operation instead of O(1).

Simperfit · Apr 13, 2017

src/Metadata/Property/Factory/AnnotationPropertyNameCollectionFactory.php

@@ -63,7 +63,7 @@ public function create(string $resourceClass, array $options = []): PropertyName
        // Properties
        foreach ($reflectionClass->getProperties() as $reflectionProperty) {
            if (null !== $this->reader->getPropertyAnnotation($reflectionProperty, ApiProperty::class)) {
-                $propertyNames[$reflectionProperty->name] = true;
+                $propertyNames[$reflectionProperty->name] = $reflectionProperty->name;


Key may be useless now ?

It's not, it ensures the uniqueness.

jocel1 · Apr 13, 2017

I've just pushed the required tests

soyuka · Apr 13, 2017

@jocel1 may you change the target branch to 2.0 and rebase? (If you're not sure about how to rebase lmk).

jocel1 · Apr 13, 2017

rebase & squash done

dunglas · Apr 13, 2017

src/Metadata/Property/Factory/AnnotationPropertyNameCollectionFactory.php

@@ -81,7 +81,7 @@ public function create(string $resourceClass, array $options = []): PropertyName
            }

            if (null !== $propertyName && null !== $this->reader->getMethodAnnotation($reflectionMethod, ApiProperty::class)) {
-                $propertyNames[$propertyName] = true;
+                $propertyNames[$propertyName] = (string) $propertyName;


This cast can be removed too.

dunglas · Apr 13, 2017

src/Metadata/Property/Factory/InheritedPropertyNameCollectionFactory.php

@@ -41,7 +41,7 @@ public function create(string $resourceClass, array $options = []): PropertyName

        // Inherited from parent
        foreach ($this->decorated->create($resourceClass, $options) as $propertyName) {
-            $propertyNames[$propertyName] = true;
+            $propertyNames[$propertyName] = (string) $propertyName;


This cast can be removed.

mm no, this is this one in my case which was causing the issue : the symfony component PropertyInfoExtractor::getProperties() called through the PropertyInfoPropertyNameCollectionFactory returns an int instead of a string

It looks like the Symfony component suffers from the same bug. We may keep this fix temporarily but we should fix this behavior in Symfony as well then bump the minimal version required by API Platform.

Do you want to handle this? If you're out of time I can open the Symfony PR.

If you can handle this one, I'm definitely out of my time ;)

dunglas · Apr 13, 2017

Travis and AppVeyor seem to have some troubles. Can you rebase and push force again?

dunglas · Apr 13, 2017

Thank you very much @jocel1!

…(dunglas) This PR was merged into the 2.8 branch. Discussion ---------- [PropertyInfo] Prevent returning int values in some cases | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | api-platform/api-platform#282, api-platform/core#1055 | License | MIT | Doc PR | n/a PHP automatically converts array keys to an int if and only if it looks like an int... When a getter looks like `get123`, the ReflectionExtractor returns an array containing an int instead of a string. This PR fixes this. Commits ------- b190ec2 [PropertyInfo] Prevent returning int values in some cases.

…atform#1055)

soyuka reviewed Apr 13, 2017

View reviewed changes

dunglas requested changes Apr 13, 2017

View reviewed changes

Simperfit reviewed Apr 13, 2017

View reviewed changes

soyuka approved these changes Apr 13, 2017

View reviewed changes

Fix strict typing exception when the propertyName is a number

bceb2cd

jocel1 changed the base branch from master to 2.0 April 13, 2017 15:45

dunglas requested changes Apr 13, 2017

View reviewed changes

remove useless cast

4341db9

dunglas approved these changes Apr 13, 2017

View reviewed changes

dunglas merged commit 1365e44 into api-platform:2.0 Apr 13, 2017

dunglas mentioned this pull request Apr 13, 2017

[PropertyInfo] Prevent returning int values in some cases symfony/symfony#22426

Merged

teohhanhui added the bug label Apr 17, 2017

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Fix strict typing exception when the propertyName is a number (api-pl…

a13d7fd

…atform#1055)

Search code, repositories, users, issues, pull requests...

Uh oh!

Fix strict typing exception when the propertyName is a number #1055

Fix strict typing exception when the propertyName is a number #1055

Uh oh!

Conversation

jocel1 commented Apr 13, 2017 • 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!

soyuka commented Apr 13, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jocel1 commented Apr 13, 2017

Uh oh!

soyuka commented Apr 13, 2017

Uh oh!

jocel1 commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jocel1 Apr 13, 2017 • 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!

dunglas commented Apr 13, 2017

Uh oh!

dunglas commented Apr 13, 2017

Uh oh!

Uh oh!

jocel1 commented Apr 13, 2017 •

edited

Loading

jocel1 Apr 13, 2017 •

edited

Loading