Don't return a metadata object unless there is explicit annotation or attribute configuration

[mbabker]

Q	A
Bug fix?	yes
New feature?	no
Doc updated	no
BC breaks?	maybe?
Deprecations?	no
Tests pass?	yes
Fixed tickets	Partially schmittjoh/JMSSerializerBundle#931
License	MIT

Before #1469 the annotation driver would always return a JMS\Serializer\Metadata\ClassMetadata regardless of whether the object had any configured annotations or attributes. With the changes in #1469, the new attribute driver inherited the same behavior. And with schmittjoh/JMSSerializerBundle#920 prioritizing the attribute driver over the annotation driver, the end result is now that the attribute driver will always return a metadata object even if there were no attributes on the object and the chain will never fall through to a later driver (in this case, the annotation driver).

Untested, but this should change the behavior for these two drivers so that it returns null if there are no explicitly configured annotations or attributes on an object instead of a metadata object without any information. The toggle on whether to return the metadata object or null is based solely on the presence of annotations or attributes; once the code gets into the loops processing those, we'll assume there is a configuration that should be used (as those loops won't be entered for empty lists).

I mark this as "maybe" a B/C break because it's really a weird edge case. Metadata\Driver\DriverInterface::loadMetadataForClass() supports null returns, which are accounted for already in the metadata package, but there is a change in runtime behavior in that a metadata instance will no longer be provided on objects that never had configuration in the first place. If someone's relying on this behavior, they might be in for an unwelcome surprise on upgrade.

[mbabker]

Oh the test failures here are fun 😅

The decorating drivers all have assert() calls looking for non-null returns from their inner drivers. The only one that actually checks for null is the DefaultValuePropertyDriver but that check is made after the assert() call (which will fail in an environment where assertions are enabled).

So not only do we have the annotation/attribute drivers always providing a metadata object, we also have decorating drivers which expect non-null returns from the drivers they decorate.

[sylviavdv]

