Add JSON:API integration test for LayoutBuilderEntityViewDisplay

Great start!

The equivalent test coverage for REST, \Drupal\Tests\layout_builder\Functional\Rest\LayoutBuilderEntityViewDisplayResourceTestBase, ensures there's actually something to test.

Let's bring over that same test coverage to JSON:API.

Expected

        'third_party_settings' => Array &10 (
            'layout_builder' => Array &11 (
                'allow_custom' => true
                'enabled' => true
            )
        )

Actual

        'third_party_settings' => Array &10 (
            'layout_builder' => Array &11 (
                'allow_custom' => true
                'enabled' => true
                'sections' => Array &12 (
                    0 => Array &13 ()
                )
            )
        )

How can this happen?

For REST, \Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer extends ConfigEntityNormalizer does this:

  protected static function getDataWithoutInternals(array $data) {
    $data = parent::getDataWithoutInternals($data);
    // Do not expose the actual layout sections in normalization.
    // @todo Determine what to expose here in
    //   https://www.drupal.org/node/2942975.
    unset($data['third_party_settings']['layout_builder']['sections']);
    return $data;
  }

… it overrides the normalizer for EntityViewDisplay config entities if they're instances of the Layout Builder-provided override, to not expose any layout information since that's considered an implementation detail that we're not yet ready to provide BC guarantees for. This was done in #3028301: Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs.

That custom normalizer does not apply to JSON:API, since JSON:API has a different representation for config entities, and hence a different normalizer.

Regarding \Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer extends ConfigEntityNormalizer I mentioned in #5: quoting myself from #3028301-31: Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs, where I reviewed that very code:

+++ b/core/modules/layout_builder/layout_builder.services.yml
@@ -47,3 +47,10 @@ services:
+  layout_builder.normalizer.layout_entity_display:
+    class: Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer

👍This is overriding serializer.normalizer.config_entity to ensure we don't lock ourselves into that data structure either. To be honest, I had not yet considered the config entity aspect yet — I was only thinking about content entities.

But given that the default layout for a given content entity type+bundle is stored in a config entity, and a content entity's "layout" field may contain an override of that default layout, and therefore both actually would expose the same data structure that is not something we want to ossify just yet, I think this makes perfect sense! 👏

Until we have at least per-top-level key for config entities (if that ever happens, if that even makes sense), this is the only possible solution. 👍

I don't see how we can port the same deep alteration from REST to JSON:API. REST is designed to be alterable at every level, JSON:API is not, both to ensure JSON:API spec compliance and to make it possible to automatically generate accurate schemas, using https://www.drupal.org/project/openapi.

