Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.AFAICT, Drupal\jsonapi\Normalizer\NormalizerBase doesn't need to override supportsNormalization() and supportsDenormalization(). Couldn't it achieve the same behavior by removing those overrides and only overriding checkFormat()?
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3029891-7.patch | 1.94 KB | wim leers |
| #7 | interdiff.txt | 865 bytes | wim leers |
| #4 | 3029891-4.patch | 1.54 KB | wim leers |
Comments
Comment #2
effulgentsia commentedFor that matter, does it even need to override checkFormat(), or could it just change its
protected $formatsdeclaration toprotected $format?Comment #3
wim leersAnother nice find!
JSON:API started duplicating this in #2821556: [CLEANUP] Remove dependency on HAL. But that's only because it removed its dependency on the HAL module, which included overrides for those methods in
\Drupal\hal\Normalizer\NormalizerBase. To simplify refactoring, @e0ipso chose to keep that logic.But then about six months later, core cleaned up
\Drupal\hal\Normalizer\NormalizerBasein #2863778: Clean up \Drupal\hal\Normalizer\NormalizerBase: duplicate less from the parent class \Drupal\serialization\Normalizer\NormalizerBase. That's also whereprotected $formatwas introduced in\Drupal\serialization\Normalizer\NormalizerBase.So this was simply a matter of JSON:API taking the prudent road. It's cool to see that #2863778 indeed did not break BC for JSON:API, thanks to JSON:API owning the bits it cared about :)
Conclusion: +1 to everything you said, but it's only thanks to core getting better that we can do this!
Comment #4
wim leersComment #5
wim leers@e0ipso: I ran all JSON:API Extras tests locally, they still pass 👍
Comment #7
wim leersYou may be surprised to read this, but I love that failure! 🥰
Why?
Because it proves that the change above suddenly allowed JSON:API's forward port of #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API to be used for formats other than JSON:API, which caused these test failures in the one place where the JSON:API is using core's
serializerwith::normalize($format=NULL).So: we do need
::checkFormat(), but for a very good reason (to be and continue to be self-contained), which is now explicitly documented :)Comment #8
gabesulliceNice! No more reflection, less "vendored" code to babysit. LGTM.
Comment #10
wim leers🚢
Comment #11
effulgentsia commentedFollow-up: #3030148: Clean-up: remove dead $formats property from normalizers