would it be an idea to add a isConfigured() function to the ClassMetadata and in the DriverChain change
if (null !== $metadata = $driver->loadMetadataForClass($class)) {
to something like

$metadata = $driver->loadMetadataForClass($class)
if (nul !== $metadata && $metadate->isConfigured()) 

[mbabker]

It gets a little more complicated than that, unfortunately.

Discriminators won't be built correctly without a metadata object (in any form) being returned. I found this one out the hard way through the tests, the JMS\Serializer\Tests\Fixtures\Discriminator\Car fixture has no metadata attached to the class, but its parent JMS\Serializer\Tests\Fixtures\Discriminator\Vehicle does. With the straight null returns, Metadata\MetadataFactory doesn't merge things through correctly.

Because the base Metadata\ClassMetadata and child JMS\Serializer\Metadata\ClassMetadata use public properties, tracking if the metadata instance is configured gets trickier (either the method has to know the default state and make an assumption based on that, requiring all child classes to expand on that, or there'd have to be a heavy refactor to use setter methods and close the properties off from direct access). And, an isConfigured() check really only makes sense in the context of the factory to know how to deal with the returned result from the drivers.

And, that still doesn't fix the assumptions from the drivers that decorate the annotation/attribute, XML, and YAML chain (i.e. the Doctrine type drivers, the enum driver, or the default value property driver). All of those pretty much hard fail without that chain returning a metadata object (if zend.assertions is active, they all fail on assert() calls, in some cases before reaching the null checks, with the typed properties driver not even checking for null). So that's where we hit the undocumented assumption that one of the metadata based drivers (annotation/attribute, XML, or YAML) must return a metadata object, even if it's not configured.

Even if we sort out the behavior with loading metadata, we still have runtime issues in the graph visitors. Metadata\MetadataFactoryInterface::getMetadataForClass() documents a possible null return, yet those visitors have assert() calls looking for non-null returns and even without assertions there will be hard fails because of property access on a null value.

Plus, there are still other little oddities floating around. JMS\Serializer\Handler\StdClassHandler has a getMetadataForClass('stdClass') call. Nothing is configuring metadata for this core PHP object, yet because of the current runtime behavior if your driver chain has the annotation or attribute drivers loaded into it (which unless you're taking extra steps as the Symfony bundle requires the annotation reader and when using this package standalone all the defaults require the annotation driver and doctrine/annotations), that will provide a metadata object.

I didn't think this little PR would end up with me digging deep into a rabbit hole, but at least I have a cozy little corner dug out now 🤣

[goetas]

@mbabker to me this looks good. should now the "doctrine/annotations": "^1.13 || ^2.0", from the composer.json be moved tho the suggests section ?

[goetas]

can you also put a warning on the documentation saying that doctrine/annotations is needed to use annotations, and that attributes are the default?

[scyzoryck]

should now the "doctrine/annotations": "^1.13 || ^2.0", from the composer.json be moved tho the suggests section ?

It sounds like a BC change for some users 🤔

[mbabker]

should now the "doctrine/annotations": "^1.13 || ^2.0", from the composer.json be moved tho the suggests section ?

We're not quite there yet. This and #1471 in theory would make it possible to not hard require annotations, but as long as the builder API hard requires an annotation reader then that's a premature move.

It sounds like a BC change for some users 🤔

IMO it's easily called out in the upgrade notes. And for me personally, this is one of those cases where if you're relying on transient dependencies to install the packages your application has a hard requirement on, you're better off taking the 2 minutes to run the appropriate composer install command to get everything situated.

[mbabker]

BTW, if anyone's got any ideas on the failing CS checks, I'm all eyes. I've tried use statements (which failed because the sniffer decided they were unused) and just dumping FQCN's into the attributes (causing the current failures) and I haven't found something that satisfies PHP_CodeSniffer.

[scyzoryck]

It looks like the issue is with super outdated Code Sniffer. I've added MR to update it to 12.x - that understands annotations :)
#1503

[mbabker]

@goetas Dragging #1497 (review) over here...

is this a bc break? (new required param)

Yes, but this should be very low impact (the null driver isn't actually used anywhere in the package or Symfony bundle at the moment, so this would only affect folks who are using their own driver factories to build serializers and not in the context of a Symfony app). The reason it's needed is because anything that would fall back to the null driver would produce object keys that do not respect the configured naming strategy, so the keys would always be as declared in the data source. This is also a by-product of making the null driver include all the class properties instead of only creating a metadata object with a file resource reference; this matches up with the behavior that exists now when "empty" metadata objects are created by the annotation/attribute drivers for classes without that metadata.

you are using this because now the AnnotationOrAttributeDriver can return null (becase of the $configured thing...)

is it worth? what is AnnotationOrAttributeDriver returns the empty metadata as before?

So this is the hidden gotcha I got into in #1494 (comment) where basically the annotation/attribute driver will always create and return an empty metadata object even if the class it's processing doesn't have any annotations/attributes to create any metadata from, whereas the XML and YAML drivers will only create metadata objects if there is a corresponding metadata configuration. So this change makes the behavior for all of the metadata-based drivers consistent in that they only provide a metadata instance if there is metadata to generate it from, and the fallback behavior is moved out of the annotation/attribute drivers and into the null driver.

[goetas]

Only after mergeing this I realized a thing... In the bundle, that is not using the null driver, is this now going to stop serializing objects that have no annotations defined?

[mbabker]

Yes, the bundle will need an update. Shipping a combination of this and #1471 warrants a major version bump since there are a few B/C breaks in the internal APIs. I'll have a PR put together for the bundle in the next few days to make all the needed changes.

[alexander-schranz]

If I understand the comments here correctly, currently it is normal that we run under @dev versions of JMS serializer and Bundle into this issues: https://github.com/sulu/sulu/actions/runs/5739637891/job/15555690148?pr=7128 ?

[mbabker]

If Sulu's just using the JMSSerializerBundle, then the Sulu AdminBundle fails/errors look like expected side effects of this PR without the bundle having been updated.

[mbabker]

@alexander-schranz schmittjoh/JMSSerializerBundle#933 would be the bundle changes if you're able to test that with Sulu. Everything's compiling and serializing right in the test app I'm running, but it's not really a great test environment for more than making sure the container's dumped with the right services.

[goetas]

#1506 is here to minimize this bc break