Split revision-handling methods to a separate entity storage interface

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
   @@ -72,27 +72,6 @@ public function load($id);
   -  public function loadRevision($revision_id);
   ...
   -  public function deleteRevision($revision_id);
   

   I think this change is fine in practice, but it could theoretically be problematic: if you have code relying on EntityStorageInterface::loadRevision() and an implementation of the "updated" EntityStorageInterface not defining it is passed to it, your code breaks.

2. +++ b/core/lib/Drupal/Core/Entity/RevisionableEntityStorageInterface.php
   @@ -0,0 +1,31 @@
   +interface RevisionableEntityStorageInterface {
   

   Shouldn't this extend EntityStorageInterface?

We would need a new base class for the new interface as well, otherwise we'll not be allowed to add new methods to it according to our BC policy. Or should we flag the new interface @internal and somehow point to ContentEntityStorageBase as the base class of it?

It should not extend EntityStorageInterface IMO, to allow for cleaner composition. A previous example of this is \Drupal\Core\Entity\RevisionableInterface which doesn't extend EntityInterface

Why isn't it called RevisionableStorageInterface then?

@amateescu, oh well if this issue is not targeted against D 8.x then everything is fine :). Probably we should mention this in the issue title or summary? :)

Looks good, thanks

This is blocking a string of critical and major issues.

The only problem is that once we introduce the new interface we cannot add methods to it anymore. Could we probably declare it internal until the interface is complete?

I'm fine with marking it as @internal until we are done with #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision and #1730874: Add support for loading multiple revisions at once, but this should not be strictly needed:

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
[...] 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.

So even if we don't have a 1:1 relationship between the new interface and the base class we should be covered by the policy.

Thank you :).

I think it is worth mentioning that there is one more issue that will add a new method related to revisionable entities - loadRevisionUnchanged in #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load().

Hm, isn't the problem with those @deprecated that now all calls to \Drupal::entityTypeManager()->getStorage()->loadRevision() will show as deprecated, even though that's not actually the case.

I guess that also shows the downside of this, that \Drupal::entityTypeManager()->getStorage()->loadRevision() and other methods will in the future no longer autocomplete...

Also, I don't see how adding @internal to an interface that is implemented by a non-internal interface (ContentEntityStorageInterface) makes any sense :)

Also, I don't see how adding @internal to an interface that is implemented by a non-internal interface (ContentEntityStorageInterface) makes any sense :)

ContentEntityStorageInterface could inherit anything and we are allowed to add methods to it, because it has a 1:1 relationship to a base class.
It makes sense to declare the new interface internal so that custom or contrib don't implement it directly and we still could add methods to it.

Is this actually blocking the criticals and majors? It seems like it could just as easily be a followup/cleanup, no?

I don't think we can deprecate the old methods while also marking their replacements internal. Either the deprecations should be in a followup or we shouldn't mark the interface internal.

I also strongly dislike marking interfaces internal generally because it gives the developer conflicting signals. Sometimes it makes sense but we should avoid it if it's not really needed.

Separately, about raising deprecations. In the issue that does the deprecation (whether this or another) can we raise trigger_error() in implementations that use the method but don't implement the new interface? Does that make sense? Edit: I may not have totally thought this through but I try to ask each time.

@hchonov:

As mentioned in #17, we are allowed to add methods to interfaces even if they don't have a 1:1 relationship with a base class. If adding the @internal tag is problematic, I'd say let's just get rid of that tag and move on.

@xjm:

Is this actually blocking the criticals and majors? It seems like it could just as easily be a followup/cleanup, no?

It's introducing the RevisionableStorageInterface that's required by the major/critical tasks mentioned above. Moving the methods around could definitely be a non-critical clean-up issue.

@amateescu:

Based on the feedback above, especially #21 (which I verified), I'd say let's remove the deprecations and open a postponed follow-up to deprecate/remove the methods from EntityStorageInterface before releasing the last 8.x minor.

@amateescu:

Could we add the trigger_error() calls to the ConfigEntityStorage class?

After quickly discussing this with @amateescu in IRC, we agreed that since there's no actual deprecation, we cannot trigger errors.

We also agreed to release this joint statement:

Cna we just haz intaface, pleeze?

LOL how can I deny #28?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -135,13 +135,6 @@ public function loadRevision($revision_id) {
    * {@inheritdoc}
    */
-  public function loadMultipleRevisions(array $revision_ids) {
-    return [];
-  }

+++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
@@ -133,13 +133,6 @@ public function loadRevision($revision_id) {
   /**
    * {@inheritdoc}
    */
-  public function loadMultipleRevisions(array $revision_ids) {
-    return [];
-  }

So it looks like these empty implementations were just added in the other issue, and the idea is that they were only added because they were required for the base class non-revisionable implementations when they were on the other interface, and we hadn't yet split off this interface. Is that accurate?

Hmm, okay, not sure about doing that. It's docs gate so we shouldn't really postpone it. We can say in the CR that the interface is still being extended and that we'll add to it later.

I just looked at #2926958: Remove revision-related methods from EntityStorageInterface and it's filed as being for 9.x. But we should actually add a @deprecated in 8.x (right?). Because anyone not extending a core base class still needs that information, and that's also what we'll be grepping for when we go to open 9.x. Sorry if there was some confusion from #24; trying to figure out how we could trigger_error() for interface methods doesn't need to be in scope here. (We have the same issue for constants.) But the notice itself can follow the standard form.

Sorry, sad-eyed kitty, I do think it needs to be NW. :) Hopefully not too painful though since it looks like we're in CI limbo ATM anyway.

Quickly discussed #33 with Jess, we are ok with leaving the @todos and opening a follow-up to update our policies to address cases like this one.

@amateescu:

+++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
@@ -133,13 +133,6 @@ public function loadRevision($revision_id) {
-  public function loadMultipleRevisions(array $revision_ids) {

We need to move this to KeyValueContentEntityStorage, which should fix those failures.

I'm going to take care of the CR and the follow-up.

EDIT: I think it's fine to leave the CR in draft status until we're done with these refactorings, more or less what Hristo was suggesting above with the @internal tag.

Follow-up to discuss policy changes at #2927251: [policy, no patch] Decide how to deal with methods needing to be moved down an interface hierarchy.

Draft CR at https://www.drupal.org/node/2927226.

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
   @@ -79,20 +79,13 @@ public function loadUnchanged($id);
   +   * @see https://www.drupal.org/node/2926958
   

   Can we add a second @see to the CR? So that it's still sorta like other deprecations even though we're using an @todo.

2. +++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
   @@ -0,0 +1,43 @@
   +   *   The revision id.
   

   Nit: ID unless we're talking about Freudian psychology. ;)

Maybe we were making this too complicated...

catch [1:57 PM]
@xjm why can’t we remove the method declarations from the parent given it extends?

NM, we're deprecating the method on the other parent, but it still raises warnings in IDEs for the child.

Patch diff is also a little confusing; looks like part might have been staged and the other part unstaged.

NM, missed that there are two KV imlementations.

Yay

OK I think this is sufficient given the followups and that it's blocking other things. Committed and pushed to 8.5.x, and published the CR. Thanks!