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

How to pass MetadataFactory::create() into SerializationVisitorInterface::startVisitingObject()? #1226

Closed
simPod opened this issue Jul 4, 2020 · 8 comments

Comments

@simPod
Copy link
Contributor

simPod commented Jul 4, 2020

I'm creating metadata for class through $classMetadata = $context->getMetadataFactory()->getMetadataForClass(My::class);

interface MetadataFactoryInterface
{
    /**
     * @return ClassHierarchyMetadata|MergeableClassMetadata|null
     */
    public function getMetadataForClass(string $className);

So the return type is ClassHierarchyMetadata|MergeableClassMetadata|null

Though SerializationVisitorInterface::startVisitingObject() expects ClassMetadata which is not contravariant to what MetadataFactoryInterface::getMetadataForClass() returns.

interface SerializationVisitorInterface extends VisitorInterface
{
    public function startVisitingObject(ClassMetadata $metadata, object $data, array $type): void;

How to properly create those metadata and pass them then?

@goetas
Copy link
Collaborator

goetas commented Jul 4, 2020

hmmm... did not get it. :-(

can you explain your goal rather than the technical consequence of it?

@simPod
Copy link
Contributor Author

simPod commented Jul 4, 2020

I'm trying to create class metadata so I can pass them to the visitor.

For that I'm using metadata factory. But it does not return compatible type to be passed into SerializationVisitorInterface:: startVisitingObject() (even though it technically works, the typing is not correct).

I need to pass JMS\Serializer\Metadata\ClassMetadata there but from factory I'm getting ClassHierarchyMetadata|MergeableClassMetadata

Currently what I'm doing is that I have to assert the type to be safe, because I'm not guaranteed by the MetadataFactory I'll receive JMS\Serializer\Metadata\ClassMetadata instance

$classMetadata = $context->getMetadataFactory()->getMetadataForClass(My::class);
Assert::isInstanceOf($classMetadata, \JMS\Serializer\Metadata\ClassMetadata::class);

$visitor->startVisitingObject($classMetadata, $entity, ['name' => get_class($entity)], $context);

So I'm looking for a proper way to get something like

interface MetadataFactoryInterface
{
    public function getMetadataForClass(string $className) : ?\JMS\Serializer\Metadata\ClassMetadata;

That said to receive ClassMetadata instance that I can safely pass into a visitor. Passing ClassHierarchyMetadata|MergeableClassMetadata does not seem to be type safe.

@goetas
Copy link
Collaborator

goetas commented Jul 4, 2020

$classMetadata = $context->getMetadataFactory()->getMetadataForClass(My::class); should "almost always" return some metadata (despite its signature), so if you get NULL, something is wrong with your configs...
did you disable the annotation metadata reader?

@simPod
Copy link
Contributor Author

simPod commented Jul 4, 2020

The null is not issue. The issue is that it returns ClassHierarchyMetadata|MergeableClassMetadata metadata types while visitor requires \JMS\Serializer\Metadata\ClassMetadata which is not contra variant.

@goetas
Copy link
Collaborator

goetas commented Jul 4, 2020

did you customize MetadataFactory ? by providing your own implementation? (that class should choose the right metadata...)

@simPod
Copy link
Contributor Author

simPod commented Jul 4, 2020

Yes, it gives correct type actually. When executed, it returns \JMS\Serializer\Metadata\ClassMetadata. There's no issue with running the code. This is about API.

I'd like to rely on the interfaces and these two are incompatible:

MetadataFactoryInterface::getMetadataForClass(...) : ClassHierarchyMetadata|MergeableClassMetadata|null;
SerializationVisitorInterface:: startVisitingObject(\JMS\Serializer\Metadata\ClassMetadata, ...);

so it works but it's not correct in terms of interface type compatibility. I cannot pass return type ClassHierarchyMetadata|MergeableClassMetadata where \JMS\Serializer\Metadata\ClassMetadata is expected. It's not covariant.

@goetas
Copy link
Collaborator

goetas commented Jul 4, 2020

well, I'm aware that this library is not fully type safe. Adding phpstan/psalm is a lot of work (and making it type-safe would require BC breaks for sure).

Is one day someone will be interested to work on adding static analysis, that would be nice!

@simPod
Copy link
Contributor Author

simPod commented Jul 4, 2020

Ah ok, I was wondering whether I'm doing something wrong or not. Thanks!

@simPod simPod closed this as completed Jul 23, 2020
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

No branches or pull requests

2 participants