ImageItem should have an "derivatives" computed property, to expose all image style URLs

A follow-up for this: #2825814: [PP-1] Allow URL query argument to limit the image style URLs returned by ImageItemNormalizer.

#4:

This was similar to #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata \Drupal\Core\Cache\CacheDependencyBubbleTrait was copied directly from there

Yay!

But EntityReferenceFieldItemNormalizer which this extends provides the absolute URL.

That's not a file URL. All file URLs must be root-relative since #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send.

#5.2:

I think using an absolute URL is the right thing to do, because it makes it easier to use in a custom client. In case it would be a relative URL, the client code would need to know the base URL, do the appending of strings etc.

I humbly disagree. Absolute URLs are fine for fully decoupled apps where the end user-facing app is a web app that lives on a different domain or is a native app. But in all other cases, the root-relative URL is much more useful. It's much easier to do domain + string_the_rest_api_sent_me than it is to parse a URL into the separate components and then reassemble correctly again. And for progressively decoupled use cases, you'll want relative URLs anyway.

#6:

Which brings up should we be doing the same thing in the HAL module?

IMO it should be done here. Fix the normalization across all supported formats in one go. It's very closely related anyway. And probably you can even share some of the test coverage code. Which also benefits from updating the default normalization and HAL's at the same time.

