Hey!

I think @mtarld has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@mtarld The original issue is marked as Bug, should I target 4.x release or 6.1? I'd say it's rather a feature.

Also, I don't see any documentation for normalizers except for https://symfony.com/doc/5.2/serializer/normalizers.html#built-in-normalizers, I guess it's self-documented enough.

And lastly, I'm still wondering if the backward compatible behavior is needed in this edge case. Or at least what about adding a deprecation notice in case the original check does not pass so that we can delete it in a subsequent version? I mean I don't mind and most people configuring their normalizers can probably find this configuration option but it would prevent the unexpected first encounter of this feature 😄

Yes indeed, IMO it must be a feature 🙂

I guess the documentation, code, and context builders are enough.

For the BC migration path part, it is tricky. The behavior of these normalizers is changed (and the same goes for the serializer itself), so we must trigger a deprecation IMHO.

Maybe we could leverage the func_num_args function as it does not count the optional args that aren't explicitly given. This will hopefully work as the allowNormalizationOfObjectsWithoutXXX is the last property.
Something like if (func_num_args() < 6) then trigger a deprecation that tells the user to explicitly give the allowNormalizationOfObjectsWithoutXXX as it'll default to true in the 7.0 release.

WDYT?

Sounds good, I've added the deprecation, updated the default symfony bundle configuration (src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php) and updated tests so that the deprecation is not triggered :)

Do we want to target 6.1 or 6.2 release?

As 6.1 is in feature freeze, we must target 6.2

Failing checks seem to be unrelated

Anything I can do to get a second review and merge it?