(The alteration that LayoutEntityDisplayNormalizer makes is unknowable by https://www.drupal.org/project/openapi and will hence result in an inaccurate schema.)

Potential solutions

JSON:API makes it easy to omit a field, which in config entity land is the equivalent of a top-level key-value pair, but omitting third_party_settings completely is not what we want: we need to omit a nested value of third_party_settings (this is why I characterize this as a "deep alteration"). So this is not a viable solution

The only two possible solutions I can see are:

1. Start exposing third_party_settings.layout_builder.sections after all. But this requires #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API to be solved, which is no small task.
2. Add a temporary work-around to \Drupal\jsonapi\Normalizer\ResourceObjectNormalizer::serializeField(). I think this is reasonable given that JSON:API landed in core only today, which resulted in JSON:API test coverage for every entity type class being required as of today, which the Layout Builder people could not have known. Although I wish I had predicted this when I worked on #3028301-31: Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs — sorry 😔

P.S.: a third possible solution in the future is to bring the internal flag from Typed Data definitions over to config schema as well, then we could just mark third_party_settings.layout_builder.sections as internal and JSON:API would respect that.

Tagging as it blocks #3041053: Mark Layout Builder as a stable module

I have no experience with JSONAPI before today but the explanation in #7 is very clear, the follow-ups are good (I like the third one in the PS), and the patch is clear

Wait! Not RTBC.

The test must be
\Drupal\Tests\layout_builder\Functional\Jsonapi\LayoutBuilderEntityViewDisplayTest
It is currently
\Drupal\Tests\layout_builder\Functional\JsonApi\EntityViewDisplayTest

Note the namespace case-sensitivity as well as the LayoutBuilder prefix.
This also means the use alias can be removed

This can be observed by changing layout_builder.info.yml as follows

-package: Core (Experimental)
+package: Core

And running \Drupal\Tests\jsonapi\Functional\TestCoverageTest::testEntityTypeRestTestCoverage()

#8++ — I was gonna look into that next, since I didn't understand why #2 even passed tests!

And #9 is the reason why — thanks for that pointer!

Okay, great! Confirmed that testEntityTypeRestTestCoverage is satisfied. Thanks!

+++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
@@ -95,6 +95,11 @@ protected function serializeField($field, array $context, $format) {
+      if ($context['resource_object']->getResourceType()->getDeserializationTargetClass() === 'Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay' && $context['resource_object']->getField('third_party_settings') === $field) {

any reason not to use a class constant here?

I did that only to minimize lines of code in JSON:API that depend on Layout Builder: to minimize dependency and future changes when we remove it.

Also makes it easier to port this to the JSON:API contrib module which we'll have to maintain for Drupal 8.6.

I'm fine with committing this as a workaround to not prematurely expose section data and to get Layout Builder's tests passing. However, I'm concerned by the fact that we needed this workaround in the first place. It's a workaround in two ways: It works around us not having decided how to expose this data over HTTP APIs yet, but it also works around a limitation whereby we can't control the visibility of this info over JSON:API anyway without hacking JSON:API itself.

So, I think we need some followups for this specific case (the bit about third-party settings, or config schema's version of internal, or whatever) to the followup plan on #3040025: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs. This might be #3040354: Introduce entity type annotation key for indicating that an entity type is *safe* for HTTP API usage, or it might be a new issue specifically about third-party settings or something, but in either case we should document this particular issue in the followups.

We should probably also add whichever issue to the @todos in the codebase for the JSON:API workaround, since I think we want to replace that workaround with a proper API even before we expose section data to HTTP APIs.

Leaving RTBC, but can we get this captured in followups and then added to the @todo. Thanks!

but it also works around a limitation whereby we can't control the visibility of this info over JSON:API anyway without hacking JSON:API itself.

A quick recap because I don't like how this can be read as putting blame on JSON:API — the problem here is quite simple:

1. When Drupal 8.0.0 decided to expose config entities via REST, it assumed we'd always want to expose the entire config entity.
2. For Layout Builder, that assumption is wrong. That's why Layout Builder "hacks" REST using \Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer since #3028301: Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs. And that's why here, we need to "hack" JSON:API itself, because "hacks" such as those for REST are not sustainable (as explained in #6: spec compliance & schema support).
3. The root cause therefore is that the config (entity) system isn't designed to be API-First.
4. But we didn't want to block REST on it (and JSON:API merely followed REST's lead), and we don't want to block Layout Builder on it either.
5. So here we are: rest.module was well-intentioned, as was the configuration entity system, as is JSON:API, as is Layout Builder. The configuration entity system and the rest.module happened largely in parallel, and hence their assumptions were never integrated; they existed in parallel. So we still need to do the integration work.

I think that per #7, @tim.plunkett agrees with this general high-level assessment.

it might be a new issue specifically about third-party settings or something, but in either case we should document this particular issue in the followups.

Agreed! Created #3043245: Allow marking a config schema types to be marked as "internal" (to avoid exposing them via HTTP APIs). Added @todo to the patch, pointing to that issue.

Saving issue credit and fixing which files are listed in the IS.

+++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
@@ -95,6 +95,12 @@ protected function serializeField($field, array $context, $format) {
+      // @todo Replace this work-around once https://www.drupal.org/project/drupal/issues/3043245 lands.
+      // @todo Better yet, remove the need for this work-around in https://www.drupal.org/node/2942975.

Oops, these are over 80 characters.

Reformatted that comment, and included a blank line to make it clear where the @todo ended

I understood the idea of the two separate @todos, but the second would read strangely without the first anyway, so I made it a single block. Also, @see doesn't work inline like that.

Committed and pushed to 8.8.x and cherry-picked to 8.7.x. Thanks!

Per @tim.plunkett there's a filename/namespace case sensitivity issue, so reverted for that.

\Drupal\Tests\jsonapi\Functional\TestCoverageTest::testEntityTypeRestTestCoverage() explicitly looks for
Jsonapi
in the namespace. Yet the patch contained
JsonApi
which fails on a case-sensitive file system.

Pushed #25 to 8.8.x and 8.7.x.