Problem/Motivation

This is a sister issue of #2517030: Add a URL formatter for the image field and #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field. The former issue solved it for image entities exposed via Views REST export displays. This issue must solve it at the serialization/normalization level, so that all REST resources (as well as JSON API) benefit.

The latter issue already is doing it for FileItem (and hence ImageItem because it extends FileItem), but only for the "canonical" file URL. In the case of ImageItem, we also want image style URLs.

Proposed resolution

Add ImageItemNormalizer.

Remaining tasks

None.

User interface changes

None.

API changes

Adds an API to allow explicit bubbling of cacheability metadata: pass in a 'cacheability' => instanceof CacheableMetadata key-value pair in the serialization context.

(The ImageItem normalizer that we add here uses this to its advantage: this new capability allows that normalizer to be properly unit-testable. No more global state manipulation! See #67's interdiff.)

Data model changes

None.

CommentFileSizeAuthor
#127 2825812-properly_use_typed_data_but_i_cant_figure_it_out-do-not-test.patch3.67 KBWim Leers
#124 2825812-124.patch11.89 KBWim Leers
#124 interdiff.txt2.16 KBWim Leers
#120 interdiff.txt862 bytesWim Leers
#119 2825812-119.patch11.79 KBWim Leers
#119 interdiff.txt1.12 KBWim Leers
#116 2825812-115.patch11.79 KBWim Leers
#116 interdiff.txt2.65 KBWim Leers
#114 2825812-114.patch11.79 KBWim Leers
#114 interdiff.txt5.51 KBWim Leers
#110 2825812-110.patch8.63 KBWim Leers
#110 interdiff.txt2.55 KBWim Leers
#107 2825812-107.patch6.15 KBWim Leers
#107 Screen Shot 2017-11-09 at 12.01.17.png97.55 KBWim Leers
#107 Screen Shot 2017-11-09 at 11.57.30.png80.94 KBWim Leers
#101 2825812-101.patch20.16 KBtedbow
#101 interdiff-98-101.txt5.75 KBtedbow
#98 2825812-98.patch19.15 KBtedbow
#98 interdiff-91-98.txt1.71 KBtedbow
#91 2825812-91.patch19.08 KBWim Leers
#91 interdiff.txt5.1 KBWim Leers
#90 2825812-90.patch18.1 KBtedbow
#90 interdiff-87-90.txt581 bytestedbow
#87 2825812-87.patch17.53 KBtedbow
#87 interdiff-83-87.txt669 bytestedbow
#83 interdiff-72-82.txt526 bytesJo Fitzgerald
#83 2825812-82.patch16.88 KBJo Fitzgerald
#72 interdiff-70-72.txt4.3 KBtedbow
#72 2825812-72.patch16.92 KBtedbow
#70 interdiff-2825812-67-70.txt1.91 KBhimanshu-dixit
#70 2825812-70.patch16.97 KBhimanshu-dixit
#67 2825812-67.patch17.01 KBtedbow
#67 interdiff.txt9.68 KBtedbow
#62 2825812-62.patch17.14 KBtedbow
#62 interdiff-60-62.txt5.01 KBtedbow
#60 2825812-60.patch16.42 KBtedbow
#60 interdiff-57-60.txt1.91 KBtedbow
#57 2825812-57.patch16.36 KBmartin107
#57 interdiff-2825812-52-57.txt7.05 KBmartin107
#52 interdiff-2825812-49-52.txt1.35 KBmartin107
#52 2825812-52.patch16.71 KBmartin107
#49 interdiff-2825812-45-49.txt11.04 KBmartin107
#49 2825812-49.patch16.68 KBmartin107
#45 interdiff-2825812-41-45.txt11.31 KBmartin107
#45 2825812-45.patch16.13 KBmartin107
#41 2825812-41.patch16.99 KBtedbow
#41 interdiff-38-41.txt6.93 KBtedbow
#38 interdiff-34-38.txt3.43 KBtedbow
#38 2825812-38.patch16.88 KBtedbow
#34 interdiff-30-34.txt11.49 KBtedbow
#34 2825812-34.patch16.72 KBtedbow
#30 interdiff-27-30.txt7.83 KBtedbow
#30 2825812-30.patch14.29 KBtedbow
#27 2825812-27.patch10.21 KBtedbow
#27 2825812-27-plus-2827218-47.patch41.74 KBtedbow
#27 interdiff-26-27.txt734 bytestedbow
#26 2825812-26.patch10.21 KBtedbow
#26 interdiff-19-26.txt1.24 KBtedbow
#26 2825812-19-plus-2827218-47.patch41.66 KBtedbow
#19 interdiff-11-19.txt3.13 KBtedbow
#19 2825812-19.patch10.14 KBtedbow
#11 interdiff-9-11.txt4.3 KBtedbow
#11 2825812-11.patch8.89 KBtedbow
#9 interdiff-6-9.txt7.42 KBtedbow
#9 2825812-9.patch8.79 KBtedbow
#6 interdiff-4-6.txt1.84 KBtedbow
#6 2825812-6.patch8.33 KBtedbow
#4 2825812-3-image_styles_normalization.patch8.23 KBtedbow
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

tedbow’s picture

Assigned: Unassigned » tedbow

Working on this.

tedbow’s picture

Status: Active » Needs review
FileSize
8.23 KB

ok here is patch.
It has a test.

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

Couple questions I had:
#2517030: Add a URL formatter for the image field provides the image styles URLs using relative urls. I used relative urls here. But EntityReferenceFieldItemNormalizer which this extends provides the absolute URL. so for images field items you get both like this:

 (
                    [target_id] => 1
                    [alt] => jkk
                    [title] => 
                    [width] => 970
                    [height] => 1339
                    [target_type] => file
                    [target_uuid] => 96572986-4381-4037-ae87-534353c72d9f
                    [url] => http://www.octo2.dev/d8_3_rest/sites/default/files/2016-11/sadtear.jpg
                    [image_styles] => Array
                        (
                            [large] => /d8_3_rest/sites/default/files/styles/large/public/2016-11/sadtear.jpg?itok=GEE2-4vL
                            [medium] => /d8_3_rest/sites/default/files/styles/medium/public/2016-11/sadtear.jpg?itok=GPXdb1lB
                            [thumbnail] => /d8_3_rest/sites/default/files/styles/thumbnail/public/2016-11/sadtear.jpg?itok=O2emam6p
                        )

                )

Should we change this to use absolute URLs or provide and extra 'image_url' or should EntityReferenceFieldItemNormalizer provide relative?

I added the cache dependencies for each image styles because if you change the machine name or deleted an image style the urls would update. but also if you added another image style the array would need to be updated. Not sure how to handle that.

dawehner’s picture

This looks pretty good!

  1. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
    @@ -0,0 +1,62 @@
    +    $styles = $this->entityTypeManager->getStorage('image_style')->loadMultiple();
    +    if ($styles) {
    +      $data['image_styles'] = [];
    

    I'm wondering whether we should always provide the 'image_styles' key, so client code does not have to explicitly check it.

  2. +++ b/core/modules/image/src/Normalizer/ImageFieldItemNormalizer.php
    @@ -0,0 +1,62 @@
    +        $data['image_styles'][$id] = file_url_transform_relative($style->buildUrl($uri));
    

    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.

tedbow’s picture

@dawehner thanks for the review!

1,2 makes sense and fixed

I also change the priority of the new service because it has to be lower than serializer.normalizer.entity_reference_item.hal or it will break Image fields for HAL.
Which brings up should we be doing the same thing in the HAL module? If so should that be a follow up?

If we don't do it in for HAL should still in follow up create a test in HAL for any field type that extends EntityReference that current normalizer works. Because this normalizer with the higher priority broke Image references in HAL but there were no tests. The same could happen with \Drupal\file\Plugin\Field\FieldType\FileItem

tedbow’s picture

Assigned: tedbow » Unassigned

Unassigning

Wim Leers’s picture

Status: Needs review » Needs work

#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

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.79 KB
7.42 KB

#5.2 added back file_url_transform_relative

#6 ok. will do hal in this issue(not in this patch)

1. pulled in other version with new name though had to add back \Drupal\Core\Cache\ConditionalCacheabilityMetadataBubblingTrait::renderer

2. fixed - also changed test name to match
3. fixed
4. you mean File::load() not File::loadMultiple() right? if so fixed
5-9 fixed

Wim Leers’s picture

+++ 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.

tedbow’s picture

#9.2 I didn't actually update the test name. Fixed.

re #10

This method was removed in the other issue.

Fixed

Also fix couple small nits in Test.

tedbow’s picture

Here are the nits I was referring to in #11

  1. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
    @@ -29,13 +30,23 @@ class ImageItemNormalizer extends EntityReferenceFieldItemNormalizer {
    +   * The renderer.
    +   *
    +   * @var \Drupal\Core\Render\RendererInterface
    +   */
    +  protected $renderer;
    +
    +  /**
    

    Didn't declare field

  2. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ImageItemNormalizerTest.php
    @@ -66,7 +73,6 @@ protected function setUp() {
    -    $this->imageFactory = $this->container->get('image.factory');
    

    This was never used. c/p error

Wim Leers’s picture

Status: Needs review » Needs work

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

garphy’s picture

Just a thought, we used to implement this in D7 (as an extension of the Service Entity module.

We started exactly like this patch, outputting an associative array of URL indexed by style name but we quickly concluded that it is in most case not enough as there is other useful pieces of information alongside the style URL itself, such as the resulting derivative dimensions.

So instead of just a string literal, we output an associative array with the "URL", "width" and "height" keys.

I would suggest switching to that kind of output.

Wim Leers’s picture

Great feedback, thanks @garphy!

tedbow’s picture

re #14 I think this could open up can of worms.

So instead of just a string literal, we output an associative array with the "URL", "width" and "height" keys.

We can't determine the width or height by just looking at the image style. For instance if you scale an image but only specify height width will be different for each image.
Also if you don't "Allow Upscaling" then you can't tell either because the image could be smaller and then not scaled. Also what about all the contrib image effects where we don't know how to determine the dimensions?

If we had to look at each individual processed image to see what the dimensions are what about images where a particular(or all) image style derivatives have not been made yet? are they still made when first requested? Do we force them to be created so we can provide dimensions?

garphy’s picture

I've not checked if it's still the case with D8 API (Sorry!) but with D7's Style API, each style operation provide a callback used to compute the final dimension of the image without actually execute the operations, thus allowing to compute the resulting dimensions without creating all possible derivatives.

Of course there will still be edge cases were this precalculation cannot be made, but we still can provide it for the ~90% use cases.

When an (integrated) drupal theme outputs a Only local images are allowed. tag, it use this API to know the dimensions of the image (populating the width/height attributes) without actually generate the image. It'll be generated when the browser will actually request it.

tedbow’s picture

I've not checked if it's still the case with D8 API (Sorry!)

No problem at all.

but with D7's Style API, each style operation provide a callback used to compute the final dimension of the image without actually execute the operations, thus allowing to compute the resulting dimensions without creating all possible derivatives.

Interesting I will look to see if I can find this in D8.

If it exists still I would be interested if anyone knows what kind of overhead that might involve. Thinking of entities with lots image fields or multiple value image fields with lots of images on 1 entity.

UPDATE:
Ok this is probably it \Drupal\image\ImageStyleInterface::transformDimensions
The docs don't explicitly say the derivative is not created but that would make sense.

So I guess if the overhead is not a concern this seems doable.

tedbow’s picture

Status: Needs work » Needs review
FileSize
10.14 KB
3.13 KB

Ok here is patch does @garphy suggestion in #14 and adds the height and width as well as url.

It also updates the tests. In the test I had to set all the effect to "upscale" or the dimensions would be the same for all styles.

As per @Wim Leers' comment on #13 this still needs the hal work done.

tedbow’s picture

Re my worry

So I guess if the overhead is not a concern this seems doable.

I was imagining some image processing going on which is not done. At least for scaling this is done in \Drupal\Component\Utility\Image::scaleDimensions which is just calculations.

dawehner’s picture

garphy’s picture

@dawhener: Why would it revert this ? In my understanding, Media brings in a new entity type alongside File entities, not replacing it.

Wim Leers’s picture

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.

damiankloip’s picture

Also totally +1 on having the height and width.

  1. +++ b/core/modules/image/src/ImageServiceProvider.php
    @@ -0,0 +1,35 @@
    +      $service_definition->addTag('normalizer', ['priority' => 7]);
    

    I think this is still ok after https://www.drupal.org/node/2827218 ?

  2. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
    @@ -0,0 +1,79 @@
    +  public function normalize($object, $format = NULL, array $context = array()) {
    

    nit: new array syntax [] ?

  3. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
    @@ -0,0 +1,79 @@
    +    $image = File::load($data['target_id']);
    ...
    +    $styles = ImageStyle::loadMultiple();
    

    Not sure if it matters too much in the grand scheme of things, but we could get the entity storage from the entity type manager and use load() from that. Same goes for both here.

  4. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
    @@ -0,0 +1,79 @@
    +        'url' => file_url_transform_relative($style->buildUrl($uri)),
    

    Do you think there is any value to having both absolute and relative? definitely relative is the more useful/flexible one.

tedbow’s picture

1. in #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods serialization.normalizer.field_item priority is set to 6 so 7 for image.normalizer.image_item would work(needs to be higher).
but just to be sure including a patch that combines 2825812-19.patch and 2827218-47.patch
2. fixed
3. With on going expansion of the universe what really matters in grand scheme of things?? but Fixed.
4. I would be in favor of just providing relative. clients should be able to easily create absolute if needed.

tedbow’s picture

The combination patch 2825812-19-plus-2827218-47.patch actually failed. Not sure why it is green.

I forgot that image.normalizer.image_item needs be higher than serializer.normalizer.entity_reference_field_item because an image reference is a special kind of entity reference.

The last submitted patch, 27: 2825812-27-plus-2827218-47.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2825812-27.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
14.29 KB
7.83 KB

Ok this patch provides a new \Drupal\image\Normalizer\ImageItemHalNormalizer
and \Drupal\image\Normalizer\ImageItemNormalizerTrait
The trait is used because the adding the Image style information is mostly the same between hal and default.

I haven't had time to create the new test yet but just thought I should post it before I finish for the day. Also will prove it doesn't break any of the existing tests(hopefully, worked locally).

Status: Needs review » Needs work

The last submitted patch, 30: 2825812-30.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review

retesting.

Status: Needs review » Needs work

The last submitted patch, 30: 2825812-30.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.72 KB
11.49 KB

Ok I am not sure what the CI error was in #30

This patch adds the testing for the Hal normalizer.

Status: Needs review » Needs work

The last submitted patch, 34: 2825812-34.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review

Test passed again

dawehner’s picture

  1. +++ b/core/modules/image/src/ImageServiceProvider.php
    @@ -0,0 +1,51 @@
    +      // Priority should be higher than
    +      // serializer.normalizer.entity_reference_field_item but lower than
    +      // serializer.normalizer.entity_reference_item.hal.
    +      $service_definition->addTag('normalizer', ['priority' => 9]);
    ...
    +      // Priority should be higher than
    +      // serializer.normalizer.entity_reference_item.hal which 10.
    +      // Priority of 20 gives the ability to have other normalizers between this
    +      // one and serializer.normalizer.entity_reference_item.hal.
    +      $service_definition->addTag('normalizer', ['priority' => 20]);
    

    Just an idea for the future and maybe for new normalizes, ... what about watching both service definitions get the priority and then choose a priority of (A+B)/2

  2. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
    @@ -0,0 +1,41 @@
    +    $image = $this->entityTypeManager->getStorage('file')
    +      ->load($item->target_id);
    +    $uri = $image->getFileUri();
    

    I'm wondering whether we should check whether this image actually exists.

  3. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
    @@ -0,0 +1,41 @@
    +      $dimensions = ['width' => $item->width, 'height' => $item->height];
    

    Note: This could be moved outside of the loop

  4. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ItemItemNormalizerBase.php
    @@ -0,0 +1,168 @@
    +    foreach ($normalized_image_styles as $id => $img_info) {
    

    IMHO we should not provide short variable names

tedbow’s picture

@dawehner thanks for the review.
1. It seems like having hard-coded numbers while not perfect at least allows custom and contrib normalizers to know where they are in relation to core normalizers. Though custom and contrib could also follow the dynamic method you suggested there are probably normalizers using hard-coded values and developers who are use to this.
2. Fixed
3. This actually has to be inside the loop because $dimensions is passed by reference to \Drupal\image\ImageStyleInterface::transformDimensions. Then the "Implementations have to store the resulting width and height, in pixels." So it is changed in with each iteration of the loop and needs to be reset the original image dimensions.
4. Good suggestion! changed $id to $image_style_id and $img_info to $image_style_dimensions

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
    @@ -19,22 +19,22 @@
    +        $dimensions = ['width' => $item->width, 'height' => $item->height];
    +        $style->transformDimensions($dimensions, $uri);
    

    The world is a sad place ... like I would have expected this method to return the transformed dimensions. Of course this is not caused by this particular patch.

  2. +++ b/core/modules/image/tests/src/Kernel/Normalizer/ItemItemNormalizerBase.php
    @@ -130,10 +130,10 @@ public function testNormalize() {
    +      $this->assertNotEmpty(strstr($image_style_dimensions['url'], "files/styles/$image_style_id/public/example.jpg"));
    

    Nitpick, when there is time between now and the commit: We could use assertContains here.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

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/

tedbow’s picture

Status: Needs work » Needs review
FileSize
6.93 KB
16.99 KB

#39.2 fixed

#40
1-4 fixed
5. Changed from "The field image item." to "The image field item."
IMHO "The image field." doesn't specify it is an individual item in a field list.
6,7 fixed
8. changed to "Set upscale to TRUE in all image style scale effects."
9. fixed

RE

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!

I think #2751325: All serialized values are strings, should be integers/booleans when appropriate is more disruptive because it changes the data types of existing parts of the response. This just adds new elements on the field item level which is more like #2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats. There was no BC layer for that. The client would have to be validating that no unknown keys on the field item level in both case to break.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs review » Needs work
  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/

martin107’s picture

Assigned: Unassigned » martin107

I've been following along for a while ...all of #43 looks reasonable

it is high time I contributed a little

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
16.13 KB
11.31 KB

1) Fixed.

2) Fixed by adding a dependencies comment to ImageServiceProvider

3-11) Fixed

In addition I fixed :-

ImageItemHalNormalizer was injected with a deprecated \Drupal\rest\LinkManager\LinkManagerInterface type object.
which feed it onto the parent EntityReferenceItemNormalizer which was actually expecting a \Drupal\serialization\LinkManager\LinkManagerInterface type object.

Status: Needs review » Needs work

The last submitted patch, 45: 2825812-45.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue tags: +blocker

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.

martin107’s picture

Status: Needs work » Needs review
FileSize
16.68 KB
11.04 KB

Interdiff Review:

2) I don't see that comment?

Fixed. [Me stares at the ground mumbling] I hear that happens SOMETIMES when you create a patch before you press save in your editor. #ThatIsMyLineAndIAmStickingToIt.

I have appended the word Interface t the annonation.

Thanks for keeping me honest!

Patch Review

1) 2) Fixed

In the last iteration the critical method was named addImageStyles() [Plural] so I pluralized the name changes

An example of the pattern I went with was

ImageItemWithStylesHalNormalizerTest

3) Fixed.

A Small nit I noticed while work on this

ImageItemNormalizerTrait::addImageStyles()

There was a disconnect between the annotated @parm name "normalization" and the method param name ... I went with $normalization.

On Sunday I will try and workout whatever screw up I introduced that broken testing..

Status: Needs review » Needs work

The last submitted patch, 49: 2825812-49.patch, failed testing.

tedbow’s picture

+++ b/core/modules/image/src/Normalizer/ImageItemWithStylesHalNormalizer.php
@@ -0,0 +1,60 @@
+   * @param Drupal\serialization\LinkManager\LinkManagerInterface $link_manager

Needs a "\"

Re @Wim Leers' comment #47

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.

The problem with ImageItemWithImageStyle(Hal)Normalizer is that this what we are doing with the normalizer now. But since we can only have 1 normalizer for ImageItem I think we should keep it generic. Because what happens when we find there is something else we need to do in ImageItem normalization(or denormalization) that does have to do with 1 of ImageItem properties? We have to add it to this class and we can't rename it. Probably makes sense to re-name the trait but I think it makes sense for normalizers to be name after the thing that they are normalizing.

martin107’s picture

Status: Needs work » Needs review
FileSize
16.71 KB
1.35 KB

#49

On Sunday I will try and workout whatever screw up I introduced that broken testing..

Fixed.

#51

Needs a "\"

Thanks fixed.

Hmm still thinking up a better name....for ImageItemWithImageStyle(Hal)Normalizer

Status: Needs review » Needs work

The last submitted patch, 52: 2825812-52.patch, failed testing.

Wim Leers’s picture

#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.

Wim Leers’s picture

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.

garphy’s picture

Quick feedback:
I was hoping that advances made with this patch would also be useful when using json:api.
But unfortunately, as style URLs are added at the field item level, it is merely "invisible" on json:api output which treat entity reference generically.
I think that image styles information (URL, size, ...) would be valuable at the file-entity level when the said entity is an image. As a result we could have this information when querying file entites with rest (entity/file/?_format=json) or when including referenced entities with json:api (api/node//?include=field_image).

martin107’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
16.36 KB

Just limiting the change to a rename.

tedbow’s picture

@garphy re

I think that image styles information (URL, size, ...) would be valuable at the file-entity level when the said entity is an image. As a result we could have this information when querying file entites with rest (entity/file/?_format=json)

The file entity itself doesn't know anything about styles. The only thing that makes image styles available I think is if upload as part of an image field. For a file entity if the file itself is an image file you can't tell if there should be image styles unless you check its usage in a image field. That is you could upload an image in a file field and then the images styles would not be available.

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/image/src/ImageServiceProvider.php
    @@ -0,0 +1,50 @@
    + * Provides additional normalizer services for image field items when the
    + * serialization and hal modules are enabled.
    

    Nit: class description should be 1 line then empty followed longer description if needed.

  2. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizer.php
    @@ -0,0 +1,53 @@
    +   * Constructs an ImageItemWithStylesNormalizer object.
    

    Need to update class name in comment

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
16.42 KB

Fixed nits I mentioned in #59 and 1 small other 1. All in comments

Wim Leers’s picture

Status: Needs review » Needs work
  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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.01 KB
17.14 KB

1. fixed
2. Expanded the comment to list the reasons which are the classes extend class from the hal and serialization modules, they have no use without these modules and the hal one will throw a ServiceNotFoundException because of a missing argument service.

I also nested the check for the hal module under the check for the serialization module. The hal service requires services from the serialization module. So even if serialization wasn't dependent on serialization this service would be.

3. fixed in both classes.
4. It can.fixed.
5. Yep, makes sense the way it is.
6. Added the comment to both classes. and change parameter name in \Drupal\image\Normalizer\ImageItemHalNormalizer::normalize to $field_item to match the other class.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Hurray :)

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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!

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
  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?

tedbow’s picture

1. that looks a good idea.
I implemented this. I could remove ConditionalCacheabilityMetadataBubblingTrait.

2. I left as a kernel test for now because wanted to see if would still pass. I had remove the url.site cache context from Hal test but think that is because not serializing in render context anymore.

Will update test to unit if this passes without breaking any rest tests.

Wim Leers’s picture

  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/

Wim Leers’s picture

Status: Needs review » Needs work
himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
16.97 KB
1.91 KB

Fixed #68 in this new patch. I am not sure about the second point, @WimLeers can you see if it is correct ?

Status: Needs review » Needs work

The last submitted patch, 70: 2825812-70.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.92 KB
4.3 KB

@himanshu-dixit thanks for fixing #68.1

I think for #68.2 @Wim Leers was just saying to swap out 'cacheability' for 'cacheable_metadata' in the array we use for the context array.
You should gotten some fails in the test b/c you updated the key but it needs to be the same in ImageItemNormalizerTrait, ImageItemNormalizerTestBase and ResourceResponseSubscriber.

I updated this.

There were a bunch of other fails just because #2854830: Move rest/serialization module's "link manager" services to HAL module landed since #67. So we need use the hal.link_manager service instead of the serialization.link_manager service

I just had to do this for another issue so I updated this too.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: 2825812-72.patch, failed testing.

denutkarsh’s picture

Status: Needs work » Reviewed & tested by the community

That was a unrelated fail. Returning the RTBC Status

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: 2825812-72.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Drupal CI had a bad moment.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: 2825812-72.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

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).

Status: Needs review » Needs work

The last submitted patch, 72: 2825812-72.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
martin107’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

2825812-72.patch applies for me 8.4.x, I am retesting.

BTW I am on commit df2fd66d5999884504ab1d66f921aa6d0bf73711

here is the git log output

Author: Alex Pott
Date: Mon Mar 27 11:39:13 2017 +0100

Issue #2843756

Jo Fitzgerald’s picture

Does not require re-roll - patches applies ok.

Removed unused use statement.

Wim Leers’s picture

Status: Needs review » Needs work

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.

tedbow’s picture

@wim re: #79

I could see that #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL would have changed the cache contexts and \Drupal\entity_test\Entity\EntityTestLabel entity doesn't have a canonical so for that reason the context 'url.site' would not be expected.

But it seems link since \Drupal\hal\LinkManager\LinkManagerBase::getLinkDomain might use $request->getSchemeAndHttpHost() . $request->getBasePath(); then in this case 'url.site' context should be added in that case. Does that sound right?

Wim Leers’s picture

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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
669 bytes
17.53 KB

All right updating \Drupal\Tests\hal\Functional\EntityResource\EntityTestLabel\EntityTestLabelHalJsonAnonTest::getExpectedCacheContexts to not return 'url.site'. Because 'entity_test_label' entity type doesn't provide a canonical link it currently should expect 'url.site' context.

tedbow’s picture

Status: Needs review » Needs work

The last submitted patch, 87: 2825812-87.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
581 bytes
18.1 KB

... and in the meantime #2843752: EntityResource: Provide comprehensive test coverage for Item entity
got committed.

Same problem except with \Drupal\aggregator\Entity\Item since it doesn't provide a canonical url.

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

Wim Leers’s picture

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?

tedbow’s picture

@Wim Leers yes I think you change in #92 is correct to not break BC but also make it easier to add cacheability metadata.

Wim Leers’s picture

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

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

Wim Leers’s picture

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

#91 and the CR look good!

@wim leers thanks for the full explanation in #91.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/image/src/Normalizer/ImageItemNormalizerTrait.php
    @@ -0,0 +1,46 @@
    +          'url' => file_url_transform_relative($style->buildUrl($uri)),
    

    I think we need to call \Drupal\image\ImageStyleInterface::supportsUri() to ensure that the style supports the image. And only then add the image style. See #2652138: Image styles do not play nicely with SVGs

  2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -150,14 +158,25 @@ protected function renderResponseBody(Request $request, ResourceResponseInterfac
    +        if (!$context->isEmpty()) {
    +          @trigger_error('Implicit cacheability metadata bubbling (onto the global render context) in normalizers is deprecated since Drupal 8.4.0 and will be removed in Drupal 9.0.0. Use the "cacheability" serialization context instead, for explicit cacheability metadata bubbling.', E_USER_DEPRECATED);
    +          $response->addCacheableDependency($context->pop());
    +        }
    

    Nicely done.

Wim Leers’s picture

#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 :)

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
19.15 KB

re #96.1 added call to \Drupal\image\ImageStyleInterface::supportsUri()

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs work

... not going to be popular... but I think we need tests for the if ($style->supportsUri($uri)) {

We should have a .svg uri in the test I guess

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
20.16 KB

... not going to be popular... but I think we need tests for the if ($style->supportsUri($uri)) {

I guess i won't vote you down on mostpopulardrupalcorecommitter.com today ;)

No problem good thing to test for!!!!

Added a dataprovider that uses existing jpg test case and svg test case.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs work

#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/

tedbow’s picture

I think this method might be overly complicated
See: #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

If that seems like good idea we should probably postpone this.

Wim Leers’s picture

Title: ImageItem normalizer should expose all public image style URLs » [PP-1] ImageItem normalizer should expose all public image style URLs
Status: Needs work » Postponed
Related issues: +#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

#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!

Wim Leers’s picture

Title: [PP-1] ImageItem normalizer should expose all public image style URLs » ImageItem normalizer should expose all public image style URLs
Status: Postponed » Needs review
Issue tags: +Needs tests
FileSize
80.94 KB
97.55 KB
6.15 KB

#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:

Wim Leers’s picture

Title: ImageItem normalizer should expose all public image style URLs » ImageItem should have an "image_styles" computed property, to expose all image style URLs
Issue tags: +Needs issue summary update

Status: Needs review » Needs work

The last submitted patch, 107: 2825812-107.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
8.63 KB

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.

Wim Leers’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 110: 2825812-110.patch, failed testing. View results

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.51 KB
11.79 KB

#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.

Berdir’s picture

Hm, \Drupal\image\Entity\ImageStyle/image_style config entity is as codebase as it gets and that definitely uses image styles, not derivates. Almost all code references to derivatives in image.module are related to the allow_insecure_derivatives setting, no idea why that doesn't use derivatives.

I'd say using a non-drupal term could in turn be confusing for anyone who actually knows Drupal :)

Also a bit worried about adding more computed properties. They aren't free and unlike non-computed properties are automatically created (not actually computed/accessed, but created) in the constructor of the FieldItemBase. There is an issue to try and optimize that but I couldn't make it work yet.

Wim Leers’s picture

Title: ImageItem should have an "image_styles" computed property, to expose all image style URLs » ImageItem should have an "derivatives" computed property, to expose all image style URLs
FileSize
2.65 KB
11.79 KB

And this then implements #113.

tedbow’s picture

re #115 @Berdir would know but better than what the performance it would be computed fields but as a counterpoint....

The reason we did #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts was because if we expose extra metadata in through normalizers which this issue originally was going to do then from that issue summary:

You can see how this gets complicated very quickly:

  1. new normalizers, multiplied by all different normalizations in core (and that doesn't include the contrib normalizations yet, such as https://www.drupal.org/project/jsonapi)
  2. API-exposing modules not relying on normalizers, such as https://www.drupal.org/project/graphql, would have to again duplicate all this logic
  3. finally, automatically generating documentation that describes the structure of the generated responses also becomes impossible to automate, because they'd have to hardcode the special cases of every normalizer … which we could do in theory for Drupal core's normalizers, but impossible to know all the contrib/custom normalizers!

I think we determined 3) was big reason that applies here we can't dynamically document our REST API(or JSON API in contrib) if this type of information is added in normalizers.

Status: Needs review » Needs work

The last submitted patch, 116: 2825812-115.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
11.79 KB

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

Wim Leers’s picture

FileSize
862 bytes

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

e0ipso’s picture

I'd say using a non-drupal term could in turn be confusing for anyone who actually knows Drupal :)

I agree with this. However this term is going to be exposed through APIs to non-drupalers (iOS, Roku, React, …). In fact, they will be the main customers of this property. That's why I feel like @Wim Leers accurately describes in:

image_styles is a bad name for this computed property, because it's very Drupal-y. derivatives would be a better name.

That being said, I don't feel strongly about this. I just have a preference.

damiankloip’s picture

This is still looking good overall.

  1. +++ b/core/modules/image/src/Plugin/DataType/ImageStyle.php
    @@ -0,0 +1,54 @@
    + *   id = "image_styles",
    

    Should this just be "image_style"? I know it's mainly being used as a list implementation but this plural naming seems weird in this context.

  2. +++ b/core/modules/image/src/Plugin/DataType/ImageStyle.php
    @@ -0,0 +1,54 @@
    + *   label = @Translation("Image style URL"),
    

    This label should indicate we get more than just a URL maybe.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -555,7 +555,8 @@ public function testGet() {
    +// @todo fix \Drupal\image\Plugin\DataType\ComputedImageStyleList::computeImageStyleMetadata(), see comment 114
    +//      $this->assertSame($expected, $actual);
    

    Is this because EntityResourceTestBase::castToString() needs more smarts? I guess the expected response we assert against thinks all the properties should be strings as it works recursively?

    I wonder if it would help if we could use a MapDataDefinition type implementation instead, so we have actual typed data properties for the properties here. This would then fix the normalization case I think? As sites opting in to the BC behaviour would get all strings I think. Oh... hmm. That would mean we'd have to set the initial values as strings, and rely on casting in the primitive data normalizer...

  4. +++ b/core/modules/serialization/src/Normalizer/ListNormalizer.php
    @@ -25,8 +25,8 @@ class ListNormalizer extends NormalizerBase {
    +    foreach ($object as $key => $value) {
    +      $attributes[$key] = $this->serializer->normalize($value, $format, $context);
    

    Should we modify the ListNormalizerTest slightly to reflect this?

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
2.16 KB
11.89 KB
  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).
Berdir’s picture

+++ b/core/modules/image/src/Plugin/DataType/ComputedImageStyleList.php
@@ -0,0 +1,92 @@
+    foreach (ImageStyle::loadMultiple() as $style) {
+      $this->list[$style->getName()] = $this->createItem($style->getName(), $this->computeImageStyleMetadata($file, $width, $height, $style));
+    }

One thing to consider is that sites tend have a lot of image styles. Especially when using responsive image styles (see below).

I just looked at one example of a site that I know has a lot of image styles and I'm seeing a *pager* on the image style site, in total 61 image styles.

Unconditionally showing all of them really isn't an option IMHO.

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.

The second thing to consider are responsive image styles, but I suppose logic for that will be implemented client-side?

e0ipso’s picture

I just looked at one example of a site that I know has a lot of image styles and I'm seeing a *pager* on the image style site, in total 61 image styles.

Unconditionally showing all of them really isn't an option IMHO.

I strongly +1 this. I tried to explain my reasons in this article. The main objective of https://www.drupal.org/project/consumer_image_styles is to mitigate this problem.

Wim Leers’s picture

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

I think this needs something like the attached patch.

Wim Leers’s picture

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

Wim Leers’s picture

#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.

e0ipso’s picture

Not to dwell on this, but it do think that dozens of image styles is indeed the norm. At least for the sites I am usually involved.

Wim Leers’s picture

If we're not going to expose this by default, then I don't see the point of doing this. If it's there, but so hidden that nobody will discover it (i.e. it's required to read docs first), then we might as well not do it.

Wim Leers’s picture

To be clear: #131 is not a knee-jerk reaction. I'm not mad or annoyed. I just am trying to ensure that we are working on higher impact issues first. And if this is not exposed by default, I don't believe this is high-impact anymore.

dawehner’s picture

Because A) having dozens of image styles is not the norm

Haha, websites are more complex than you imagine. Still, the strategy has been with Drupal core to always expose everything.
One example is to expose formatted text by default. The additional size of the response of adding formatted text easily outweights MANY image styles.

For me at least on the longrun jsonapi / the consumer module provides a better solution, but for the purpose of core itself exposing everything provides a reasonable solution.

Berdir’s picture

Good to know about that issue, but that kind of underlines my point :)

We define a computed property that automatically exposes and generates* all image style URL's and then the follow-up will have to implement a custom serializer to then limit those options again, because we definitely can not somehow pass URL query arguments to a computed property (which has no concept of passing arguments).

So.. why not make it a serializer in the first place that only generates the actually requested** image styles?

It's great that we have a new hammer (ability to expose computed properties) but not every missing information in REST is a nail :)

* note that doing this is not cheap in some cases, for example, when hosting images on S3, the current implementation has to fetch the file, convert it, and then send the generated image back up to S3 *when* the url is built, because it can not create them on-deman.

** Yeah, also not convinced that we should send all by default, I'd also say that any non-trivial site quickly has a dozen or more image styles, you just need 3 responsive image styles to get to 9, then a few thumbnails and so on and you're at a dozen.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.