Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Fix integer Identifiers denormalization #1899

Merged
merged 1 commit into from
Apr 29, 2018

Conversation

antograssiot
Copy link
Contributor

@antograssiot antograssiot commented Apr 27, 2018

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

Right now the Identifier Denormalizer will automatically result in the identifier being casted to string.
This basicaly break the search filter on related entities with a strict typehint on the getId() because when getting references to the object in the getItemFromIri() the call to getId() will result in a TypeError.

ping @soyuka

@antograssiot antograssiot force-pushed the fix-identifier-casting branch 3 times, most recently from 21080a5 to 9a29741 Compare April 27, 2018 14:48
composer.json Outdated
@@ -34,7 +34,7 @@
"behatch/contexts": "^3.1",
"doctrine/annotations": "^1.2",
"doctrine/doctrine-bundle": "^1.8",
"doctrine/orm": "^2.5.2",
"doctrine/orm": "^2.6.0",
Copy link
Contributor Author

@antograssiot antograssiot Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doctrine/orm#4884 erf php7.0 support I can't bump to 2.6.0 so the lower-deps will remain failling 😢

@soyuka
Copy link
Member

soyuka commented Apr 27, 2018

Can we introduce a DoctrineIdenfitierDenormalizer (checks metadata column type and calls Type::convertToPhpValue) instead? The string is because the identifier comes from the Request and type is unknown at that time.

@@ -30,7 +30,7 @@ class ContainNonResource
/**
* @var mixed
*
* @ORM\Column(type="integer")
* @ORM\Column(type="string")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the change here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test result instead

@antograssiot
Copy link
Contributor Author

antograssiot commented Apr 27, 2018

Can we introduce a DoctrineIdenfitierDenormalizer

I can do that. But this denormalizer will still only cast string to int so would be a DoctrineIntegerTypeDenormalizer ? it feels a bit heavy but will allow customization, is it your concern @soyuka ?

@soyuka
Copy link
Member

soyuka commented Apr 27, 2018

But this denormalizer will still only cast string to int so would be a

It would handle every doctrine types, not necessarily strings.

After a second though we have everything we need for this, just do an IdentifierIntegerDenormalizer (the second argument of denormalize === Type::BUILTIN_TYPE_INT). Doctrine isn't responsible IIUC.

@antograssiot antograssiot force-pushed the fix-identifier-casting branch 2 times, most recently from edf765d to aa43057 Compare April 27, 2018 20:31
@antograssiot antograssiot changed the title [Bug] Restore identifiers casting to integer when necessary [Bug] Fix integer Identifiers denormalization Apr 27, 2018
@antograssiot
Copy link
Contributor Author

antograssiot commented Apr 27, 2018

I've update the code @soyuka.

I've no clue on how to please the travis build with low-dependencies as I hit a bug with doctrine. I could be to revert the change done on the TypeHint of DummyFriend, but that was the easiest way to have a failing behat scenario... found another way to ensure non-regression in behat

Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful 👍 once green

@soyuka
Copy link
Member

soyuka commented Apr 27, 2018

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - This package requires php >=7.0 but your PHP version (5.6.17) does not satisfy that requirement.
  Problem 2
    - php-mock/php-mock-phpunit 2.1.1 requires php >=7 -> your PHP version (5.6.17) does not satisfy that requirement.
    - php-mock/php-mock-phpunit 2.1.0 requires php >=7 -> your PHP version (5.6.17) does not satisfy that requirement.
    - php-mock/php-mock-phpunit 2.0.1 requires php >=7 -> your PHP version (5.6.17) does not satisfy that requirement.
    - php-mock/php-mock-phpunit 2.0.0 requires php >=7 -> your PHP version (5.6.17) does not satisfy that requirement.
    - Installation request for php-mock/php-mock-phpunit ^2.0 -> satisfiable by php-mock/php-mock-phpunit[2.0.0, 2.0.1, 2.1.0, 2.1.1].

weird

@antograssiot
Copy link
Contributor Author

Circle-ci is drunk and waiting for config from 2.2 branch, travis should be good

@soyuka
Copy link
Member

soyuka commented Apr 29, 2018

I think that this should be done on 2.2 because it's a bug fix. May you rebase?

@antograssiot antograssiot changed the base branch from master to 2.2 April 29, 2018 06:25
@antograssiot
Copy link
Contributor Author

@soyuka rebased and target branch updated. I failled to see when this got into 2.2, I was pretty sure your initial MRs (#1820) were against into master sorry.

@soyuka soyuka merged commit 5335963 into api-platform:2.2 Apr 29, 2018
@soyuka
Copy link
Member

soyuka commented Apr 29, 2018

thanks @antograssiot

@antograssiot antograssiot deleted the fix-identifier-casting branch April 29, 2018 12:22
@sroze
Copy link
Contributor

sroze commented May 2, 2018

This definitely should have been based on master :)

@soyuka
Copy link
Member

soyuka commented May 2, 2018

@antograssiot could you reopen a merge request on master with this patch please? Really sorry for the mistake, we're all humans I guess 😄

@soyuka
Copy link
Member

soyuka commented May 2, 2018

Actually, just restore your branch with the github button so that I'm sure we have the patch on your remote and I'll take care of it :).

@antograssiot
Copy link
Contributor Author

I'll have a look tonight

@antograssiot antograssiot restored the fix-identifier-casting branch May 2, 2018 16:18
@antograssiot
Copy link
Contributor Author

I restored the branch, I'll rebase it onto master and open a new PR, is that ok ?

@teohhanhui
Copy link
Contributor

Yes, it's necessary to open a new PR because the merge functionality is gone for an already merged PR.

@soyuka
Copy link
Member

soyuka commented May 2, 2018

1ea6f68 everything back to normal :)

@antograssiot antograssiot deleted the fix-identifier-casting branch May 2, 2018 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants