Problem/Motivation

#2926540: Split revision-handling methods to a separate entity storage interface introduced a specialized interface for revisionable storage handlers and duplicated the loadRevision() and deleteRevision() methods from EntityStorageInterface because those methods could not be deprecated during the 8.x.x cycle (see #2927251: [policy, no patch] Decide how to deal with methods needing to be moved down an interface hierarchy).

Proposed resolution

Remove those two methods in the last minor version of Drupal 8.

Remaining tasks

Wait for the 9.0.x branch to be created :)

User interface changes

Nope.

API changes

Revision-related methods will no longer be part of the generic EntityStorageInterface.

Data model changes

Nope.

Issue fork drupal-2926958

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

plach’s picture

Wim Leers’s picture

because those methods could not be deprecated during the 8.x.x cycle.

Why?

Wim Leers’s picture

Apparently the answer to my question is given in the first paragraph of the issue summary of the issue plach linked 😀👍

plach’s picture

Issue summary: View changes

Yep, sorry, updated the IS to reflect that.

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

andypost’s picture

Status: Postponed » Needs review
FileSize
1.45 KB

Let's see if something affected

andypost’s picture

Also removed unused implementations

The last submitted patch, 7: 2926958-7.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: 2926958-8.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
6.54 KB

Fix tests, not sure the change is right
maybe \Drupal\Core\Entity\ContentEntityStorageInterface is better choice

andypost’s picture

FileSize
2.47 KB
9.02 KB

Bit more fixes

The last submitted patch, 11: 2926958-11.patch, failed testing. View results

andypost’s picture

As it pass now, needs review for test coverage changes mostly

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This looks fine to me. If we decide to remove these methods then this is does what needs to be done. The related policy issue was never resolved, but I'm not sure what exactly we could do more here. The only thing seems to be to add explicit @trigger_error() to the implementations that would go away in the future like ConfigEntityStorage, but there never were any reasons to call them in the first place, and it wouldn't have helped with the mocks in the tests that we need to fix here.

alexpott’s picture

So the interesting thing about this is that it messes with the typehints from things like this:
$entity = $this->entityTypeManager->getStorage('block_content')->loadRevision($this->configuration['block_revision_id']);
currently IDEs etc can resolve this to \Drupal\Core\Entity\EntityStorageInterface::loadRevision - with this change it can't be resolved.

Berdir’s picture

That's a good point. we could document getStorage() that it may return any of the generic storage interfaces to improve that?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think something needs to be done about #16 and #17 seems like the best solution.

xjm’s picture

Version: 9.0.x-dev » 9.1.x-dev

Hmm, this slipped through the cracks; we should had made it a child of the deprecation removal meta.

I debated whether this should be moved to 9.1.x now. On the one hand, the docblock does tell them that it's deprecated, and removing a method from an interface is mostly BC aside from this edgecase:

Ideally we would remove these methods and none would notice, however this is theoretically a BC break because 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.

On the other hand, though, that edgecase does exist, and the @todo doesn't tell the person in any detectable way.

Could we try a more "proper" deprecation with something like what @plach suggested on the meta? That is:

A possibility to avoid the deprecation in any 8.x.x branch and still warn contrib module authors preparing for D9, could be to add trigger_error() calls to the NULL implementations. This way (theoretical) API consumers relying on the deprecated definitions would actually be warned and could adapt and switch to a different implementation.

Can we do that in 9.1, and then deprecate the methods for D10?

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje’s picture

Copying a few (relevant) comments over from #3213895: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 10 branch:

@Spokje in #3213895-51:

...
@todo Deprecated in Drupal 8.5.0
Note the very unorthodox use of @todo. Is this code "officially" deprecated?

https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/lib/Drupal/C...

@catch in #3213895-52:

8.5 might be from before we could actually deprecate things properly in core (deprecation tests etc.)

With something that old, we'd want a 9.5 issue to make the deprecation proper, if that shows there are no core usages, then maybe check for contrib usages, then we can decide if they should be for removal in 10.x or 11.x depending on how deprecated they really are. But either way better to fix the docs/trigger_error() in 9.5.x

Spokje’s picture

FileSize
6.4 KB

First stab at proper deprecation in 9.5.0.

Spokje’s picture

Status: Needs work » Needs review
Spokje’s picture

Issue tags: +Deprecation Removal
longwave’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -122,6 +122,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+    @trigger_error('ConfigEntityStorage::loadRevision() is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Entity\RevisionableStorageInterface instead. See https://www.drupal.org/node/1', E_USER_DEPRECATED);

I think each of the implementations should just say "There is no replacement." as it makes no sense to load revisions of config that is not revisionable in the first place, each of these methods just returned NULL.

To be clear the interface should still say "Use RevisionableStorageInterface instead" as that is the correct thing to do there.

Ratan Priya’s picture

Assigned: Unassigned » Ratan Priya
Ratan Priya’s picture

Assigned: Ratan Priya » Unassigned
Status: Needs work » Needs review
FileSize
6.42 KB
37.22 KB

@longwave,

I made the changes you required at comments #28

Needs review.

Status: Needs review » Needs work

The last submitted patch, 30: 2926958-30.patch, failed testing. View results

catch’s picture

MeenakshiG’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
6.88 KB

Updated the patch as mentioned in comments #28.
Added interdiff as well

Spokje’s picture

Status: Needs review » Needs work

There are a couple of references to https://www.drupal.org/node/1 which seems to be a placeholder for a new Change Record.

If that's the case, we need that CR to be created so we can link to it.

-   * @see https://www.drupal.org/node/2926958
-   * @see https://www.drupal.org/node/2927226
+   * @see https://www.drupal.org/node/1
+    @trigger_error('KeyValueEntityStorage::loadRevision() is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/1', E_USER_DEPRECATED);
MeenakshiG’s picture

Issue tags: +Needs change record
Berdir’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -122,6 +122,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    */
   public function loadRevision($revision_id) {
+    @trigger_error('ConfigEntityStorage::loadRevision() is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/1', E_USER_DEPRECATED);
+
     return NULL;
   }

I don't think this can still be deprecated for 10.0 which is soon in beta. It's too late and not important enough for an exception.

Move it to 10.1/11.0 and then is much more likely to land.

ravi.shankar’s picture

I've made changes as per comment #33, please review. Still needs work for change record.

quietone’s picture

@ravi.shankar, remember to check for other work that needs to be done. In #35 there is a request to update the link to the CR.

The draft change record for this issue was added in July 2022. Removing tag for a change record and adding the tag for change record updates.

This still needs work for #35 and the version in the issue meta data doesn't match the deprecation message.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje’s picture

Updated CR for 10.1.x

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
@@ -80,11 +80,10 @@ public function loadUnchanged($id);
-   * @todo Deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
-   *   Use \Drupal\Core\Entity\RevisionableStorageInterface instead.
+   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use
+   * \Drupal\Core\Entity\RevisionableStorageInterface instead.
...
-   * @see https://www.drupal.org/node/2926958
-   * @see https://www.drupal.org/node/2927226
+   * @see https://www.drupal.org/node/1

not sure removal of existing links is useful

Spokje’s picture

Switch to MR workflow

andypost’s picture

Added feedback to MR, mostly nits

Spokje’s picture

Status: Needs work » Needs review

Thanks @andypost, resolved all your issues.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, should be ready

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3a9e58f and pushed to 10.1.x. Thanks!

andypost’s picture

Looks not pushed

  • alexpott committed 3a9e58f on 10.1.x
    Issue #2926958 by Spokje, andypost, Ratan Priya, MeenakshiG, ravi....

Status: Fixed » Closed (fixed)

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