Bad query parameters when the identifier is a Doctrine Type in Doctrine ItemDataProvider:getItem

[magarzon]

Hi.

I found an issue in doctrine ItemDataProvider introduced in v2.0.8.

In the doctrine ItemDataProvider, when is getting an item, is building a query using the identifiers in the request.

To get the identifiers, first it normalizes them, because can be another than id and even can be two fields

This normalization is performed now by a trait, IdentifierManagerTrait. So, in this line: https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Util/IdentifierManagerTrait.php#L76 is converting the value of the identifier, if it has a Doctrine type, to its PHP value.

I don't know if this it's needed, because eventually this value is used to query the database, and it should be the database value for that type, not the PHP value, that can be even an object.

If the reason to add this is some kind of conversion of the input to the PHP value, there is another step missing: In the addWhereForIdentifiers method in ItemDataProvider, when is setting the parameter in the query builder, it should check again the type (as it's done in the trait), and add it as the third parameter:
$queryBuilder->setParameter($placeholder, $value, $type);

If you don't do that, the default type is string, and then doctrine doesn't call Type::convertToDatabaseValue and instead of the right value for that parameter/type, the value could be (as in my case, that is an object), the classname.

So, or either remove the conversion in the trait if it's not really needed, or the type must be added as I explained.

[Simperfit]

@magarzon I think this is a bug, do you mind opening a PR to fix it ?

[magarzon]

@Simperfit ok, but I don't know which solution it's better, because I don't know what was the final purpose of this change

[soyuka]

May I ask what Type are identifiers in your case?

[magarzon]

In my project the username is always a mobile. We need to make things like restrict registration to certain countries and operators, that the user can introduce his username with or without prefix, or using a 0 instead the prefix (in some countries they're get used to this), but the final username in the database must be a "normalized" one with the country prefix. For all that (and more) we use this bundle: https://github.com/misd-service-development/phone-number-bundle, that comes with validators and a custom doctrine type that is able to do all that stuff "automatically". This custom doctrine type at the end generates an object of a certain class, to handle of the conversions, validations, etc.

And in the routes we want to use the mobile as id, for many reasons (the id is autoincremental, we don't want the user knows how many other users are, for example). But as this field is of that custom type defined by the bundle, we found this problem.

[soyuka]

This should be fixed with the implementation of identifiers denormalizer.