NodeRevisionDeleteForm::submitForm() has a SQL query and is not checking access on redirect

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Moving to NW for the tests

Did not test or review yet.

After a deeper review I think the condition mentioned on this issue was misunderstood, because the intension is to know if there are existing revisions and redirect the user accordingly, if existing revisions go to the revisions history page, if not go to the node page.

It is also true that is needed to check if the user has access to the revisions history page, so we can take this opportunity to fix both things:

• Check if user has access to revisions history page.
• Replace the hardwired SQL query.

I improved the patch to remove the database service as well, because it is not used anymore for this test.

I'm improving the issue description and title.

I forgot to move it to NR.

Shouldn't the old parameter be deprecated vs replaced? Could this cause existing sites to now break

if ($this->connection->query('SELECT COUNT(DISTINCT [vid]) FROM {node_field_revision} WHERE [nid] = :nid', [':nid' => $this->revision->id()])->fetchField() > 1) {

The problem is only with this line, so I think the way to get the existing revisions to redirect the user can be just changed to use the nodeStorage:

$this->nodeStorage->revisionIds($this->revision)

As I did in the patch.

Removing tests as they appear to be added.

I think the old parameter should be deprecated. If someone is overriding this form it could break for them I believe.

Will need a simple deprecation test once that's added.

You are right, we need to deprecate the connection argument, I did so and also I started a draft change record.

Thanks!

Will also need a simple test case checking for the deprecation.

Added deprecation test.

Thanks for sticking with this. Think this is good for a committer to take a look now.

Committed and pushed to 10.1.x, thanks!

Fixed up grammar in some comments on commit:

--- a/core/modules/node/src/Form/NodeRevisionDeleteForm.php
+++ b/core/modules/node/src/Form/NodeRevisionDeleteForm.php
@@ -152,11 +152,11 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
         '@type' => $node_type,
         '%title' => $this->revision->label(),
       ]));
-    // Set redirect to revisions history.
+    // Set redirect to the revisions history page.
     $route_name = 'entity.node.version_history';
     $parameters = ['node' => $this->revision->id()];
-    // If not revisions found or user has not the access to revisions page,
-    // then redirect to node canonical page.
+    // If no revisions found, or the user does not have access to the revisions
+    // page, then redirect to the canonical node page instead.
     if (!$this->accessManager->checkNamedRoute($route_name, $parameters) || count($this->nodeStorage->revisionIds($this->revision)) === 1) {
       $route_name = 'entity.node.canonical';
     }

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

Published the change record.