Integer denormalizer is numeric

[soullivaneuh]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2116 (cc @blesenechal, please confirm)
License	MIT
Doc PR	N/A

Concrete case:

final class ServerDataProvider implements ItemDataProviderInterface, RestrictedDataProviderInterface
{
    /**
     * @var EntityManagerInterface
     */
    private $entityManager;

    public function __construct(EntityManagerInterface $entityManager)
    {
        $this->entityManager = $entityManager;
    }

    /**
     * {@inheritdoc}
     */
    public function getItem(string $resourceClass, $id, ?string $operationName = null, array $context = []): ?Server
    {
        $serverRepository = $this->entityManager->getRepository(Server::class);
        $server = $serverRepository->find($id);

        if (null === $server) {
            $server = $serverRepository->findOneBy([
                'name' => $id,
            ]);
        }

        return $server;
    }

    /**
     * {@inheritdoc}
     */
    public function supports(string $resourceClass, ?string $operationName = null, array $context = []): bool
    {
        return Server::class === $resourceClass;
    }
}

This permit to try find a server by it's name if nothing is found with the id.

But in the current stable version, the string value will always be 0 because it's converted into string by the denormalizer.

Current workaround with a decorated service:

/**
 * @see https://github.com/api-platform/core/issues/2116#issuecomment-437909022
 * @see https://github.com/api-platform/core/pull/2323
 */
final class IntegerDenormalizer implements DenormalizerInterface, CacheableSupportsMethodInterface
{
    /**
     * @var \ApiPlatform\Core\Identifier\Normalizer\IntegerDenormalizer
     */
    private $decorated;

    public function __construct(\ApiPlatform\Core\Identifier\Normalizer\IntegerDenormalizer $decorated)
    {
        $this->decorated = $decorated;
    }

    /**
     * {@inheritdoc}
     */
    public function denormalize($data, $class, $format = null, array $context = array())
    {
        return $this->decorated->denormalize($data, $class, $format, $context);
    }

    /**
     * {@inheritdoc}
     */
    public function supportsDenormalization($data, $type, $format = null)
    {
        return $this->decorated->supportsDenormalization($data, $type, $format) && \is_numeric($data);
    }

    public function hasCacheableSupportsMethod(): bool
    {
        return $this->decorated->hasCacheableSupportsMethod();
    }
}

[dunglas]

Looks legit to me, but let's wait for @soyuka's review. It should target 2.3 as it's a bug fix.

[antograssiot]

@soullivaneuh Do you still have the issue if your ItemProvider implements the DenormalizedIdentifiersAwareItemDataProviderInterface ?
That's the "latest" way to do it if I remember correctly, you will get $identifiers as an array and should not need this modification.

[blesenechal]

