On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal()

#6 is going to fail against 8.3, and should pass against 8.5. The new core API that this patch uses is only available in 8.5.

See #2825812-107: ImageItem should have an "derivatives" computed property, to expose all image style URLs: thanks to #6, image fields normalized by JSON API automatically get an image_styles property in the meta section for image field normalizations:

#6 only updated the entity reference field case, but we also need to update the generic field case.

Still needs explicit tests.

See #2577923-119: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property, thanks to #10, link fields normalized by JSON API automatically get a target_uuid property for link fields that point to an entity:

Do you have a plan so we can maintain compatibility for 8.3 through 8.4?

Not yet, but of course I will try to achieve that. The above patches are just the bare minimum to make things work.

I'm hesitant to do this before we have full test coverage of JSON API, i.e. the equivalent of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method for JSON API, because without that test coverage, how would we possibly know for certain that we wouldn't be breaking BC?

I'm not sure I like this solution. This is verbose explanation why https://www.lullabot.com/articles/decoupled-drupal-hard-problems-image-s...

If each consumer has 10 - 15 image styles, that means 30 - 45 image styles URLs, where only one will be used. […] This situation is not ideal because a malicious user can still generate 45 images in our disk for each image available in our content.

It's up to the administrator of the API server/host to set up image styles that can be used for multiple clients. 10 image styles per consumer seems a lot, and zero reuse across consumers also seems pretty extreme.

That doesn't mean I don't agree that often it should be the client that does the resizing… that is true. But if that's the case, then this site could just mark the image_styles computed property as internal, and it'd disappear from JSON API responses (as well as REST responses).

This is not meant to be an optimal solution. Different API servers/hosts will have different use cases: there's no perfect answer. This just happens to be the default answer Drupal would give. Upon installing https://www.drupal.org/project/consumer_image_styles, that default behavior would be overridden.

Doesn't that seem acceptable?

In my experience I think it's a rather optimistic number, but I guess that depends on each project. Drupal aims to support all kinds of projects, so we need to plan for the worst case scenario.

Agreed! 👌

We must allow for any scenario, but provide sensible defaults. And requiring a contrib module to be able use image styles, is not a sensible default. I think you agree with that? 🙂

consumer_image_styles for "BC" reasons

BC would be up to the https://www.drupal.org/project/consumer_image_styles module to provide, not the JSON API module.

If you feel strongly that the key should not be image_styles, but derivatives, please post a comment at #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs expressing why that is. Interestingly, the image module calls it Image Styles in the UI, but derivatives in most of the code. I can totally see "image styles" being too Drupal-y, and "derivatives" being a better name, because it's understandable by a broader audience.
Posted this understanding as a comment at #2825812-113: ImageItem should have an "derivatives" computed property, to expose all image style URLs. Please reply to it, to clarify your thoughts. In fact, I even implemented it in #2825812-115: ImageItem should have an "derivatives" computed property, to expose all image style URLs.
Hope you like it! 😄

On the topic of BC with the the existing consumer_image_styles module: I do think that the data provided by https://www.drupal.org/project/consumer_image_styles is insufficient: without the width and height, browsers cannot efficiently render <img> tags. Not every image style has fixed dimensions, some just resize to e.g. 50%. And even if there are fixed dimensions, it should not be necessary to hardcode those in every consumer, because that means 3 pieces of coupled knowledge: A) image style name, B) image style width, C) image style height. It's better to have only 1 (image style name).

I think I missed the use case for the image dimensions.

Not specifying width & height causes reflows: browsers render DOM elements while images are being downloaded. If the image dimensions aren't known, they need to be read from the downloaded image. And if the image is not yet downloaded, the browser of course can't know it. This then causes a layout reflow.

Maintaining an ecosystem of modules around this topic on my free time is incredibly hard and time consuming.

