Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
JSON:API has a full suite of tests for every core entity type. Layout Builder takes over the entity_view_display entity type, though, so it would be useful to also test its integration with JSON:API when Layout Builder is enabled.
Proposed resolution
Add a test.
Remaining tasks
Review and commit a patch.
Comment | File | Size | Author |
---|---|---|---|
#25 | 3042198-json-24.patch | 2.98 KB | tim.plunkett |
#25 | interdiff.txt | 1.31 KB | tim.plunkett |
#18 | 3042198-json-18-interdiff.txt | 1.51 KB | tim.plunkett |
#18 | 3042198-json-18.patch | 2.98 KB | tim.plunkett |
#10 | interdiff.txt | 1.19 KB | Wim Leers |
Comments
Comment #2
phenaproximaLet's see if this flies!
Comment #3
Wim LeersGreat 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.
Comment #5
Wim Leers\Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer extends ConfigEntityNormalizer
does this:… 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.
Comment #6
Wim LeersRegarding
\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: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 ofthird_party_settings
(this is why I characterize this as a "deep alteration"). So this is not a viable solutionThe only two possible solutions I can see are:
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.\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 markthird_party_settings.layout_builder.sections
asinternal
and JSON:API would respect that.Comment #7
tim.plunkettTagging 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
Comment #8
tim.plunkettWait! 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
Comment #9
tim.plunkettThis can be observed by changing
layout_builder.info.yml
as followsAnd running
\Drupal\Tests\jsonapi\Functional\TestCoverageTest::testEntityTypeRestTestCoverage()
Comment #10
Wim Leers#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!
Comment #11
tim.plunkettOkay, great! Confirmed that testEntityTypeRestTestCoverage is satisfied. Thanks!
Comment #12
larowlanany reason not to use a class constant here?
Comment #13
Wim LeersI 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.
Comment #14
xjmI'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!
Comment #15
Wim LeersA quick recap because I don't like how this can be read as putting blame on JSON:API — the problem here is quite simple:
\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).rest.module
was well-intentioned, as was the configuration entity system, as is JSON:API, as is Layout Builder. The configuration entity system and therest.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.
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.Comment #16
xjmSaving issue credit and fixing which files are listed in the IS.
Comment #17
xjmOops, these are over 80 characters.
Comment #18
tim.plunkettReformatted 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.
Comment #21
xjmCommitted and pushed to 8.8.x and cherry-picked to 8.7.x. Thanks!
Comment #24
xjmPer @tim.plunkett there's a filename/namespace case sensitivity issue, so reverted for that.
Comment #25
tim.plunkett\Drupal\Tests\jsonapi\Functional\TestCoverageTest::testEntityTypeRestTestCoverage()
explicitly looks forJsonapi
in the namespace. Yet the patch contained
JsonApi
which fails on a case-sensitive file system.
Comment #28
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed #25 to 8.8.x and 8.7.x.