1. +++ b/core/lib/Drupal/Core/Cache/CacheDependencyBubbleTrait.php
   @@ -0,0 +1,34 @@
   +trait CacheDependencyBubbleTrait {
   

   The equivalent in #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata was updated significantly. Let's pull in the latest iteration from there.

2. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
   @@ -0,0 +1,60 @@
   +class ImageFieldItemNormalizer extends EntityReferenceFieldItemNormalizer {
   

   s/ImageFieldItem/ImageItem/

3. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
   @@ -0,0 +1,60 @@
   +    /** @var \Drupal\file\Entity\File $image */
   

   Must use interface.

4. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
   @@ -0,0 +1,60 @@
   +    $image = $this->entityTypeManager->getStorage('file')->load($data['target_id']);
   

   Why not use File::loadMultiple()?

5. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
   @@ -0,0 +1,60 @@
   +    /** @var \Drupal\image\Entity\ImageStyle[] $styles */
   

   Must use interface.

6. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
   @@ -0,0 +1,60 @@
   +    $styles = $this->entityTypeManager->getStorage('image_style')->loadMultiple();
   

   Why not use ImageStyle::loadMultiple()?

7. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageFieldItemNormalizerTest.php
   @@ -0,0 +1,107 @@
   +    /** @var \Drupal\image\Entity\ImageStyle $image_style */
   

   Interface.

8. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageFieldItemNormalizerTest.php
   @@ -0,0 +1,107 @@
   +      $cache_tags = array_merge($cache_tags, $image_style->getCacheTags());
   

   Use Cache::mergeTags().

9. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageFieldItemNormalizerTest.php
   @@ -0,0 +1,107 @@
   +    $image_style_keys = array_keys($data['image_test'][0]['image_styles']);
   

   $image_style_ids

+++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
@@ -0,0 +1,42 @@
+  /**
+   * Gets the render service.
+   *
+   * @return \Drupal\Core\Render\RendererInterface
+   *   The renderer.
+   */
+  protected function renderer() {
+    return isset($this->renderer) ? $this->renderer : \Drupal::service('renderer');
+  }

This method was removed in the other issue.

Other than that, this looks great. All the remains, is HAL support.

Cool. :) So, now zero nits remain, we just need to update the HAL normalization to do the same!

Great feedback, thanks @garphy!

I think having width + height makes sense also from a decoupled POV: if you want to use those image URLs to generate <img src="URL" width="WIDTH" height="HEIGHT">, then you need width and height. <img src="URL"> (without width+height) is a bad practice. It causes poor performance, because it requires the browser to reflow layout, since it cannot know the image dimensions.

So +1 from that POV.

I think this looks great. I have a bunch of nitpicks, and some naming concerns that are easily fixed.

There is one bigger concern: BC. Can we really just enable this? This would change the responses, just like #2751325: All serialized values are strings, should be integers/booleans when appropriate, only much more so! That one has a BC layer, this one does not. Yet that one is getting BC concerns raised. So I'm pretty sure they'll be raised here too.

1. +++ b/core/modules/image/src/ImageServiceProvider.php
   @@ -0,0 +1,51 @@
   +      // serializer.normalizer.entity_reference_item.hal which 10.
   

   Nit s/which 10/which is 10/

2. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,71 @@
   + * ImageItem normalizer to provide URLs to image styles.
   

   s/ImageItem normalizer/ImageItem HAL normalizer/

3. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,71 @@
   +   * Constructs an ImageItemNormalizer object.
   

   ImageItemHalNormalizer

4. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,41 @@
   +   * Adds images style information to normalized ItemItem field data.
   

   s/images/image/
   s/ItemItem/ImageItem/

5. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,41 @@
   +   *   The field image item.
   

   I think: The image field.? It currently is pretty strange.

6. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,41 @@
   +   * @param array $data
   +   *   The normalized field data.
   

   $data is super generic.

   I think this would be clearer:

   * @param array $normalization.
   *   The image field normalization to add image style information to.
   

7. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemHalNormalizerTest.php
   @@ -0,0 +1,36 @@
   +class ImageItemHalNormalizerTest extends ItemItemNormalizerBase {
   
   +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTest.php
   @@ -0,0 +1,23 @@
   +class ImageItemNormalizerTest extends ItemItemNormalizerBase {
   
   +++ b/core/modules/image/tests/src/Kernel/Normalizer/ItemItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   + * Base class for ItemItemNormalizer testing.
   ...
   +abstract class ItemItemNormalizerBase extends KernelTestBase {
   

   ItemItemNormalizerBase?

   ImageItemNormalizerBase!

8. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ItemItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +    // Change all images style scale effect to upscale.
   

   Needs a touch of language improvement :)

9. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ItemItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +   * @param array $data
   

   s/$data/$normalization/

1. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,32 @@
   +   * Bubbles cacheability metadata to the current render context.
   

   "to the current render context, if any"

2. +++ b/core/modules/image/src/ImageServiceProvider.php
   @@ -0,0 +1,51 @@
   +class ImageServiceProvider implements ServiceProviderInterface {
   

   Why can't we put this in image.services.yml?

   Let's document the reason.

3. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
   @@ -0,0 +1,64 @@
   +  public function normalize($object, $format = NULL, array $context = []) {
   +    $data = parent::normalize($object, $format, $context);
   +    if (empty($data['target_id'])) {
   +      return $data;
   +    }
   +    $this->addImageStyles($object, $data);
   +    return $data;
   +  }
   

   This is probably THE most essential part of the patch. What this means is that image items are normalized just like before… but we just also add image style information to the normalization, if the field is not empty.

   But I think the logic can be much simpler:

   if (!$object->isEmpty()) {
     $this->addImageStyles($object, $data);
   }
   

   Bonus points for renaming $object to $field_item and adding a type hint comment.

4. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,41 @@
   +    if ($image = $this->entityTypeManager->getStorage('file')->load($item->target_id)) {
   ...
   +      $styles = $this->entityTypeManager->getStorage('image_style')
   +        ->loadMultiple();
   

   Why not File::load() and ImageStyle::loadMultiple()?

   Easier to read, and we would no longer need to have the entity type manager injected.

5. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +abstract class ImageItemNormalizerBase extends KernelTestBase {
   

   The name currently doesn't make it clear that this is a test class.

   This should be renamed to ImageItemNormalizerTestBase.

6. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +  protected $image;
   +
   +
   +  /**
   

   Extraneous blank line.

7. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +    $this->serializer = \Drupal::service('serializer');
   

   $this->serializer = $this->container->get('serializer');

8. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +    FieldStorageConfig::create(array(
   

   s/array()/[]/

9. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
   @@ -0,0 +1,168 @@
   +    $data = $this->container->get('renderer')
   

   Let's rename $data to either $normalization or $normalized_entity.

10. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
    @@ -0,0 +1,168 @@
    +   * Gets the expected cacheability metadata.
    

    s/expected/expected bubbled/

11. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerBase.php
    @@ -0,0 +1,168 @@
    +   *   The cacheability metadata.
    

    s/The/The expected bubbled/

Thanks!

2) I don't see that comment?

RE: the additional thing: great find! :) That's very recent, but it's great to have it done correctly here! Well-spotted!

Interdiff review:

1. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -41,18 +33,15 @@ class ImageItemHalNormalizer extends EntityReferenceItemNormalizer {
   +   * @param Drupal\serialization\LinkManager\LinkManager $link_manager
   

   Must be typehinted to the interface, not the implementation.

2. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -41,18 +33,15 @@ class ImageItemHalNormalizer extends EntityReferenceItemNormalizer {
   -  public function __construct(LinkManagerInterface $link_manager, EntityResolverInterface $entity_Resolver, EntityTypeManagerInterface $entity_type_manager, RendererInterface $renderer) {
   +  public function __construct(LinkManagerInterface $link_manager, EntityResolverInterface $entity_Resolver, RendererInterface $renderer) {
   

   It's done correctly here :)

Patch review:

1. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,60 @@
   +/**
   + * ImageItem HAL normalizer to provide URLs to image styles.
   + */
   +class ImageItemHalNormalizer extends EntityReferenceItemNormalizer {
   
   +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
   @@ -0,0 +1,51 @@
   +/**
   + * ImageItem normalizer to provide URLs to image styles.
   + */
   +class ImageItemNormalizer extends EntityReferenceFieldItemNormalizer {
   
   +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,42 @@
   +trait ImageItemNormalizerTrait {
   

   After I posted #43 yesterday, I was thinking a bit more about this.

   These are two new normalizers. But ImageItems were already normalized jsut fine without this patch.

   What this patch is doing, and what these normalizers are doing is not normalizing ImageItems but decorating ImageItem normalizations!

   Think about it: image styles are NOT part of ImageItems in any way: image items are 100% unaware of image styles.

   It's just that we are deciding that it makes sense to provide image style information for every image in an image field.

   So… I'm now thinking that it'd be better to name this ImageItemWithImageStyle(Hal)Normalizer (and ImageItemWithImageStyleNormalizerTrait), which would fully explain what this is doing.

   Or something like that. What do you think? tedbow, what do you think?

2. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,60 @@
   +      $this->addImageStyles($object, $data['_embedded'][$field_key][0]);
   
   +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
   @@ -0,0 +1,51 @@
   +      $this->addImageStyles($field_item, $data);
   
   +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,42 @@
   +  protected function addImageStyles(ImageItem $item, array &$data) {
   

   The addImageStyles() method would then also need to be renamed to decorateWithImageStyles().

3. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemHalNormalizerTest.php
   @@ -0,0 +1,36 @@
   +  public static $modules = ['system', 'entity_test', 'serialization', 'image', 'field', 'user', 'file', 'hal', 'rest'];
   

   The rest module no longer needs to be enabled as of #2758897: Move rest module's "link manager" services to serialization module + #45's insight to not use the deprecated REST link manager, but the Serialization link manager :)

   Also, this does not need to list the system, image etc modules, it inherits that from the base class automatically! This can be reduced to just ['hal'] now :)

4. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -0,0 +1,167 @@
   +   * The format to use in normalization.
   

   The format whose normalization to test.

This is now also blocking #2748617: EntityResource only sets entity cache tags on response; cache tags for entity reference fields missing because this adds ConditionalCacheabilityMetadataBubblingTrait, which that issue also needs.

#51: great point! So perhaps best to stick with ImageItemNormalizer, but make it very explicit in the documentation that this is decorating the default normalization with image style metadata.

Just discussed this with @tedbow and the Serialization module maintainer, @damiankloip. He agrees with Ted. The name of the class should always match the $supportedInterfaceOrClass property. So, it should be ImageItemNormalizer, and we can make it clear in the documentation that it's just decorating the default normalization.

1. +++ b/core/modules/image/src/ImageServiceProvider.php
   @@ -0,0 +1,51 @@
   + * Provides additional normalizer services for image field.
   

   Provides normalizer services for the "image" field type.

2. +++ b/core/modules/image/src/ImageServiceProvider.php
   @@ -0,0 +1,51 @@
   + * The services are only set if the serialization and hal modules are enabled.
   

   Well this does not explain why we can't put these in image.services.yml.

3. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,60 @@
   +    $data = parent::normalize($object, $format, $context);
   

   Nit: s/$data/$normalization/

   Also in the other normalizer class.

4. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,60 @@
   +    if (!empty($data['_embedded'])) {
   

   Why can't we use !$field_item->isEmpty() here, just like we do in ImageItemNormalizer?

5. +++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
   @@ -0,0 +1,60 @@
   +      $this->decorateWithImageStyles($object, $data['_embedded'][$field_key][0]);
   

   I started to wonder whether this was actually correct: whether this still worked for multi-valued fields. My manual testing shows it works. And stepping through it with a debugger shows that this indeed makes sense!

6. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
   @@ -0,0 +1,53 @@
   +  public function normalize($field_item, $format = NULL, array $context = []) {
   +    $data = parent::normalize($field_item, $format, $context);
   +    if (!$field_item->isEmpty()) {
   

   This is still missing a

   // @var \Drupal\image\Plugin\Field\FieldType\ImageItem $field_item
   

   comment.

Point 2 is my main concern. Once that is fixed, this is RTBC.

Hurray :)

+++ b/core/modules/image/src/Normalizer/ImageItemHalNormalizer.php
@@ -0,0 +1,61 @@
+class ImageItemHalNormalizer extends EntityReferenceItemNormalizer {
...
+   * Constructs an ImageItemHalNormalizer object.

Let's rename this to ImageItemNormalizer. All of the existing normalizers in the HAL module also don't mention Hal in their classnames!

Ah, damn, #64 does not make sense, because we have ImageItemNormalizer for serialization (i.e. default normalizer) and ImageItemHalNormalizer for hal, and both live in the same directory and namespace. That's why #64 is not feasible.

1. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,32 @@
   +trait ConditionalCacheabilityMetadataBubblingTrait {
   
   +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -0,0 +1,42 @@
   +        $this->bubble($style);
   

   The way the jsonapi module does this instead:

   $serializer->serialize($data, $format, ['request' => $request, 'cacheable_metadata' => $response->getCacheableMetadata()]);
   

   We could make \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody() work int he same way.

   Then we wouldn't be bubbling onto "the current global render context" anymore. Which feels a lot less brittle.

2. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -0,0 +1,171 @@
   +abstract class ImageItemNormalizerTestBase extends KernelTestBase {
   

   And it means this can become a unit test!

Thoughts? @tedbow in particular?

1. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
   @@ -34,7 +37,9 @@ protected function decorateWithImageStyles(ImageItem $item, array &$normalizatio
   +          $context['cacheable_metadata']->addCacheableDependency(CacheableMetadata::createFromObject($style));
   

   This can be simply

   $context['cacheable_metadata']->addCacheableDependency($style);
   

   Because ImageStyle implements EntityInterface which implements CacheableDependencyInterface.

2. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -109,14 +108,11 @@ public function testNormalize() {
   +    $normalization = $this->serializer->normalize($entity, $this->format, ['cacheable_metadata' => $cacheability_metadata]);
   

   s/cacheable_metadata/cacheability/

Perfect!

Drupal CI had a bad moment.

Retesting. Might be a legitimate fail though. Cache contexts for GET responses have changed, it seems, due to some commit today (EDIT: likely #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL).

Sorry, by reroll I meant "reroll and need small patch fix", I don't mean "reroll to apply cleanly on latest HEAD".

#83 will fail.

Sigh, you're right, that's simply something that's been forgotten all this time in HAL responses.

EDIT: IMO that's something that's out of scope for this issue though; we need a follow-up for that.

I wanted to RTBC but …

I am assuming both these issues are because we changed \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody to not use executeInRenderContext().

Well, isn't that a problem then? That's a BC break!

+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -151,14 +151,14 @@ protected function renderResponseBody(Request $request, ResourceResponseInterfac
+      if ($response instanceof CacheableResponseInterface) {
+        if (!$context->isEmpty()) {
+          $response->addCacheableDependency($context->pop());
+        }
+        $serialization_contexts['cacheability'] = $response->getCacheableMetadata();

This is not making any sense. This is popping the render context and adding its cacheability metadata to the response… but we never use the render context anymore in this new code!

I asked for this in #66. It makes normalizers fully unit testable (since you don't need to set up a global render context etc). It makes the code easier to follow, because less magic. @tedbow implemented it in #67.
However, what I didn't notice/realize, is that this suggestion of mine:

Then we wouldn't be bubbling onto "the current global render context" anymore. Which feels a lot less brittle.

… is blatantly false. Doing that implies doing a BC break. Because until now, we explicitly supported this, and for good reason. Let me quote the docs of \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody():

   * Serialization can invoke rendering (e.g., generating URLs), but the
   * serialization API does not provide a mechanism to collect the
   * bubbleable metadata associated with that (e.g., language and other
   * contexts), so instead, allow those to "leak" and collect them here in
   * a render context.

This change makes that statement untrue! Which means this is a BC break!

So, I think we need to revert that change. I think we can and should still do the explicit cacheability metadata bubbling from normalizers by using $serialization_context['cacheability']. But we should also still continue to support the old mechanism. Not doing that implies a BC break.

So, did that. And turns out @tedbow is right: that allows us to revert the changes in #87 + #90.

Also, this means that the existing documentation is wrong, and needed updating. Changed

   * Serialization can invoke rendering (e.g., generating URLs), but the
   * serialization API does not provide a mechanism to collect the
   * bubbleable metadata associated with that (e.g., language and other
   * contexts), so instead, allow those to "leak" and collect them here in
   * a render context.

to:

   * During serialization, encoders and normalizers are able to explicitly
   * bubble cacheability metadata via the 'cacheability' key-value pair in the
   * received context. This bubbled cacheability metadata will be applied to the
   * the response.
   *
   * In prior versions of Drupal 8, we allowed implicit bubbling of cacheability
   * metadata because there was no explicit cacheability metadata bubbling API.
   * To maintain backwards compatibility, we continue to support this, but
   * support for this will be dropped in Drupal 9.0.0. This is especially useful
   * when interacting with APIs that implicitly invoke rendering (for example:
   * generating URLs): this allows those to "leak", and we collect their bubbled
   * cacheability metadata automatically in a render context.

because now there is an API to explicitly bubble cacheability metadata!

This also means we'll need a change record for this.

Thoughts? Agreed with this direction?

I reviewed + RTBC'd @tedbow's ImageItemNormalizer code.

@tedbow can reviewed + RTBC my changes in #91. I first need to add a CR though.

CR created: https://www.drupal.org/node/2865042.

#96.1: that's a new API added in #2652138: Image styles do not play nicely with SVGs, so we couldn't have possibly done that until now!

But yay, that seems like an easy reroll :)

#100 hahahahah, I was struggling with that — was contemplating asking for it. I wasn't sure. It's definitely better to have it.

1. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -97,11 +99,18 @@ protected function setUp() {
   +      'uri' => $uri,
   

   Nit: could use setFileUri().

2. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -150,18 +165,41 @@ public function testNormalize() {
   +   * @param bool $styles_included
   

   I think $styles_included is a bit strange, but ok. Can't think of anything better atm.

3. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTestBase.php
   @@ -150,18 +165,41 @@ public function testNormalize() {
   +   * Datatprovider for testNormalize().
   

   Nit: s/Datatprovider/Data provider/

This has effectively been postponed on that issue for >2 months now.

#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in, #2910211: Allow computed exposed properties in ComplexData to support cacheability. is the last remaining blocker (was split off from #2871591 to make its scope narrower and help it land sooner), and that too is now RTBC!

#2910211: Allow computed exposed properties in ComplexData to support cacheability. also landed, this is now fully unblocked!

I've rerolled #101 entirely in the spirit of #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts. I got some inspiration on how to add a computed property that contains a list of things from \Drupal\entity_test\Plugin\Field\ComputedTestFieldItemList.

Rolling this patch took me more than 6 hours of work, because Typed Data is excruciatingly difficult to figure out. Hopefully once there's a bunch of examples, it'll become much simpler for others to do.

The result in the REST module:

And together with #2921257-6: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() (to leverage #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts), it even works automatically in the contrib JSON API module:

Hah, the Media entity has this base field:

$fields['thumbnail'] = BaseFieldDefinition::create('image')

Which means this patch is automatically adding image styles for the thumbnail. Proving that it works beautifully everywhere where image fields are used!

Interesting fact: only the ?_format=json tests are failing, the ?_format=hal_json tests are passing, because the "image_styles" computed property simply does not show up for hal_json. This is because ImageItem extends FileItem extends EntityReferenceItem, and the HAL normalizers treat entity reference fields in a special way (and therefore also image fields).
I'd say we have to fix that first, but … the HAL normalization intentionally chose to do this. In fact, for this same reason, the hal_json-formatted response also doesn't allow you to get the alt, width or height properties of image fields. image_styles is merely another in that list.

+++ b/core/modules/image/src/Plugin/DataType/ComputedImageStyleList.php
@@ -0,0 +1,92 @@
+class ComputedImageStyleList extends ItemList {

I think this should actually extend Map, not ItemList.

I think that @e0ipso is implying at #2921257-15: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() that image_styles is a bad name for this computed property, because it's very Drupal-y. derivatives would be a better name. That also happens to be the name that is used in the codebase. "Image Styles" is kind of the human-readable name for the same concept.

So, how do people feel about that? I personally think that probably makes more sense.

#110 was failing because for some reason, the width and height are strings for the large and medium image styles, but integers for the thumbnail styles.

We can easily fix that: casting them all to integers.

But then the

    // BC: serialization_update_8302().
    // Only run this for fieldable entities. It doesn't make sense for config
    // entities as config values are already casted. They also run through the
    // ConfigEntityNormalizer, which doesn't deal with fields individually.
    if ($this->entity instanceof FieldableEntityInterface) {
      // Test primitive data casting BC (strings).
      $this->config('serialization.settings')->set('bc_primitives_as_strings', TRUE)->save(TRUE);

test coverage fails. Because \Drupal\image\Plugin\DataType\ComputedImageStyleList::computeImageStyleMetadata() returns [string, int, int], not [StringData, IntegerData, IntegerData]. Fixing that is for a next comment. For similar reasons, the XML test coverage is going to fail.

So in this patch, I commented out the "primitives as strings" BC layer test coverage. I also disabled the XML "GET" test coverage. Just to prove the general approach.

This patch should be green, and hopefully this patch will finally start getting the reviews it needs to be able to move forward.

And this then implements #113.

Ugh, the #116 interdiff was correct, but the patch was not. It was missing one hunk of the interdiff.

Damn, #119's interdiff is wrong. Attached is the interdiff between #116 and #119. Sorry for the confusion!

So:

1. @Berdir in #115 preferring "image styles"
2. @e0ipso in #121 preferring "derivatives"

I see both points of view. But I think that any Drupalist will recognize image style URLs even if they're under the "derivatives" key. And "derivatives" makes more sense to the broader world. So I do tend to lean towards @e0ipso's preference/recommendation here.

1. 👍 Yeah, is a remnant. Fixed. Also renamed the class to ComputedImageStyle.
2. 👍 Another remnant.
3. No, as the code comment says: see #114 for the explanation. The current code for ComputedImageStyle::getValue() is hacky. It shouldn't return PHP primitives, it should return TypedDataInterface objects implementing PrimitiveInteface.
   I literally can't figure out how to implement that, because I can't just do new IntegerData(…), because a data definition must be passed.

   This patch needs review from a Typed Data expert I think.

4. Probably, but I'd first like sign-off on the way this uses Typed Data (and fix it, per the previous point).

This patch needs review from a Typed Data expert I think.

I think this needs something like the attached patch.

In any case, this can be updated to use ComputedItemListTrait, introduced in #2392845: Add a trait to standardize handling of computed item lists, right? Well, it doesn't work, for some reason $this->valueComputed starts of as TRUE! I suspect PathItemList::$valueComputed's value is somehow being transfered to ComputedImageStyleList… :O

#125 + #126 I've already had this discussion with @e0ipso in #2921257-14: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() through #2921257-17: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal(). Please read my arguments there too.

If this would be done in context of REST/serializer, then I could imagine it being controlled by a query argument like ?include_image_styles=medium,thumbnail. But having this on the entity field level makes that more complicated.

#2825814: [PP-1] Allow URL query argument to limit the image style URLs returned by ImageItemNormalizer exists for that (added in #2!). The default would still have to include all image styles. Because A) having dozens of image styles is not the norm, B) it ensures good discoverability.