Deprecate NodeStorageInterface::revisionIds in favor of entity query

+++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
@@ -65,4 +65,17 @@ public function deleteRevision($revision_id);
+   * @return int[]
+   *   Entity revision IDs.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1692,4 +1692,18 @@ protected function storageDefinitionIsDeleted(FieldStorageDefinitionInterface $s
+    return NULL;

@return docs don't match the possible NULL return.

Would an empty array here not be better DX, so you always have something iterable?

This is will help JSON API achieve solid support for the content_moderation module.

+++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
@@ -60,4 +61,11 @@ public function getLatestTranslationAffectedRevisionId($entity_id, $langcode) {
+  public function revisionIds(EntityInterface $entity, $order = 'ASC') {
+    return NULL;
+  }

Shouldn't this also be []?

One last thing, I hope:

1. +++ b/core/modules/node/src/NodeStorageInterface.php
   @@ -11,17 +11,6 @@
   -   *   Node revision IDs (in ascending order).
   

   We've lost the bit about them being in ascending order.

   We need to keep that aspect to preserve BC.

2. +++ b/core/lib/Drupal/Core/Entity/RevisionableStorageInterface.php
   @@ -65,4 +65,17 @@ public function deleteRevision($revision_id);
   +   *   Entity revision IDs.
   

   So this should read something like:

   > An array of the entity revision IDs, in ascending order.

> An array of the entity revision IDs, in ascending order.

This isn't strictly true as the $order argument defines this, but why do we even need the $order argument anyway? Is there really a use case for returning them in reverse order? The caller can just use array_reverse(), can't they?

> This isn't strictly true as the $order argument defines this

Oh DUH you're right. So the Node docs are actually incorrect, and the new docs are better.

> but why do we even need the $order argument anyway? Is there really a use case for returning them in reverse order? The caller can just use array_reverse(), can't they?

Also a good point, but we need to keep it for BC and deprecating it should be a separate issue.

Though isn't it quicker to order in SQL than PHP?

Based on that I'm going to say this is RTBC.

The $order argument is new in this patch. NodeStorageInterface::revisionIds() always returns in ascending order (which is why the previous comment exists). It was added in #2 but with no real reason given, but to me this is technically a new feature and out of scope here, at least until we have a use for it.

I'm looking to get revision ids from entities other than nodes. Is there still interest in this patch? If so, it would have to be re-rolled and the comments in #16 need to be addressed.

To address #16, do we just need to remove the $order argument that was added? Or is it more complicated than that?

> just need to remove the $order argument that was added

Yup.

And copy the bit from the node docs about the @return being in ascending order.

Added reroll of patch #11 on Drupal 9.4.x.

Thanks! That was fast on the re-roll! I updated the re-roll in #25 to remove the $order argument and add back in the docs about ascending order.

I tested this manually. For my use case, I created a custom views field that uses revisionIds to get the previous content moderation state for nodes, blocks, and media entities.

• Cloned a clean 9.4.x-dev site.
• Added patch.
• Enabled modules: workflows, content_moderation, media, media_library.
• Configured default workflow to apply to All custom block types, All media types, and All content types /admin/config/workflow/workflows/manage/editorial.
• Added custom blocks of type Basic block, media of type Image, and content of type Basic in various states of content moderation.
• Created views--Custom Block revisions, Media revision, and Content revisions--added my custom views field.

All 3 entity types worked as expected: I was able to use revisionIds to get a list of revision IDs for the block_content, media, and nodes.

LGTM.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityNullStorage.php
@@ -147,4 +147,11 @@ public function hasData() {
 
+  /**
+   * {@inheritdoc}
+   */
+  public function revisionIds(EntityInterface $entity) {
+    return [];
+  }

Since this will be new code on the interface, could we add a return type hint?

Also looks like we still need some explicit test coverage here.

What do we need for "explicit test coverage"? Do we need another function like testPendingRevisions(), but for blocks (and/or other entity types?)? Or should testPending Revisions be more generic so it covers other revisionable entities?

@capysara there should be some existing test coverage of SqlContentEntityStorage to add to, Drupal\KernelTests\Core\Entity\EntityRevisionsTest is probably a good place.

Why do we need this method, can't this be done with an entity query?

Yes, exactly, like that. Although I remember some issues with that in entity reference revisions.

But the revision ui issue also doesn't seem to need this.

Deprecating without direct replacement is definitely an option. This method likely predates entity query revision support

Right, the issue I had was this: #2766135: EntityQuery with condition on the revision field leads to wrong results, this has been fixed. I'm sure there are other issues, one of the most important ones is entity query performance, but I think for this, using an entity query should be OK. I'd suggest we verify that by deprecating the method and replacing all calls to it with such an entity query.

The search doesn't seem to work too well (getting a lot of endpoint requested too many times errors).

But almost all of those seem to be 1:1 copies of NodeStorage, and many callers (http://grep.xnddx.ru/search?text=-%3ErevisionIds%28) are 1:1 copies of the revision page controller.

And all custom methods won't be affected by deprecating the node storage method.

I'd say if those results prove one thing, then that it would be good to get #2350939: Implement a generic revision UI into core, with a generic revision UI, most of that code wouldn't need to exist at all.

FWIW, I'm using this patch to replace some code like this, which (I believe) was borrowed from Media revisions UI.

function getNonNodeEntityRevisionIds($entity) {
  $entityTypeManager = \Drupal::entityTypeManager();
  $entityStorage = $entityTypeManager->getStorage($entity->getEntityTypeId());
  $id_column = ($entity->getEntityTypeId() == 'media') ? 'mid' : 'id';
  $revision_column = ($entity->getEntityTypeId() == 'media') ? 'vid' : 'revision_id';
  $result = $entityStorage->getQuery()
    ->allRevisions()
    ->condition($id_column, $entity->id())
    ->sort($revision_column, 'DESC')
    ->pager(50)
    ->execute();

  return array_keys($result);
}

I would be happy to see this patch move forward (and was planning to try to write some tests). I would also love to see #2350939: Implement a generic revision UI get into core! :)

Thoughts on the path forward for this one?

Came up in slack and the advice from @berdir was to deprecate instead. I agree as it's minimal code to get a list of revision IDs using an entity query.

This issue was fixed as part of #3396062: Deprecate most methods on NodeStorage. The change record is https://www.drupal.org/node/3519187.