ImageStyleStorage should extend ConfigEntityStorageInterface

First, if anything, that interface should extend ConfigEntityStorageInterface, not EntityStorageInterface, but there are other issues.

Changing that interface to extend EntityStorageInterface is a BC break. Any code implementing that interface as it stands would break if you added methods by extending it. Additionally, implementing interfaces multiple times through inherited classes and interfaces is wonky at best. It's probably okay in this case, but I try to avoid it. ImageStyleStorage already implements EntityStorageInterface by extending ConfigEntityStorage, so it's unnecessary to also implement it through the ImageStyleStorageInterface (yes, I know we do do this with other storage interfaces, like I said, in this particular case it works, but only because both implementation are inherited. If you tried to directly implement EntityStorageInterface in ImageStyleStorage you would get an 'unable to implement already implemented interface" error.

Can you not just mock the class directly, which implements both interfaces, rather than trying to mock one interface or the other? Or make a test interface in your test file that extends both so you have a mockable interface to use?

Any chances to see this in Drupal 9 (with ConfigEntityStorageInterface of course)?
I need a way to discover all storages that extends ConfigEntityStorageInterface.

I understand the explanations from #3.

It is just a little tricky from a programmer perspective with an IDE to work with the ImageStyleStorage, because we have to type-hint to ConfigEntityStorageInterface to get the (config) entity storage methods.

Maybe, at least, we can change the interface comment to tell the programmer that classes which implement ImageStyleStorageInterface must also implement ConfigEntityStorageInterface?

From:

/**
 * Interface for storage controller for "image style" configuration entities.
 */

to:

/**
 * Interface for storage controller for "image style" configuration entities.
 *
 * Classes implementing this interface are also expected to implement
 * Drupal\Core\Config\Entity\ConfigEntityStorageInterface.
 */

Patch against 8.9.x

Please check #3 which explains why this change may introduce backwards compatibility issues: classes inheriting ImageStyleStorageInterface will have to implement the methods from the extended interface. Moreover, @mikelutz suggests using ConfigEntityStorageInterface instead of EntityStorageInterface.

@Dakwamine #3 has been fixed in this patch

@Dakwamine Sorry i was missed #6 now this has been fixed in this patch both #3 and #6 fixed in this patch.

I'm agree that \Drupal\image\ImageStyleStorageInterface should extend \Drupal\Core\Config\Entity\ConfigEntityStorageInterface.

In the current state if you typehint image style storage with this interface the IDE suggest only three methods from it. But it actually doesn't true, since it also implements ConfigEntityStorageInterface indirectly. This is done that way for other entities, so it must extend as well for consistency.

The issue with adding extends ConfigEntityStorageInterface is that any classes implementing ImageStyleStorageInterface will require to implement ConfigEntityStorageInterface methods.

The only class in core which implements ImageStyleStorageInterface is ImageStyleStorage.

ImageStyleStorage inherits ConfigEntityStorage and implements ImageStyleStorageInterface.

Any custom or contrib module can decide to implement ImageStyleStorageInterface in a new class to replace the default service.

By changing the ImageStyleStorageInterface hierarchy, we break those modules.

An alternative would be to provide better typehints on the properties:

/**
 * Image style storage.
 *
 * @var \Drupal\image\ImageStyleStorageInterface|\Drupal\Core\Config\Entity\ConfigEntityStorageInterface
 */
protected $imageStyleStorage;

But the real issue is in core. It wrongly expects ImageStyleStorageInterface to extend ConfigEntityStorageInterface.

The backwards compatible way to handle the issue raised by the original post is to use either ConfigEntityStorageInterface or ImageStyleStorageInterface in the ImageFormatter and MediaThumbnailFormatter constructors, then, test if the service is an instance of the other interface before calling its methods.

/**
 * Image style storage.
 *
 * @var \Drupal\Core\Config\Entity\ConfigEntityStorageInterface
 */
protected $imageStyleStorage;

public function __construct(....., ConfigEntityStorageInterface $image_style_storage) {
  ...
  $this->imageStyleStorage = $image_style_storage;
}

public function xyz() {
  ...
  $blabla = $this->imageStyleStorage->load($id);
  ...
  if ($this->imageStyleStorage instanceof \Drupal\image\ImageStyleStorageInterface) {
    $this->imageStyleStorage->getReplacementId(...);
  }
}

The definitive way to handle this would be to break the compatibility. Is this desirable? Can image style storages NOT be config entity storages? Well, I cannot decide this on my own. ;)

In a reversal from apparently 3 years ago me, I do think this is a bug and we should seriously consider fixing this.
Per the backwards compatibility policy here

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

• Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
• Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

We are able to add to interfaces in minor releases, particularly in this case where there is a 1:1 base class in core using the interface that custom classes should be extending, as ImageStyleStorageInterface is NOT marked @api. If we could do it in D10.0 and have the break fall on a major release, that would be even better, IMHO. Regardless the final decision on this would fall to a Release Manager, so I propose we see if one can comment here and tell us what they would be willing to do about this, and go from there.

As a note, just looking through code usages of ConfigEntityStorage, I see four classes that extend ConfigEntityStorage while implementing an additional custom storage interface.

• ShortcutSetStorageInterface
• VocabularyStorageInterface
• RoleStorageInterface
• ImageStyleStorageInterface

Of those, all except ImageStyleStorageInterface DO extend ConfigEntityStorageInterface.

Additionally all 8 custom ContentEntityStorageInterfaces I see (Feed, Item, Comment, File, MenuLing, Node, Term and User) all extend ContentEntityStorageInterface as well.

So it seems to me like ImageStyleStorageInterface is the odd one out, and it definitely should extend ConfigEntityStorageInterface. I was not able to find another custom storage interface in core that DIDN'T extend it's respective EntityStorageInterface.

Just talked to @catch in slack and got this:

catch 2 minutes ago
@mikelutz (he/him) I think this is allowable in 9.4 under the 1-1 rule.

I'm going to RTBC this and leave the RM review tag on, and see what happens.

Agreed with #21 that this is covered by the 1-1 rule.

Because it's minor-eligibile, it should go into 9.4 as well as 10.0.x, we try to keep 10.0.x-only changes to things that really can can only be changed like a major.

Agree with the change, I think we don't even need to specially document that, it's a comment that explains the change that this issue is doing, not the code as it will then be (or it does, but it's also very self-explanatory).

The patch in #16 is also still being tested against 9.1 and is failing there. Based on my comment above, unsure if I should set to needs work, or reupload the patch and keep it at RTBC.

Yes, the comment change is not needed if the interface is designed in a self-explanatory fashion (fixing the interface inheritance is).

@Berdir, did you mean the patch #15?

Yes, I did (I think I even looked at #14, that's the one that is currently being re-tested, #15 is hidden). Which is why I suggested to re-upload the correct patch, which would in fact be the hidden #15 as we don't need an extra comment.

Oh, right.

The interface now extends ConfigEntityStorageInterface, without the extra useless comment.

Let's wait for the test pass...

While the comment is useless, I suppose we should really put in a change record, since there is some potential disruption here...

Added a change record

Actually, since I wrote the CR, someone else should review and re-RTBC.

Yes, back to RTBC with you, tests should pass unless we get more of these lovely random JS test fails.

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

+1

I published the CR. Thanks all!