Mark the JSON API serializer, normalizer and encoder services as private

#2920536: Force all serializer encoders + normalizers services to be private landed in core. Would be great if we would mirror that behavior in JSON API.

This ports the relevant changes from #2920536: Force all serializer encoders + normalizers services to be private. Even just this change is causing lots of fails because for example JsonApiDocumentTopLevelNormalizerTest is retrieving the normalizer from the container.

This does NOT yet mark the jsonapi.serializer_do_not_use_removal_imminent service itself as private.

Still postponed on #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API. Just wanted to post an initial patch.

@gabesullice pointed out that the problem here is that the RequestHandler needs to get the serializer service. But that can be solved by injecting it. Attached interdiff fixes that, and makes all services be injected into the request handler.

This should pass more tests.

#8 :)
#8.4: that's dead code… I should just remove it.

Fix RequestHandlerTest's mocks.

Fix remaining failures in JsonApiDocumentTopLevelNormalizerTest.

#12 was incomplete.

The remaining failures were hard to debug. They happen because \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::__construct() gets \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer injected. But because it's injected, setSerializer() has not yet been called on it, which \Drupal\jsonapi\Serializer\Serializer::__construct() does.

But we haven't changed any logic, so why is this happening?

Well, it's been happening since #4. #4 is marking all normalizer services private. Marking a service private also makes it non-shared. And it's impossible to mark it shared explicitly when it's marked private. So simply doing setShared(TRUE) or shared: true doesn't fix the problem.

The real problem here is that RelationshipItemNormalizer is doing something it shouldn't. And we even added

 @todo Remove the dependency on \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer

for that in issue comment #2831185-5: Rename DocumentWrapper to JsonApiDocumentTopLevel and DocumentRootNormalizer to JsonApiDocumentTopLevelNormalizer (commit b064a203) in Dec 2016. Fixing that is out of scope here, so just making sure that the setSerializer() calls cascades.

WHEW! That was a hard one to debug!

GREEN! Green for the first time!

So now let's do the final step: mark the serializer service as private too.

YAY! ALSO GREEN!

Let's fix CS violations.

Self-review:

1. +++ b/src/Controller/RequestHandler.php
   @@ -2,36 +2,95 @@
   +  public function __construct($serializer, CurrentContext $current_context, RendererInterface $renderer, ResourceTypeRepositoryInterface $resource_type_repository, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, FieldTypePluginManagerInterface $field_type_manager, LinkManager $link_manager) {
   

   Missing docs.

2. +++ b/tests/modules/jsonapi_test_normalizers_kernel/jsonapi_test_normalizers_kernel.info.yml
   @@ -0,0 +1,5 @@
   +name: 'JSON API test normalizers in Kernel tests'
   

   Needs better description.

Fixed both.

Self-review:

I love your self-reviews, they are so helpful when reviewing!

I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.
I will not copy/paste <em>any</em> comment without re-assessing it.

Fixed :)

Also:

I love your self-reviews, they are so helpful when reviewing!

❤️

This was marked postponed/[PP-1] in #6 by me:

Still postponed on #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API. Just wanted to post an initial patch.

… but in fact I see no reason to block this on that. In fact, this issue makes #2883086 simpler! This patch makes zero functional changes to RequestHandler. The only thing this changes, is how it gets the services it needs injected. #2883086 will change much more, and this makes that slightly easier.

Removing the [PP-1].

🤐

This now also blocks #2941685 and #2883086. See #2883086-33: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API.

1. +++ b/tests/modules/jsonapi_test_normalizers_kernel/jsonapi_test_normalizers_kernel.services.yml
   @@ -0,0 +1,4 @@
   +services:
   +  jsonapi_test_normalizers_kernel.jsonapi_document_toplevel:
   +    alias: serializer.normalizer.jsonapi_document_toplevel.jsonapi
   +    public: true
   

   Nifty!

2. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
   @@ -358,7 +358,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
   -      ->get('serializer.normalizer.jsonapi_document_toplevel.jsonapi')
   +      ->get('jsonapi.serializer_do_not_use_removal_imminent')
   

   I'm not a fan of using the serializer (that calls the normalizers) to kernel-test the normalizers. However I see the convenience of it and I support that.

💥

I'm not a fan of using the serializer (that calls the normalizers) to kernel-test the normalizers. However I see the convenience of it and I support that.

This was accidental! Attached is a patch that fixes both places where that happened, and actually updates even one more pre-existing one!

This also means #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets is now less blocked, as is #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API! Hurray!

And it also means there's one less thing to do for #2931785: The path for JSON API to core :)

Committed! Thanks again. You rock, I will not get tired of telling you both.

❤️