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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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
CreditAttribution: Jelle_S 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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 CreditAttribution: sidharrell commented
FileSize
interdiff-2383165-1-8.txt5.28 KB
responsiveimagestyle-2383165-8.patch3.05 KB

reroll of #1

sidharrell’s picture

sidharrell CreditAttribution: 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 CreditAttribution: sidharrell commented
Status: Needs work » Needs review
FileSize
responsiveimagestyle-2383165-11.patch3.08 KB
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 CreditAttribution: sidharrell commented
Status: Needs work » Needs review
FileSize
interdiff-2383165-11-13.txt828 bytes
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 CreditAttribution: sidharrell commented
Status: Needs work » Needs review
FileSize
responsiveimagestyle-2383165-15.patch3.11 KB
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 CreditAttribution: sidharrell commented
Status: Needs work » Needs review
FileSize
interdiff-2383165-15-17.txt880 bytes
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 🇪🇺🌍
CreditAttribution: alexpott 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 CreditAttribution: sidharrell commented
Status: Needs work » Needs review
FileSize
responsiveimagestyle-2383165-20.patch3.18 KB
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 🇪🇺🌍
CreditAttribution: alexpott 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 CreditAttribution: 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 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
20-23-interdiff.txt4.86 KB
2383165.23.patch4.25 KB

Added dependencies for both types of image style mappings.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
23-24-interdiff.txt6.32 KB
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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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 🇨🇦
CreditAttribution: webchick 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.