[Serializer] Add template parameter to avoid necessity to assert data in normalize #59200

danrot · Dec 12, 2024

Q	A
Branch?	TBH I am not sure about this one 😅
Bug fix?	Also not sure...
New feature?	no
Deprecations?	maybe
Issues	Didn't create one, since the code change is minimal anyway
License	MIT

In a project I am working on we are using quite some value objects, and I also want to normalize some of them. Instead of repeating the serialization part for every object, I like to create a separate ValueObjectNormalizer (whereby I use ValueObject as a placeholder here and in the below code).

We are also using PHPStan with its highest level, and then there are some "issues" with that. The problem is that I always have to add a call to assert in the normalize function, since the $data variable is typed as mixed, and whatever I am doing with the passed data PHPStan won't like. This looks something like this:

<?php

namespace App\Serializer;

use App\Domain\ValueObject;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

final class ValueObjectNormalizer implements NormalizerInterface
{
	public function normalize(mixed $object, null|string $format = null, array $context = []): string
	{
		\assert($object instanceof ValueObject);

		return $object->someValueObjectProperty;
	}

	public function supportsNormalization(mixed $data, null|string $format = null, array $context = []): bool
	{
		return $data instanceof ValueObject;
	}

	public function getSupportedTypes(null|string $format): array
	{
		return [
			ValueObject::class => true,
		];
	}
}

With the change being introduced in this PR, it would be possible to make use of generics in order to avoid this assert call, which I think is a bit nicer.

<?php

declare(strict_types=1);

namespace App\Serializer;

use App\Domain\ValueObject;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

/**
 * @implements NormalizerInterface<ValueObject>
 */
final class ValueObjectNormalizer implements NormalizerInterface
{
	public function normalize(mixed $object, null|string $format = null, array $context = []): string
	{
		return $object->someValueObjectProperty;
	}

	public function supportsNormalization(mixed $data, null|string $format = null, array $context = []): bool
	{
		return $data instanceof ValueObject;
	}

	public function getSupportedTypes(null|string $format): array
	{
		return [
			ValueObject::class => true,
		];
	}
}

What do you think about this? I am also not sure in which branch such a change would have to go, and I am also not sure if you consider this a breaking change.

carsonbot · Dec 12, 2024

Hey!

Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "TBH I am not sure about this one 😅".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

danrot · Dec 12, 2024

I already saw that this causes some issues... But instead of fixing all of that now and occupying github action pipelines I'll wait for your feedback to see if you would accept this PR.

nicolas-grekas · Jan 7, 2025

src/Symfony/Component/Serializer/Normalizer/NormalizerInterface.php

