Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce:
- Create a new node node
- Edit the node, check the [new revision] checkbox
- Expected result: The revisions tab is shown, actual result: The revision tab is hidden
Problem is: \Drupal\node\Access\NodeRevisionAccessCheck::checkAccess
does not take into account cacheability metadata
Comments
Comment #2
Wim LeersHere's a start.
Comment #3
dawehnerI'll write some tests, its pointless IMHO to try to fix that without a test.
Comment #4
dawehnerWell its easy to reproduce, but for some reason the tests works, see patch.
Comment #6
Wim LeersDrupalCI seems drunk. Re-testing.
Comment #7
dawehnerSounds like a full disk for something.
Comment #8
Wim LeersSo #4 being green… means … that other cache tags are being invalidated also, which then masks the problem.
This tests the local tasks block, which also has other local tasks that have the node's cache tag, and so if any of the other local tasks' cacheability metadata (or their access checks' cacheability metadata) causes an invalidation, it doesn't allow us to see the problem here in isolation.
Therefore I think we need to write a separate test, that tests some output that depends on this access check in isolation.
Comment #9
dawehnerWell, to be clear, the problem was the following bit in the code:
and well, this should be basically invalidated by the node list tag, but well, I don't see how this is ever added to the block.
Comment #10
Wim LeersPer @catch: #808730: Show the Revisions tab/page even when only one revision exists. is related.
Comment #11
matsbla CreditAttribution: matsbla commentedI created an issue here #2611188: Revision tab is only displayed when translation is enabled for content type, but maybe it is the same as this one, so I just cross-post what I observed when testing:
The revision tab is only displayed when translation for the content type is enabled. In fact, if you create a node before you enable translation for the content type, the revision tab will not be displayed for that node even after you turn on content type translation. It also seem like at least one extra language needs to be enabled to get the revision tab; I enabled translation for content type without adding any languages, then the tab was not displayed until I enabled a second language and made a new revision. However, for the node created before the translation for the content type was enabled, the revision tab did not display, even after enabling a second language and making a new revision...
Comment #12
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedThe problem with the non-failing test is with caching metadata for specific permissions. With the bypass permission we stop the bubbling of the metadata up, so this should make the tests fail. Check for node_node_access() in node.module for more clues and discuss with @berdir about it.
I'm providing the complete patch, the failing test and the interdiff of the changes in the test.
Comment #13
BerdirThat works but then we also lose test coverage of the specific permissions and that those are enough to do this.
I'd add a duplicate test method with a user that has bypass permission and limit the test steps to the minimum to reproduce this.
Comment #15
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedHere is the updated patch with the new test.
Comment #17
BerdirThis should document that it is more or less the same as the existing test except that we are testing with a user with bypass access permissions. Maybe we could also do it as a test method of the other one
The name should then reflect this too if we keep a class or in the method if we do it as a method of the existing one.
NodeRevisionsUiBypassAccessTest or something similar for the method.
Description is too long.
Comment #18
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRenamed the test and fixed the comments as suggested.
Comment #19
BerdirSuggested edit:
This test is similar to NodeRevisionsUITest except that it uses a user with the bypass node access permission to make sure that the revision access check adds correct cacheability metadata.
Comment #20
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commented:)
Comment #22
mbayntonLooks like the test was rerun successfully?
Comment #23
lauriiiMissing comment
s/Create users./Create user.
I don't think we should use t() here
Unnecessary line change
Comment #24
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedDone the small changes. I'm just not sure about which empty line you think of... :)
Comment #25
cilefen CreditAttribution: cilefen commentedComment #26
BerdirLooks fine to me, lets fix this.
Comment #27
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!