Absolutely! And it's made harder by the fact that we rushed to ship things that weren't done, which then cause BC breaks for that ecosystem :( I'm trying to fix the entire ecosystem as you know, and that's even harder — and I'm only able to do it because it's my primary task in my job for the past ~1.5 year. So … *hugs* 🙂

I have the impression that this will not land until at least some computed property in core is doing setInternal(FALSE).

#13 through #18 got sidetracked with the hypothetical example I showed in #12.

As soon as any of the following 3 issues land, we'll have a concrete thing to look at:

1. #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata
2. #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization)
3. #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs

And it's probably better to do this after #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only anyway.

Two issues mentioned in #19 weren't yet in the list of related issues.

#2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field landed, which is the second item in the list in #19. This is now unblocked! Except we still want to wait for #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only first.

#23: thanks for updating this, but it's still postponed. It's still blocked by #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, which I'm actively working on right now :)

#19 explained that this needed >=1 core issue that added a non-internal computed field property to core. #22 already indicated that #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field was that first example. And thanks to #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, there's now two of those.

As of #2775205-46: [PP-2] [FEATURE] Discus how to make JSON API use text input formats for long text fields, this is a hard blocker for #2775205.

Without this issue, JSON API will not gain the two serialization gap fixes that currently exist in core. The sooner this lands, the sooner we can further validate Drupal 8.5 core's architecture/strategy/solution, and if necessary, change course. So, bumping this to critical.

On top of that, this is also listed as a must-have in #2931785: The path for JSON API to core, further indicating the criticalness of this issue.

Note that ideally this issue would land first, and then we would be able to solve #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it.

Also, hurray, everything is coming together, all tricky things are being addressed! 🍾

To clarify: this is not blocked on #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it (that is actually blocked on this). This is blocked on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only (see #22).

And #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only is nearing completion!

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases landed!

Rebased #10 — now that #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases have landed, we already have all the integration test coverage we need! It'll become clear which normalizations would change for all entity types across Drupal core.

5 fails on 8.6, 30 fails on 8.4. Because on 8.4 the API that #31 uses doesn't exist yet.

This should fix all fails.

#33 passed on 8.4.x but failed on 8.5.x/8.6.x. That already failed before, I just forgot to fix it.

1. :)
2. :D

Assigning to @e0ipso for final review.

#2937850: Mark revision_default as internal for REST consumers just landed, which means we'll need to update our expectations (#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only are expecting revision_default). But before we can update our expectations, this needs to land, since #2937850 didn't remove those fields, but merely marked them "internal", and JSON API will only start respecting that as of this patch.

So this patch will need a reroll to update those expectations. Currently retesting #34, that should fail.

they didn't break as expected

… because this patch did not update JSON API's ContentEntityNormalizer yet, oops.

This does that, and should hence fail tests.

::isInternal() only exists on >= 8.5.

#43 should still fail on 8.5/8.6, this should make it pass again.

In MediaTest:

                // @todo Uncomment in https://www.drupal.org/project/jsonapi/issues/2921257.
                // @codingStandardsIgnoreStart
                /*
                'url' => $file->url(),
                */
                // @codingStandardsIgnoreEnd
…
                // @todo Uncomment in https://www.drupal.org/project/jsonapi/issues/2921257.
                // @codingStandardsIgnoreStart
                /*
                'url' => $thumbnail->url(),
                */
                // @codingStandardsIgnoreEnd

This is wrong. This is blocked on #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization) — it's not related to this issue!

So I should remove those comments as well.

Did #46.

So you're concerned about $entity->getFields() vs $entity->getTypedData(). Excellent question!

First: \Drupal\serialization\Normalizer\ContentEntityNormalizer::normalize() also uses getTypedData().

Second: \Drupal\Core\TypedData\TypedDataInternalPropertiesHelper::getNonInternalProperties()'s sole parameter must be of the type ComplexDataInterface, i.e. it must be typed data. Hence we call getTypedData(). getFields() is intended for code that does not care about traversing typed data, i.e. typical custom Drupal code. In theory we could make it work with getFields(), but then JSON API would be implementing its own logic rather than using \Drupal\Core\TypedData\TypedDataInternalPropertiesHelper() which core specifically provides for this purpose.

Third: the "else" will be removed once JSON API requires Drupal >=8.5, so the difference will disappear.

Fourth: they will behave differently in that the $entity->getFields() call (on <8.5) omits computed fields, whereas the TypedDataInternalPropertiesHelper::getNonInternalProperties($entity->getTypedData()) call omits internal fields. Computed fields are still internal by default though. That's off course the goal.

Fifth: the test coverage already proves that they're equivalent! The normalized output is identical — the only difference is that computed fields (internal by default!) that >=8.5 explicitly overrides to be non-internal now do show up. That's of course the entire goal!

🎉