1. Why is this doing something very special with \Drupal\jsonapi\Normalizer\Value\ValueExtractorInterface? (Which has zero documentation, and no associated d.o issue, so the discussion leading to this design decision is impossible to find.) It's also not tested.
  2. Why is this doing something special with $context['data_wrapper']? It is not documented nor tested.
  3. Why does this not implement \Symfony\Component\Serializer\Encoder\EncoderInterface + \Symfony\Component\Serializer\Encoder\DecoderInterface?
  4. Why does this not extend \Drupal\serialization\Encoder\JsonEncoder?
CommentFileSizeAuthor
#2 2829407--encoder-clarifications--2.patch1001 bytese0ipso
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
FileSize
2829407--encoder-clarifications--2.patch1001 bytes
  1. I added a comment about it. This is actually tested in: Drupal\Tests\jsonapi\Kernel\Normalizer\DocumentRootNormalizerTest::testNormalizeException
  2. I added a comment about it. This is actually tested in by all the error format tests: Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testWrite and Drupal\Tests\jsonapi\Kernel\Normalizer\DocumentRootNormalizerTest::testNormalizeException
  3. It implements them via inheritance of Symfony\Component\Serializer\Encoder\JsonEncoder. Do you want me to surface that as well in the current class?
  4. The serialization module encoder is only in charge of declaring the supported base formats. JSON API is totally foreign to json and ajax. Therefore, imo, it shouldn't extend that class only to override all of its responsibilities.

@Wim Leers let me know if there are any action items for this one besides the current code doc patch.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Active » Needs review
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

  • e0ipso committed ec51401 on 8.x-1.x 
    docs(Serialization) Clean up \Drupal\jsonapi\Encoder\JsonEncoder (#...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Reviewed & tested by the community » Fixed

Ugh! I forgot to merge this one. Sorry for the delay.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

This did not address point 4 in the IS. Created #2840967: \Drupal\jsonapi\Encoder\JsonEncoder should extend the serialization module's JsonEncoder, not Symfony's for that.