NodeController::revisionOverview() does not have a pager, which results in unlimited queries

Here is the patch adding limit and offset on revisionIds function of NodeStorage class

Changing to Needs review so test bot can trigger testing.

a) We change the public interface, this should not happen
b) We construct a SQL query using strings, let's switch to db_select() instead
c) IMHO we should just skip using this method and go to use an entity query in the first place.

In addition to @dawehner's review issues in #5, the patch provided does not implement a pager for the revisions UI (NodeController::revisionOverview()), so doesn't get us close enough to the needed solution. But it's a start!

If I understand (c) in #5, @dawehner is saying "just dump this method and use an entity query directly in NodeController::revisionOverview()", which would work fine IMO.

If I understand (c) in #5, @dawehner is saying "just dump this method and use an entity query directly in NodeController::revisionOverview()", which would work fine IMO.

Yeah exactly. https://github.com/fago/entity/blob/8.x-1.x/src/Controller/RevisionContr... has an example of that already.

@dawehner
I have changed query for initial testing, does this sounds good ?
Updated code from fresh 8.2.x so no interdiff added.

@dawehner was correct in #5.c), we cannot change \Drupal\node\NodeStorage::revisionIds() in any way because that would be an API change, as proven by the test fails in #8, so let's skip using the node storage method and just write our own entity query.

Do we want to test the new pager behavior for revisions overview page?

Also, no API change means that this could go into 8.1.x as well?

Yeah let's add a test for it. Beside this is looking fine. Maybe for readability reasons this could be moved to a protected method.

Ok then, here's a test. Not attaching a test-only patch because it's testing new behavior introduced by this issue.

Oops, forgot about moving the query to a protected method. Attaching a second interdiff for that :)

+++ b/core/modules/node/src/Tests/NodeRevisionsAllTest.php
@@ -64,6 +56,33 @@ protected function setUp() {
+    $node->setNewRevision();
+    $node->save();
+
+    $node_storage->resetCache(array($node->id()));
+
+    // Make sure we get revision information.
+    return $node_storage->load($node->id());

Wait, this resetCache call is really needed? This sounds like a bug, don't you think so? Seems so out of scope of this issue to discuss about that. If needed thought loadUnchanged() does the same kind of stuff.

Turns out most of that is not needed at all.

Nice

Discussed with @alexpott, @catch, @xjm, @dixon, @berdir, @effulgentsia, @timmillwood - we decided to keep it as major. Also setting back to RTBC after a random DrupalCI error.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -170,12 +171,9 @@ public function revisionOverview(NodeInterface $node) {
   -    foreach (array_reverse($vids) as $vid) {
   +    foreach ($this->getRevisionIds($node, $node_storage) as $vid) {
   

   At first I was unsure why the array_reverse() was going away here, but now I see. NodeStorage::revisionIds() loads the full list, ascending. The new method in the patch selects them in descending order. Edit: Point being, this is correct.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -279,4 +279,25 @@ public function addPageTitle(NodeTypeInterface $node_type) {
   +  protected function getRevisionIds(NodeInterface $node, NodeStorageInterface $node_storage) {
   

   This adds a protected method to a controller, which is a safe internal API change. Since this is a major-at-least bug, we also should backport the fix to 8.1.x. In that case would we add an underscore to the beginning of its name? That's the strategy we've used before and is documented on our API policy: https://www.drupal.org/core/d8-bc-policy#underscore

3. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -279,4 +279,25 @@ public function addPageTitle(NodeTypeInterface $node_type) {
   +      ->pager(50)
   

   I questioned whether this should be hardcoded, but I see it is already hardcoded this way for users and comments, so this seems acceptable for a major (and increasingly critical) bug fix.

Unfortunately, the patch does not seem to apply to 8.2.x. Can we reroll it for that branch, then look at the backport? (It's most critical in 8.2.x where all nodes are already revisioned by default, although the bug will still also cause fatals on 8.1.x sites where revisioning is enabled for large content types.)

Re-roll for Drupal 8.2.x
Fixed one more issue and changed order to Ascending.

@mohit_aghera, thanks for the reroll.

I did not mean that the order should be ascending. Descending is correct. array_reverse() of ascending is descending. :) I will edit my comment to make that clearer; I was just documenting questions I had while reviewing and what the answers were.

Also, be sure to provide an interdiff when you update patches. In this case though, let's undo the change to ascending that broke the patch. :) Thanks!

Rerolled for 8.2.x without any changes.

And for 8.1.x with an underscore for the new method. @xjm, thanks for pointing out this underscore policy for BC, I didn't know about it :)

Tests are passing, not sure why failure was indicated. Back to RTBC.

Thanks @amateescu!

I also tested this manually and confirmed that the revision page pager works as I would expect. (Had a revert-war with myself, since devel_generate seems not to allow generating revisions!)

Committed a0fc0b0 and pushed to 8.2.x and 58de4e7 to 8.1.x. Thanks!