Problem/Motivation

Thunder installation fails on Drupal 10.3 because it has an override of image.style.thumbnail that is not properly applied. This occurs because #3406267: All core shipped image styles should include webp conversion has added an extra image effect. This results in the plugin collection being instantiated when the profile configuration is applied during the profile module install. This uncovers the fact that \Drupal\Core\Plugin\DefaultLazyPluginCollection::setInstanceConfiguration() assumes that the existing instance and the incoming $configuration are for the same plugin. But this is not necessarily the case. Core's configuration is:

  1cfec298-8620-4749-b100-ccb6c4500779:
    uuid: 1cfec298-8620-4749-b100-ccb6c4500779
    id: image_scale
    weight: 0
    data:
      width: 100
      height: 100
      upscale: false
  c4eb9942-2c9e-4a81-949f-6161a44b6559:
    uuid: c4eb9942-2c9e-4a81-949f-6161a44b6559
    id: image_convert
    weight: 2
    data:
      extension: webp

while Thunder's is:

  1cfec298-8620-4749-b100-ccb6c4500779:
    uuid: 1cfec298-8620-4749-b100-ccb6c4500779
    id: focal_point_scale_and_crop
    weight: 0
    data:
      width: 100
      height: 100
      crop_type: focal_point

When core only has a single effect (ie. 10.2.x) Thunder's overrides were successfully applied because the effect plugin instances are not create when you change the entity. But no there is more that one they are because we need to sort them. This result in applying the configuration to the wrong plugin type - and everything breaks.

Steps to reproduce

Try to run a thunder test on Drupal 10.3.x

Proposed resolution

The simple solution is to change \Drupal\Core\Plugin\DefaultLazyPluginCollection::setInstanceConfiguration() to unset the plugin instance prior to getting it. But that would go against the docs:

   * If there is no plugin instance yet, a new will be instantiated. Otherwise,
   * the existing instance is updated with the new configuration.

Maybe we can detect somehow that the configuration is for a different plugin type and then unset.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3445950

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

I've filed this issue against 10.3.x because it occurs due to the 10.2 to 10.3 upgrade but it is also present in 11.x

alexpott’s picture

Status: Active » Needs review
tim.plunkett’s picture

I'm not able to run the tests-only job for some reason. But when I do locally, I get

Failed asserting that Double\ConfigurableInterface\P1 Object (...) is not an instance of interface "Drupal\Component\Plugin\ConfigurableInterface".

Which sounds like a Prophecy mixup, not an actual working assertion.

@alexpott am I overthinking it? The change looks good

catch’s picture

I think there are permissions issues on the test only job when the MR was opened by a core committer.

alexpott’s picture

@tim.plunkett thanks for the review - it's the expected failure because we're doing:

    $configurable_plugin = $this->prophesize(ConfigurableInterface::class);
    $nonconfigurable_plugin = $this->prophesize(PluginInspectionInterface::class);

So these are Prophecy classes that implement the respective interface.

tim.plunkett’s picture

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

@catch that would explain it!

@alexpott I would have thought the `->reveal()` calls would have helped, but fair enough

longwave’s picture

Status: Reviewed & tested by the community » Needs work

Comment nit, otherwise LGTM.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Nice catch @longeave given it was just a nit back to rtbc.

longwave’s picture

Status: Reviewed & tested by the community » Fixed

This is eligible for backport to 10.2.x under the bugfix policy but given it has only arisen in 10.3.x not sure it's worth it. Feel free to reopen for backport if this is incorrect.

Committed and pushed 926d069c2c to 11.x and 55463c9ef0 to 11.0.x and b7c0549a3c to 10.4.x and ef095f9bd2 to 10.3.x. Thanks!

  • longwave committed ef095f9b on 10.3.x
    Issue #3445950 by alexpott, tim.plunkett: \Drupal\Core\Plugin\...

  • longwave committed b7c0549a on 10.4.x
    Issue #3445950 by alexpott, tim.plunkett: \Drupal\Core\Plugin\...

  • longwave committed 55463c9e on 11.0.x
    Issue #3445950 by alexpott, tim.plunkett: \Drupal\Core\Plugin\...

  • longwave committed 926d069c on 11.x
    Issue #3445950 by alexpott, tim.plunkett: \Drupal\Core\Plugin\...
longwave’s picture

Status: Fixed » Closed (fixed)

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