@@ -41,8 +43,8 @@ public function normalize(mixed $data, ?string $format = null, array $context =
    /**
     * Checks whether the given class is supported for normalization by this normalizer.
     *
-     * @param mixed       $data   Data to normalize
-     * @param string|null $format The format being (de-)serialized from or into
+     * @param TData|mixed   $data   Data to normalize


why keep mixed?

For supportsNormalization(), it might presumably receive all sorts of data (since it's testing for serialization support).

The idea is that (for example) NormalizerInterface<string> will only normalize() strings, but still might receive other types to test for serialization (otherwise (NormalizerInterface<string>)->supportsNormalization(123, ...) will cause a static analysis tool warning, which is undesirable and unintended).

I guess you could argue that mixed already contains TData as well, so we might as well just keep mixed as a type.

wouterj · Jan 7, 2025

Looking at the normalizers shipped in Symfony, they all verify the passed data to normalize()/denormalize() is indeed what is expected (and throw a serializer exception otherwise)

So to me, it sounds like it is a good practice to verify the type in the method itself rather than relying on the return value of supportsNormalization()/getSupportedTypes()?

uuf6429 · Jan 7, 2025

Looking at the normalizers shipped in Symfony, they all verify the passed data to normalize()/denormalize() is indeed what is expected (and throw a serializer exception otherwise)

@wouterj I just checked two normalizers randomly and that doesn't seem to be the case (unless I misunderstood your point):

symfony/src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php

Line 46 in 23fd2eb

public function normalize(mixed $object, ?string $format = null, array $context = []): array

(line 62 assumes mixed $object is a list of violations - but we know it should be ConstraintViolationListInterface)
symfony/src/Symfony/Component/Messenger/Transport/Serialization/Normalizer/FlattenExceptionNormalizer.php

Line 29 in 36a2bb1

public function normalize(mixed $object, ?string $format = null, array $context = []): array

- same as previous case, mixed $object is assumed to be a FlattenException instance.

I don't know how/if psalm/phpstan is set up, but in either case I would have expected a warning that the usage of $object was not expected (first case not iterable, second case undefined symbol getMessage() etc).

With this PR (and some small adjustments), static analysis tools should figure out $object's type, and modern IDEs should be able to provide autocompletion with normalize() (and possibly denormalize() - but that's not covered in this PR - yet) - (without having to redeclare the PHPDoc for (de)normalise()).

People should still be able to opt-out by using @implements NormalizerInterface<mixed>.

wouterj · Jan 7, 2025

Interesting, so we are inconsistent about this in Symfony. See e.g.

symfony/src/Symfony/Component/Serializer/Normalizer/DateIntervalNormalizer.php

Lines 46 to 50 in 23fd2eb

    
           public function normalize(mixed $object, ?string $format = null, array $context = []): string 
        
           { 
        
               if (!$object instanceof \DateInterval) { 
        
                   throw new InvalidArgumentException('The object must be an instance of "\DateInterval".'); 
        
               }

and

symfony/src/Symfony/Component/Serializer/Normalizer/DataUriNormalizer.php

Lines 50 to 54 in 23fd2eb

    
           public function normalize(mixed $object, ?string $format = null, array $context = []): string 
        
           { 
        
               if (!$object instanceof \SplFileInfo) { 
        
                   throw new InvalidArgumentException('The object must be an instance of "\SplFileInfo".'); 
        
               }

danrot · Jan 8, 2025

I have to admit that this is kind of an edge case. It is not absolutely safe to just assume the type passed to normalize is the one we check for in supportsNormalization, since somebody might as well instantiate a ValueObjectNormalizer on their own and call normalize without calling supportsNormalization before.

I.e. this is probably how Symfony is calling this under the hood:

$valueObjectNormalizer = new ValueObjectNormalizer();

if ($valueObjectNormalizer->supportsNormalization($value)) {
    $valueObjectNormalizer->normalize($value);
}

But nothing stops a developer from instantiating that manually and call it in the "wrong" way:

$valueObjectNormalizer = new ValueObjectNormalizer();

$valueObjectNormalizer->normalize($value);

Nevertheless, I would say this is an edge case, and at least for me the current PR would still be a nice addition, since it is closer to how this is actually working under the hood.

Add template parameter to avoid necessity to assert data in normalize

23fd2eb

danrot requested a review from dunglas as a code owner December 12, 2024 15:22

carsonbot added Status: Needs Review labels Dec 12, 2024

carsonbot added this to the 7.3 milestone Dec 12, 2024

nicolas-grekas reviewed Jan 7, 2025

View reviewed changes

nicolas-grekas added the Serializer label Jan 7, 2025

carsonbot changed the title ~~Add template parameter to avoid necessity to assert data in normalize~~ [Serializer] Add template parameter to avoid necessity to assert data in normalize Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Add template parameter to avoid necessity to assert data in normalize #59200

[Serializer] Add template parameter to avoid necessity to assert data in normalize #59200

danrot commented Dec 12, 2024

carsonbot commented Dec 12, 2024

danrot commented Dec 12, 2024

nicolas-grekas Jan 7, 2025

uuf6429 Jan 7, 2025 •

edited

Loading

danrot Jan 8, 2025

wouterj commented Jan 7, 2025

uuf6429 commented Jan 7, 2025 •

edited

Loading

wouterj commented Jan 7, 2025

danrot commented Jan 8, 2025

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

[Serializer] Add template parameter to avoid necessity to assert data in normalize #59200

Are you sure you want to change the base?

[Serializer] Add template parameter to avoid necessity to assert data in normalize #59200

Conversation

danrot commented Dec 12, 2024

carsonbot commented Dec 12, 2024

danrot commented Dec 12, 2024

nicolas-grekas Jan 7, 2025

Choose a reason for hiding this comment

uuf6429 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

danrot Jan 8, 2025

Choose a reason for hiding this comment

wouterj commented Jan 7, 2025

uuf6429 commented Jan 7, 2025 • edited Loading

wouterj commented Jan 7, 2025

danrot commented Jan 8, 2025

uuf6429 Jan 7, 2025 •

edited

Loading

uuf6429 commented Jan 7, 2025 •

edited

Loading