ResponsivelmageMapping config entities depend on the image styles they use but their is not dependency declared. There should be.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because a configuration dependency is missing.
Issue priority Not marking critical since whilst configuration will change there is no schema change and it can easily be fixed by re-saving the mapping. Major because correct dependencies ensure that Drupal works.
CommentFileSizeAuthor
#24 2383165.24.patch6.84 KBalexpott
#24 23-24-interdiff.txt6.32 KBalexpott
#23 2383165.23.patch4.25 KBalexpott
#23 20-23-interdiff.txt4.86 KBalexpott
#20 interdiff-2383165-17-20.txt846 bytessidharrell
#20 responsiveimagestyle-2383165-20.patch3.18 KBsidharrell
#17 responsiveimagestyle-2383165-17.patch3.12 KBsidharrell
#17 interdiff-2383165-15-17.txt880 bytessidharrell
#15 interdiff-2383165-13-15.txt858 bytessidharrell
#15 responsiveimagestyle-2383165-15.patch3.11 KBsidharrell
#13 responsiveimagestyle-2383165-13.patch3.1 KBsidharrell
#13 interdiff-2383165-11-13.txt828 bytessidharrell
#11 interdiff-2383165-8-11.txt1.53 KBsidharrell
#11 responsiveimagestyle-2383165-11.patch3.08 KBsidharrell
#8 responsiveimagestyle-2383165-8.patch3.05 KBsidharrell
#8 interdiff-2383165-1-8.txt5.28 KBsidharrell
#1 2383165.1.patch3.07 KBalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2383165.1.patch3.07 KB

Patch with unit test.

This is also not critical because we do not provide any responsive image mapping default configuration in core.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: ResponsiveImageMapping config entities should depend in the image styles they use » ResponsiveImageMapping config entities should depend on the image styles they use

Noticed this problem while reviewing #2260061: Responsive image module does not support sizes/picture polyfill 2.2.

Wim Leers queued 1: 2383165.1.patch for re-testing.

Jelle_S queued 1: 2383165.1.patch for re-testing.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Title: ResponsiveImageMapping config entities should depend on the image styles they use » ResponsiveImageStyle config entities should depend on the image styles they use
Status: Needs review » Needs work
Issue tags: +Needs reroll
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Novice, +php-novice

Yeah, the patch cannot possibly pass, because the config entity has been renamed. Should be a simple reroll though.

The last submitted patch, 1: 2383165.1.patch, failed testing.

sidharrell’s picture

sidharrell commented
StatusFileSize
new interdiff-2383165-1-8.txt5.28 KB
new responsiveimagestyle-2383165-8.patch3.05 KB

reroll of #1

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: responsiveimagestyle-2383165-8.patch, failed testing.

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review
StatusFileSize
new responsiveimagestyle-2383165-11.patch3.08 KB
new interdiff-2383165-8-11.txt1.53 KB

method names changed.

Status: Needs review » Needs work

The last submitted patch, 11: responsiveimagestyle-2383165-11.patch, failed testing.

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2383165-11-13.txt828 bytes
new responsiveimagestyle-2383165-13.patch3.1 KB

hmmm.

Status: Needs review » Needs work

The last submitted patch, 13: responsiveimagestyle-2383165-13.patch, failed testing.

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review
StatusFileSize
new responsiveimagestyle-2383165-15.patch3.11 KB
new interdiff-2383165-13-15.txt858 bytes

oh, no, no, no.
You do not get to win this fight, test failures!
I get to win!

Status: Needs review » Needs work

The last submitted patch, 15: responsiveimagestyle-2383165-15.patch, failed testing.

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2383165-15-17.txt880 bytes
new responsiveimagestyle-2383165-17.patch3.12 KB

grrr.

Status: Needs review » Needs work

The last submitted patch, 17: responsiveimagestyle-2383165-17.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
  /**
   * The image style mappings.
   *
   * Each image style mapping array contains the following keys:
   *   - image_mapping_type: Either 'image_style' or 'sizes'.
   *   - image_mapping:
   *     - If image_mapping_type is 'image_style', the image style ID (a
   *       string).
   *     - If image_mapping_type is 'sizes', an array with following keys:
   *       - sizes: The value for the 'sizes' attribute.
   *       - sizes_image_styles: The image styles to use for the 'srcset'
   *         attribute.
   *   - breakpoint_id: The breakpoint ID for this image style mapping.
   *   - multiplier: The multiplier for this image style mapping.
   *
   * @var array
   */
  protected $image_style_mappings = array();

You need to account for the differences when image_mapping_type is equal to sizes. Plus the test needs to test this too.

sidharrell’s picture

sidharrell commented
Status: Needs work » Needs review
StatusFileSize
new responsiveimagestyle-2383165-20.patch3.18 KB
new interdiff-2383165-17-20.txt846 bytes

this made it pass locally. It should get a green from testbot.
@alexpott you mean something like this in the unit test:
$entity->addImageStyleMapping('test_breakpoint', '1x', array('sizes'=>array('sizes'=>'??', 'sizes_image_styles'=>'??')));

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

@sidharrell we need to add dependencies for image styles listed in sizes_image_styles if the image_mapping_type is sizes.

sidharrell’s picture

sidharrell commented
Issue tags: -Needs reroll

@alexpott that might be a little beyond me at this point. I don't really understand what the code is doing, I just picked it up for the "Novice" "Needs Reroll" tags.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 20-23-interdiff.txt4.86 KB
new 2383165.23.patch4.25 KB

Added dependencies for both types of image style mappings.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 23-24-interdiff.txt6.32 KB
new 2383165.24.patch6.84 KB

Let's use loadMultiple because then we can't load a non existing image style and it's more performant. ANd we can refactor some similar code in the field formatter.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

This patch looks awesome! Only found the tiniest of nitpicks, which can be fixed on commit.

  1. +++ b/core/modules/responsive_image/src/Entity/ResponsiveImageStyle.php
@@ -184,6 +185,11 @@ public function calculateDependencies() {
+    // Extract all the styles from the image style mappings.
+    $styles = ImageStyle::loadMultiple($this->getImageStyleIds());
+    array_walk($styles, function ($style) {
+      $this->addDependency('config', $style->getConfigDependencyName());
+    });

    Wow, very elegant! :) 

  2. +++ b/core/modules/responsive_image/src/ResponsiveImageStyleInterface.php
@@ -126,4 +126,11 @@ public function addImageStyleMapping($breakpoint_id, $multiplier, array $image_s
+   * @return array

    Can be string[], can it not?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Changed to string[], and also added a missing space on one of the switch statements.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1389f05 on 
    Issue #2383165 by sidharrell, alexpott: ResponsiveImageStyle config...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.