Order image mappings by breakpoint ID and numeric multiplier

RTBC - Looks great - nice catch!

But needs tests as this is a bug report.

Please add a fail only patch as well.

Usually core committees want to see that …

RTBC - thanks for the failing test and test.

This is good to go now!

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

This broke 10.0.x HEAD, presumably because it uses the old 9.0.0 DB dumps that don't exist anymore. This fail is even shown in #11.

Reverting from both branches. We probably want a patch that uses the 9.3.0 database dumps for both branches. Thanks!

Back to RTBC

Couple of questions:

1. +++ b/core/modules/responsive_image/src/ResponsiveImageConfigUpdater.php
   @@ -0,0 +1,47 @@
   + *   This class is only meant to fix outdated responsive image configuration and
   + *   its methods should not be invoked directly. It will be removed once all the
   + *   deprecated methods have been removed.
   

   Are we realistically able to remove this?
   Is 'deprecated methods' accurate, wouldn't it be 'deprecated config'?
   Should we be triggering a deprecation error so that e.g. install profiles can know that their config is outdated? This would require a second argument, a boolean $emit_deprecation that we'd pass FALSE to in the update hook, and TRUE to from the preSave.

2. +++ b/core/modules/responsive_image/src/ResponsiveImageConfigUpdater.php
   @@ -0,0 +1,47 @@
   +    $changed = FALSE;
   ...
   +      $changed = TRUE;
   ...
   +    return $changed;
   

   supernit, but do we need the variable here?

3. +++ b/core/modules/responsive_image/tests/fixtures/update/responsive_image-order-multipliers-numerically.php
   @@ -0,0 +1,71 @@
   + * Test re-ordering responsive image style multipliers numerically.
   

   nit: This comments doesn't describe the intention of this file, which is a fixture, not a test.

4. +++ b/core/modules/responsive_image/tests/fixtures/update/responsive_image.php
   @@ -0,0 +1,55 @@
   +$existing_updates = array_merge(
   +  $existing_updates,
   +  array_keys(responsive_image_removed_post_updates())
   +);
   

   nit: should we call array_unique here too?

5. +++ b/core/modules/responsive_image/tests/src/Functional/ResponsiveImageOrderMultipliersNumericallyUpdateTest.php
   @@ -0,0 +1,60 @@
   +  public function testEntitySave(): void {
   

   nit: missing a docblock here

RTBC

Are we realistically able to remove this?

Install profiles and modules that ship config need to update their shipped config via new exports every major release to keep up.

However I think @larowlan's correct that we should be triggering deprecations for the shipped config case. Even if it gets caught for profiles, it won't for modules and themes.

ViewsConfigUpdater handles this:

 /**
   * Sets the deprecations enabling status.
   *
   * @param bool $enabled
   *   Whether deprecations should be enabled.
   */
  public function setDeprecationsEnabled($enabled) {
    $this->deprecationsEnabled = $enabled;
  }

See for example processEntityLinkUrlHandler()

Drupal 10 patch is failing tests because it's removing the update - the update needs to be in both branches because no-one can update to 9.4 yet before updating to Drupal 10, and we so far won't prevent people going from 9.3.x to Drupal 10.

I think it would probably be easiest to make the deprecations for removal in Drupal 11, and have exactly the same MR apply to both 10.x and 9.4.x. (turned myself around twice in the MR comments until I realised what exactly was going wrong).

Those changes all look much better to me, moving back to RTBC.

Hiding patches because there's an MR

Didn't mean to change the status, but then reviewed the MR and I think we've got the triggering around the wrong way, unless I'm missing something?

The triggering is wrong in the update hook (it should set deprecations to FALSE), but not in presave() (where the default is for deprecations to be on anyway) - left a comment on the MR and marking needs work for that.

RTBCing that. Just one concern with IDE-specific usually useless asserts added here and there that'd probably need a discussion but I'm not going to let it block the issue.

Can we get a 10.0.x MR here too so we can test on both branches?

Thanks

OK, cool, I confirm that pacth #47 also works with D9.4.0 after an "drush updb".

Had a chat with xjm and she agreed at this stage, this issue would be 10.1 only now because of the update hook and new deprecations

So can we get this re-rolled with an updated depreciation notice that references 10.1 instead of 9.4

Then we can get this into 10.1 ASAP

This looks good. See there was already a tests-only patch sweet!

FYI though it was hard to follow between switching back n forth between patches and MR.

Couple of unresolved threads on the MR

Needs updating to use the 9.4.0 database dumps.

To add more the debate, calling \Drupal::service('breakpoint.manager') _already_ happens in calculateDependencies. If we want to remove that DI failure, I think we need to open a follow-up to see how we can untangle the required dependencies between breakpoints and responsive images.

If we're already calling the breakpoint manager, then the current patch isn't really making things worse, so I think we can proceed here once there's a follow-up to look into the interdependency - no idea how to resolve that cleanly tbh.

We are using patch #47 on drupal 9.4.2, PHP version 8.0.21 together with the issue https://www.drupal.org/project/drupal/issues/3192234

It applies cleanly and solves the issue in our project.

Apologise, I changed the version by mistake

+1 RTBC
It works well on my projects.
I'm using the patch from #47 + #191 from https://www.drupal.org/project/drupal/issues/3192234

Committed 4c87cc0 and pushed to 10.1.x. Thanks!

Let's open a follow-up for the dependency injection cross-dependencies.

Updated the branch/version info and published the CR.

Since the original commits on this issue were also committed to Drupal 9, is there a chance we could still get #47 committed to 9.5.x due to this being a bug please?

Discussed #71 in slack with @Dave Reid, but here's my comment for posterity

a) it adds new deprecations which we avoid in bugfix releases (minor only)
b) it adds a new update hook, which we also avoid in bugfix releases

See https://www.drupal.org/about/core/policies/core-change-policies/allowed-... and https://www.drupal.org/about/core/policies/core-change-policies/allowed-... for the sections in the 'allowed changes' regarding those two policies

The same was detailed above in #50 too