Problem/Motivation

Both node and media modules have a conditional access check where the revisions tab only shows if there is more than one revision (if revisions aren't created by default), as well as restricting access to the current revision if it's the only one.

The check was partially removed in #808730: Show the Revisions tab/page even when only one revision exists. (when revisions are enabled by default), but the remaning conditional check is causing issues for REST as well as the generic revision UI.

Steps to reproduce

Proposed resolution

Remove the conditional access check that counts revisions, always show the tab/revisions page.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3226487

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

catch created an issue. See original summary.

AaronMcHale’s picture

The approach that is proposed received UX sign off at #3225198: Drupal Usability Meeting 2021-07-30, where it was discussed in the context of #3043321: Use generic access API for node and media revision UI; The proposed resolution here covers exactly what was signed off, so we're all good to proceed from a UX perspective.

Comment #3043321-123: Use generic access API for node and media revision UI:

Conveniently, this week's Usability meeting has just taken place, which gave me the opportunity to get wider feedback on whether the revision UI should be conditional on whether there is more than one revision.

After discussing the problem and my thoughts at #3225198: Drupal Usability Meeting 2021-07-30, there was unanimous agreement that the revision UI should not be conditional on the number of revisions. For all the reasons I previously stated, but an interesting point around training users and documentation was mentioned, where it is easier if the tab was not conditional.

This means we now have sound footing for that approach going forward.

Thanks,
-Aaron

See #3043321-120: Use generic access API for node and media revision UI which relates to "all the reasons I previously stated".

acbramley’s picture

Assigned: Unassigned » acbramley
acbramley’s picture

Status: Active » Needs review
FileSize
3.84 KB

I've done a cursory run of Kernel/Functional tests and nothing else seems broken - let's see if I missed much.

Status: Needs review » Needs work

The last submitted patch, 4: 3226487-4.patch, failed testing. View results

acbramley’s picture

Interesting... the line that's failing has this comment above it:

// Verify that no link to revisions is displayed since the type
// has the 'create new revision' setting unset.

Does that mean the checkbox wasn't controlling this tab's access afterall and it was still dependent on the number of revisions?

acbramley’s picture

The above failing line in the test was added here https://www.drupal.org/project/drupal/issues/808730#comment-12630122

acbramley’s picture

Status: Needs work » Needs review
FileSize
4.77 KB
777 bytes

Confirmed via manual testing that we can remove these lines in the test as they weren't actually testing the Revisions tab in relation to the checkbox. Even without removing the number of revisions access check, the revisions tab will display when the "Create new revision by default" setting is turned off.

This also "fixes" a caching bug where if a Node had 2 revisions and then you deleted one, the Revisions tab would still show but would return a 403 when clicked :)

bbrala’s picture

Sorry clicked make issue fork while scrolling.

Berdir’s picture

  1. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
    @@ -108,14 +108,7 @@ public function checkAccess(MediaInterface $media, AccountInterface $account, $o
    -      if ($media->isDefaultRevision() && ($this->countDefaultLanguageRevisions($media) == 1)) {
    

    I guess nothing is left that would use countDefaultLanguageRevisions() ? I guess, since we plan to deprecate the whole class, we can probably avoid deprecating the method here and just ignore that it still exists ;)

    I really thought the default revision checks would disallow viewing the revision page of the default revision entirely, but apparently that was coupled to the revision count, which seems weird. I suppose that's because we use the same access check and operation for both revision list and a single revision and things got combined together..

  2. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
    @@ -127,12 +127,9 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
    +        // If this is the default revision, return access denied for revert
    +        // or delete operations.
    +        if ($node->isDefaultRevision() && ($op === 'update' || $op === 'delete')) {
               $this->access[$cid] = FALSE;
             }
    

    from this limited context, it's not clear if media has the same protection for update/delete, the existing line for this didn't do any operation checks, but I suppose at least in core we don't have UI's to delete/revert revisions so it doesn't matter? The generalization will then take care of this I suppose.

Looks pretty good, but I expected more test to adjust/update, will double check with the changes from the issue that did the recent changes.

acbramley’s picture

I guess, since we plan to deprecate the whole class, we can probably avoid deprecating the method here and just ignore that it still exists ;)

Oh yeah I was meant to ask about that! Sounds like a good plan to just defer to #3043321: Use generic access API for node and media revision UI :)

but I suppose at least in core we don't have UI's to delete/revert revisions so it doesn't matter

Hopefully!

but I expected more test to adjust/update

Honestly, me too XD

daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
    @@ -127,12 +127,9 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
    +        // If this is the default revision, return access denied for revert
    +        // or delete operations.
    

    Nitpick: I think that the "or" from the second line can go on the end of the first line.

  2. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiBypassAccessTest.php
    @@ -107,11 +107,6 @@ public function testDisplayRevisionTab() {
    -    $this->assertSession()->addressEquals($node->toUrl());
    -    // Verify that no link to revisions is displayed since the type
    -    // has the 'create new revision' setting unset.
    -    $this->assertSession()->linkNotExists('Revisions');
    

    I think we should update the comment and change the linkNotExists() to linkExists(). The comment should say why. Also, I think that we should add a test for the page $this->drupalGet('node/' . $node->id() . '/revisions'); and do some assertions to links that should and not should be there. Can we than also a some documentation with why some links are there and others are not.

  3. As we are not going to use the method countDefaultLanguageRevisions() any more, we should deprecate it.
Berdir’s picture

> As we are not going to use the method countDefaultLanguageRevisions() any more, we should deprecate it.

Per #10/#11, I think it is OK to avoid that extra work of deprecating it as this is a intermediate step and we will deprecate the whole class in #3043321: Use generic access API for node and media revision UI.

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiBypassAccessTest.php
@@ -107,11 +107,6 @@ public function testDisplayRevisionTab() {
 
-    $this->assertSession()->addressEquals($node->toUrl());
-    // Verify that no link to revisions is displayed since the type
-    // has the 'create new revision' setting unset.
-    $this->assertSession()->linkNotExists('Revisions');
-
     // Verify the checkbox is unchecked on the node edit form.

Checking the previous issue again. The whole section from line ~89 to the end was added _specifically_ to test the changed behavior of the revisions tab if the create new revision by default checkbox is _not_ enabled.

First, I think there is a lot more code that we can remove from \Drupal\node\Access\NodeRevisionAccessCheck::checkAccess(). It _should_ be fine to just entirely remove the logic around $bundle_entity->shouldCreateNewRevision(). The revision count check was a fallback *if* that setting was off. If we remove the fallback, then it should not make any difference anymore with these changes. I would expect we can just drop that if entirely, we do maybe still need to consider the view operation somehow.

And then with that code gone, I think it would be OK to remove that entire section of that test, because we're just repeating the existing test coverage then with the settings change that we know has no impact anymore on it. I don't think we need to have test coverage for something that was only historically relevant.

acbramley’s picture

Status: Needs work » Needs review
FileSize
7.63 KB
4.25 KB

Fixed #12.1

Ran most of the NodeRevision* tests locally and you're right @Berdir, by the looks of it we don't need this check at all. I was already sceptical about keeping the rest of that chunk of tests so I agree we can remove it altogether.

Let's see if anything else breaks!

daffie’s picture

Status: Needs review » Needs work

We are doing a lot more changes in core/modules/node/src/Access/NodeRevisionAccessCheck.php then in the previous patch. Can we add testing for those changes.

@Berdir: Thank you for your explanation.

Berdir’s picture

Status: Needs work » Needs review

We're mostly just removing code. I don't see what we would test additionally here. I'll try to do a final review here asap.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php
@@ -145,6 +145,11 @@ protected function setUp(): void {
+    // Access to the revision page for a node with 1 revision is allowed.
+    $node = $this->drupalCreateNode();
+    $this->drupalGet("node/" . $node->id() . "/revisions/" . $node->getRevisionId() . "/view");
+    $this->assertSession()->statusCodeEquals(200);
+

copied from media I assume, we could use $node->toUrl('revision') instead, but not so important I think.

Wanted to RTBC, but I think we need a change record that explains this change. And cross-reference with https://www.drupal.org/node/3001224. "Revisions tab is now ALWAYS visible with one draft"? ;)

bbrala’s picture

AaronMcHale’s picture

copied from media I assume, we could use $node->toUrl('revision') instead, but not so important I think.

Yeah I'd agree, using toUrl would be much better, good to follow our best practice here and avoids the chance of that ever causing a problem.

acbramley’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
801 bytes

Fixes #17

acbramley’s picture

mstrelan’s picture

Status: Needs review » Needs work
// Access to the revision page for a node with 1 revision is allowed.
$node = $this->drupalCreateNode();
$this->drupalGet($node->toUrl('revision'));
$this->assertSession()->statusCodeEquals(200);

This passes without the patch. May need something like this:

$this->assertSession()->addressEquals("node/" . $node->id() . "/revisions/" . $node->getRevisionId() . "/view");
acbramley’s picture

Status: Needs work » Needs review

😅 I'd prefer to just use patch #14 then...

Please ignore patch #20

darvanen’s picture

Status: Needs review » Needs work

Would RTBC #14 based on Berdir's comments and my own reading of the patch. However I have a tiny bit of feedback on the CR draft.

This paragraph:

Before #3226487: Always allow access to revisions based on access/permissions, not on the count of revisions when the node type enables new revisions by default the revision tab will be visible, even when there is only one revision. When the node type does not have revisions checked by default, it will fall back to only display if there was more than 1 revision.

describes a previous state in present tense, which confused me as I read it - even having the full context of this issue freshly in mind. I would like to see it use past tense, and perhaps have another go and getting the point across more clearly.

acbramley’s picture

Status: Needs work » Needs review

@darvanen I've fixed the CR. Feel free to make any additional edits yourself.

darvanen’s picture

Status: Needs review » Reviewed & tested by the community

Amazing, much clearer, thank you.

Based on the updated CR, the conversations in this issue, the code in patch and the passing tests, #14 is RTBC. Per #23, please ignore #20.

  • catch committed 6fb71f0 on 9.3.x
    Issue #3226487 by acbramley, bbrala, Berdir, AaronMcHale, darvanen,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6fb71f0 and pushed to 9.3.x. Thanks!

Also published the CR.

bbrala’s picture

Nice :D

Status: Fixed » Closed (fixed)

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