Serializer docs refer to nonexistent method

[mattattui]

I've been following along with the Serializing interfaces and abstract classes section of the Serializer docs, and got to this example (line 1234 in the current master source):

$discriminator = new ClassDiscriminatorFromClassMetadata($classMetadataFactory);
$discriminator->addClassMapping(CodeRepository::class, new ClassDiscriminatorMapping('type', [
    'github' => GitHubCodeRepository::class,
    'bitbucket' => BitBucketCodeRepository::class,
]));

Unfortunately ClassDiscriminatorFromClassMetadata doesn't appear to have an addClassMapping() method in either version 4.1, or master. In fact I can't find a method called addClassMapping anywhere in the Serializer component, which is a shame because it looks like a really useful feature :)

If someone could tell me how to add a mapping to the class discriminator in the symfony framework, I'm happy to work up a PR to correct the code example (fyi: I don't think I can use the much easier-looking annotation-based method in the example below it because I think I need to dynamically add the class mapping during a compiler pass based on a configuration. The use-case is that I'm developing a bundle that provides an abstract entity that users can extend, and I need to be able to deserialize API data into objects of those user-supplied classes).

[javiereguiluz]

@inanimatt thanks for reporting this issue! The related blog post is also wrong then: https://symfony.com/blog/new-in-symfony-4-1-serialize-and-deserialize-from-abstract-classes

Let me ask our friend Samuel (@sroze) who originally created this feature to see if he can help us here. Thanks!

[sroze]

You are completely right, it can't work and re-reading it, the example makes very little sense. The ClassDiscriminatorFromClassMetadata is responsible for getting the mapping from the "class metadata": from the YAML/XML/Annotation configuration on the given class, that's it. For 4.1 and bellow, I suggest you then simply add the metadata on the class, and it should work.

I do agree that we need the ability to configure such mapping as a "component-only" without class metadata, but it does not exist yet and can only be added in 4.2 minimum :)

[sroze]

See the following pull-request to enable the "without class metadata factory" configuration: symfony/symfony#28351.

@inanimatt would you be willing to submit a documentation pull-request to reflect this? I believe the best is to simply split the paragraph into "With class metadata" and "Manual discriminator mapping configuration".

[mattattui]

Wow, you're quick! ❤️
Just to be clear, are you asking me to make a pull request to document the functionality in the new PR, or to update the current documentation to … essentially remove the section describing functionality that doesn't currently exist? (I can do both - though I guess it makes sense for them to be separate PRs)

[sroze]

document the functionality in the new PR, or to update the current documentation

I'm pretty sure both can be done at the same time :)

essentially remove the section describing functionality that doesn't currently exist?

As I said, you need to keep the part with the class metadata factory, because it's still very valuable. But you need to remove the addClassMapping in this example and add another for the pull-request with the "manual mapping" in symfony/symfony#28351.

Thank you for taking care of this then ❤️

[mattattui]

No problem! One last question before I get to work (sorry, I've not done this often and I don't want to mess it up or cause more work for someone): should I submit a PR against the 4.1 docs, or master? I suppose what I'm really asking is whether your new ClassDiscriminatorConfiguration is going to make it into a 4.1 release.

If I add a PR against master, then the 4.1 docs will remain wrong, but if I do a PR against 4.1 docs and ClassDiscriminatorConfiguration won't exist until 4.2, then I'll just be replacing one mistake with another, if you see what I mean :) So if we know for sure your PR won't be merged into 4.1, it makes sense for me to do a PR to correct the 4.1 docs (just remove the addClassMapping bit), and then a separate PR against master once that's been merged to document the new class, right?

Thanks again for your time.

[javiereguiluz]

Yes, sadly this can't be done in a single PR. We have two different things:

• Fix a bug in 4.1 docs
• Add docs for the new feature in "master" (if it gets merged)