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

Events with arrays not deserialized correctly #189

Merged
merged 5 commits into from Jan 21, 2021

Conversation

morrislaptop
Copy link
Contributor

No description provided.

@freekmurze
Copy link
Member

@brendt could you take a look at this one?

@morrislaptop
Copy link
Contributor Author

Looking at this as well, I'm currently exploring - symfony/symfony#25524

@brendt
Copy link
Collaborator

brendt commented Jan 14, 2021

The first thing I'd normally do is create my own simple JsonSerializer and use that one instead of the one provided by the package (using Symfony's underneath).

It looks something like this:

class JsonSerializer implements EventSerializer
{
    public function serialize(ShouldBeStored $event): string
    {
        return json_encode(['event' => serialize($event)]);
    }

    public function deserialize(string $eventClass, string $json, string $metadata = null): ShouldBeStored
    {
        return unserialize(json_decode($json, true)['event']);
    }
}

I think it's a matter of coding style: I've gotten used to sanitising my data before constructing an event, so I'm pretty sure I'm always dealing with something that's json serializable.

That's just some background information if someone were to stumble upon this PR and needed a quick fix.

With regards to the PR: do you know why the problem is happening and how it's fixed?

@morrislaptop
Copy link
Contributor Author

morrislaptop commented Jan 14, 2021

@brendt I think that JSON serializer would only work with scalar types - removing all the benefits of the Symfony serializer.

To specify array types in PHP we have to use a docblock as generics aren't supported. By default the Symfony serializer only reads the strongly typed properties of the class and not docblocks - without a type it just dumps the array into the deserialized object rather than denormalizing it into a collection of objects. The Symfony serializer also needs the ArrayDenormalizer which picks up on the [] suffix to indicate an array.

The solution is to add the ArrayDenormalizer and use a custom ObjectNormalizer in config/event-sourcing.php

'event_normalizers' => [
        Spatie\EventSourcing\Support\CarbonNormalizer::class,
        Spatie\EventSourcing\Support\ModelIdentifierNormalizer::class,
        Symfony\Component\Serializer\Normalizer\DateTimeNormalizer::class,
        Symfony\Component\Serializer\Normalizer\ArrayDenormalizer::class,
        Spatie\EventSourcing\Support\ObjectNormalizer::class,
    ],
<?php

namespace Spatie\EventSourcing\Support;

use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor;
use Symfony\Component\PropertyInfo\PropertyTypeExtractorInterface;
use Symfony\Component\Serializer\NameConverter\NameConverterInterface;
use Symfony\Component\Serializer\Mapping\ClassDiscriminatorResolverInterface;
use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer as SymfonyObjectNormalizer;

class ObjectNormalizer extends SymfonyObjectNormalizer
{
    public function __construct(ClassMetadataFactoryInterface $classMetadataFactory = null, NameConverterInterface $nameConverter = null, PropertyAccessorInterface $propertyAccessor = null, PropertyTypeExtractorInterface $propertyTypeExtractor = null, ClassDiscriminatorResolverInterface $classDiscriminatorResolver = null, callable $objectClassResolver = null, array $defaultContext = []) {
        parent::__construct(
            $classMetadataFactory,
            $nameConverter,
            $propertyAccessor,
            $propertyTypeExtractor ?? new PhpDocExtractor(),
            $classDiscriminatorResolver,
            $objectClassResolver,
            $defaultContext
        );
    }
}

The above can be done in user land for sure, but might be an expectation of the package that this would work by default.

@brendt
Copy link
Collaborator

brendt commented Jan 15, 2021

Gotcha! @freekmurze I'm fine adding this, you as well?

FYI the event itself is serialized using PHP's serialize method, so it can handle objects as well.

@freekmurze
Copy link
Member

Yup, I'm ok with this too! 👍

@brendt brendt merged commit d5f2c4f into spatie:master Jan 21, 2021
@brendt
Copy link
Collaborator

brendt commented Jan 21, 2021

@mbardelmeijer
Copy link

This seems to break if phpdocumentor/reflection-docblock isn't installed in production.

Seems to be a breaking change. Perhaps phpdocumentor/reflection-docblock should be added to the require part of this package?

After updating to 4.7.1, the following exception is thrown in production:
Unable to use the "Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor" class as the "phpdocumentor/reflection-docblock" package is not installed.

@brendt
Copy link
Collaborator

brendt commented Jan 28, 2021

@mbardelmeijer very sorry for the inconvenience. 4.7.2 should fix it: https://github.com/spatie/laravel-event-sourcing/releases/tag/4.7.2

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.

None yet

4 participants