Problem/Motivation

NodeRevisionDeleteForm::submitForm runs a check to decide whether to redirect the user to the node page(if not remaining revisions) or the revision history page. This check, however, is a hardwired SQL query which not only fails with alternate storage engines but also if the route access check is changed.

Proposed resolution

Replace SQL query with a node storage revisions count.

Use the route access check, it just makes sense not to redirect to a page which ends up access denied.

Note that at least one test executes the changed code, core/modules/file/tests/src/Functional/FileFieldRevisionTest.php

Remaining tasks

Review

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Problem/Motivation

N/A

CommentFileSizeAuthor
#32 interdiff_30_32.txt1.24 KBjidrone
#32 2470489-32.patch7.05 KBjidrone
#30 interdiff_25_30.txt2.04 KBjidrone
#30 2470489-30.patch5.66 KBjidrone
#25 interdiff_16_25.txt5.02 KBjidrone
#25 2470489-25.patch5.35 KBjidrone
#16 reroll_diff_2470489_1-16.txt5.18 KBankithashetty
#16 2470489-16.patch2.95 KBankithashetty
#1 2470489_1.patch3.31 KBchx
accesscheck.patch3.35 KBchx

Comments

chx’s picture

chx commented
StatusFileSize
new 2470489_1.patch3.31 KB

My first attempt was path validator which didnt work out but the patch contained the use command. Removed unused interfaces.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

I think we'll end up spinning off the individual fixes from #2471613: FormState:setRedirect() should check access to the route, consider this the first one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: 2470489_1.patch, failed testing.

Status: Needs work » Needs review

tim.plunkett queued 1: 2470489_1.patch for re-testing.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Bot fail.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: NodeRevisionDeleteForm::submitForm uses the wrong check » NodeRevisionDeleteForm::submitForm uses the wrong check for setting a redirect
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Good find. The read part of that diff made me make a "yeaghhh" noise.

I get that the hardwired check should work the same on the default storage engine, but what about the bit where it doesn't work if route access is changed? That sounds like a bug we should be able to add test coverage for.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Version: 8.9.x-dev » 9.2.x-dev
Issue tags: +Needs reroll, +Novice, +Bug Smash Initiative

This needs a reroll and it is suitable for a novice.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2470489-16.patch2.95 KB
new reroll_diff_2470489_1-16.txt5.18 KB

Rerolled the patch in #1, thanks!

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.

quietone’s picture

quietone commented
Issue summary: View changes

Updating IS

quietone’s picture

quietone commented
Issue tags: -Novice

Removing Novice tag because writing the test is not a first time task.

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

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.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: NodeRevisionDeleteForm::submitForm uses the wrong check for setting a redirect » NodeRevisionDeleteForm::submitForm() uses the wrong check for setting a redirect
jidrone’s picture

jidrone commented
Title: NodeRevisionDeleteForm::submitForm() uses the wrong check for setting a redirect » NodeRevisionDeleteForm::submitForm() has a SQL query and is not checking access on redirect
Issue summary: View changes
StatusFileSize
new 2470489-25.patch5.35 KB
new interdiff_16_25.txt5.02 KB

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.

jidrone’s picture

jidrone commented
Status: Needs work » Needs review

I forgot to move it to NR.

smustgrave’s picture

smustgrave commented

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

jidrone’s picture

jidrone commented 
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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests

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.

jidrone’s picture

jidrone commented
Status: Needs work » Needs review
StatusFileSize
new 2470489-30.patch5.66 KB
new interdiff_25_30.txt2.04 KB

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks!

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

jidrone’s picture

jidrone commented
Status: Needs work » Needs review
StatusFileSize
new 2470489-32.patch7.05 KB
new interdiff_30_32.txt1.24 KB

Added deprecation test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

  • longwave committed 6028a1ae on 10.1.x 
    Issue #2470489 by jidrone, chx, ankithashetty, smustgrave, quietone, xjm...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

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';
     }

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published the change record.