@soullivaneuh : This fixes my issue (#2116)
@antograssiot : Implementing DenormalizedIdentifiersAwareItemDataProviderInterface does not fix the issue for me, this was already suggested by @soyuka (see #2116 (comment))

[soyuka]

The issue here is that the $type should not be Type::BUILTIN_TYPE_INT === $type, this way it won't get transformed to an integer right?

IMHO this use case is wrong, you can't use two different types for a given identifier. Use a custom operation if you want to fetch by name.

Or, you can always add another IdentifierNormalizer that gets called before the Integer one to "hack around" this.

[soyuka]

Actually thought a bit more about this and I'm okay (if we can avoid it it's better) to use is_numeric instead of is_string.

Also please check out #2126 which would be a better way of fixing this.

Then again, please don't do the following:

 $server = $serverRepository->find($id);

        if (null === $server) {
            $server = $serverRepository->findOneBy([
                'name' => $id,
            ]);
        }

The identifier of this route is an id not a name, it just looks wrong and dirty 😛

[soullivaneuh]

It should target 2.3 as it's a bug fix.

OFC, I didn't see the master target using the link from the git command line, sorry.

@antograssiot Using DenormalizedIdentifiersAwareItemDataProviderInterface does not change anything. I indeed have an array but with "id" => 0.

@soyuka I agree it's not very clean, but the use case isn't clean neither. On the server resource, I need the identifier to be the id OR the name for every opertion. Using custom operations will be overkill.

Do you have more details about the IdentifierNormalizer? This may be a subject for a documentation issue.

And, as you said on the last comment, in any case a IntegerDenormalizer should not denormalize something that does not look like an integer at all, so I think this PR is still valuable. 👍

[antograssiot]

understood, it looks okish to me 👍
hopefully you won't get a name that will be "7.2" 😉

@soyuka is it still necessary to check the is_string here to avoid to cast values that are already integer, or it should not happen nor is a big deal ? ctype_digit will handle that

[soullivaneuh]

hopefully you won't get a name that will be "7.2"

Hmm... it is not an integer either. :D

Maybe use this instead? http://php.net/manual/fr/function.ctype-digit.php

[soullivaneuh]

Updated the PR with ctype_digit.

[soyuka]

Do you have more details about the IdentifierNormalizer? This may be a subject for a documentation issue.

https://api-platform.com/docs/core/identifiers/#custom-identifier-normalizer

[soyuka]

This is so untrue :|. How can an identifier be a string but typed as an integer?

[soullivaneuh]

Stop me if I'm wrong, but I think the identified type come from PHPDoc parsing and class reflection.

[soyuka]

Yes, it's linked to the property in your class. This is why it's wrong because if you typed id as int there are no reasons it should be a string ^^.

[soullivaneuh]

Dirty solution, but works easier than the proper one. Maybe an issue may be open about OR conditions on identifier.

[soyuka]

Because the identifier should be unique, there should not be any or imho.

[soullivaneuh]

@soyuka I read this documentation, but the sample show a single identifier related to an object. I just want to check on two field, one int and one string. I don't see how to achieve it this way.

[soyuka]

For example:

<?php
namespace App\Identifier;

use ApiPlatform\Core\Exception\InvalidIdentifierException;
// assumes you indeed typed your property with
// * @IntOrString
use App\Type\IntOrString;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;

final class IntOrStringNormalizer implements DenormalizerInterface
{
    public function denormalize($data, $class, $format = null, array $context = [])
    {
        return is_numeric($data) ? (int) $data : $data;
    }

    public function supportsDenormalization($data, $type, $format = null)
    {
        // you could also check if the $data is implementing an interface or just return `true` to override default normalizers 
        return is_a($type, IntOrString::class, true);
    }
}

[soullivaneuh]

@soyuka Your solution will not work for me because I have two field. Here, you assume I have one field with the IntOrString type, am I right?

[soyuka]

Where do you have 2 fields? It'll work as a workaround because the normalizer above won't denormalize your identifier.
The example above works with an entity of type:

class Entity {
  /**
   * @var IntOrString
   */
  public $id;
}

And you can then do (even though the following is bad design :p):

 $server = $serverRepository->find($id);

if (null === $server) {
    $server = $serverRepository->findOneBy([
        'name' => $id,
    ]);
}

Or even better now that you're sure that the string will be an integer:

if (is_string($id)) {
  return $repo->findOneByName($id);
}

return $repo->find($id);

[soullivaneuh]

Where do you have 2 fields?

See #2324 (comment)

@soyuka At final, it the same thing, looking for two field with the same value.

You just advise me to create a special type with a custom normalizer just to make string and integer working. Here, the integer normalizer does not work as excpected as it should not convert anything that does not looks like an integer. Otherwise, you may accept any value.

And if nothing is able to be normalized, then I'll have the raw value. And that works for a string. ;-)

[soyuka]

You just advise me to create a special type with a custom normalizer just to make string and integer working.

Yes.

Here, the integer normalizer does not work as excpected as it should not convert anything that does not looks like an integer.

id in the url matches the id property, which is typed as an integer, so it should be transformed as one. If you take a date for example, it's even truer because if it's not converted it'll be error-prone.

[soullivaneuh]

So the result is the same. Having one type allowing int or string, placed on a property that should be an int, and search for a name. Same ugly thing. 😉

id in the url matches the id property, which is typed as an integer, so it should be transformed as one.

Well, in that case, why do we have this test:

$this->assertFalse($normalizer->supportsDenormalization([], 'int'));

And why we already check value to be a string? 🤔

With this PR, I'm looking at the normalizer itself and nothing else. It's an integer normalizer, so it should convert intergers only. Or, rename it to ApiPropertyIntNormalizer in that case. :-)

Do you better see my point of view?

[teohhanhui]

I think @soyuka has explained it pretty well. So I'm -1 on this.

[soullivaneuh]

I really don't understand here. In any case, why an integer normalizer should normalize something that does not represent an integer at all?

Even if my final needs does not fit your best practice, it's still an issue, and not related.