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.
Files: 
CommentFileSizeAuthor
#24 2383165.24.patch6.84 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,008 pass(es). View
#24 23-24-interdiff.txt6.32 KBalexpott
#23 2383165.23.patch4.25 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,836 pass(es). View
#23 20-23-interdiff.txt4.86 KBalexpott
#20 interdiff-2383165-17-20.txt846 bytessidharrell
#20 responsiveimagestyle-2383165-20.patch3.18 KBsidharrell
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,820 pass(es). View
#17 responsiveimagestyle-2383165-17.patch3.12 KBsidharrell
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,818 pass(es), 0 fail(s), and 62 exception(s). View
#17 interdiff-2383165-15-17.txt880 bytessidharrell
#15 interdiff-2383165-13-15.txt858 bytessidharrell
#15 responsiveimagestyle-2383165-15.patch3.11 KBsidharrell
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,723 pass(es), 1 fail(s), and 62 exception(s). View
#13 responsiveimagestyle-2383165-13.patch3.1 KBsidharrell
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,726 pass(es), 1 fail(s), and 62 exception(s). View
#13 interdiff-2383165-11-13.txt828 bytessidharrell
#11 interdiff-2383165-8-11.txt1.53 KBsidharrell
#11 responsiveimagestyle-2383165-11.patch3.08 KBsidharrell
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,703 pass(es), 2 fail(s), and 62 exception(s). View
#8 responsiveimagestyle-2383165-8.patch3.05 KBsidharrell
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#8 interdiff-2383165-1-8.txt5.28 KBsidharrell
#1 2383165.1.patch3.07 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2383165.1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
3.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2383165.1.patch. Unable to apply patch. See the log in the details link for more information. View

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

Title: ResponsiveImageMapping config entities should depend in the image styles they use » ResponsiveImageMapping config entities should depend on the image styles they use

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

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

Jelle_S’s picture

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

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

FileSize
5.28 KB
3.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

reroll of #1

sidharrell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

sidharrell’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,703 pass(es), 2 fail(s), and 62 exception(s). View
1.53 KB

method names changed.

Status: Needs review » Needs work

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

sidharrell’s picture

Status: Needs work » Needs review
FileSize
828 bytes
3.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,726 pass(es), 1 fail(s), and 62 exception(s). View

hmmm.

Status: Needs review » Needs work

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

sidharrell’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,723 pass(es), 1 fail(s), and 62 exception(s). View
858 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

Status: Needs work » Needs review
FileSize
880 bytes
3.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,818 pass(es), 0 fail(s), and 62 exception(s). View

grrr.

Status: Needs review » Needs work

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

alexpott’s picture

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

Status: Needs work » Needs review
FileSize
3.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,820 pass(es). View
846 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

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

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

Status: Needs work » Needs review
FileSize
4.86 KB
4.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,836 pass(es). View

Added dependencies for both types of image style mappings.

alexpott’s picture

FileSize
6.32 KB
6.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,008 pass(es). View

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

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

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.