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
Comment | File | Size | Author |
---|---|---|---|
#14 | 3226487-14.patch | 7.63 KB | acbramley |
Issue fork drupal-3226487
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:
Comments
Comment #2
AaronMcHaleThe 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:
See #3043321-120: Use generic access API for node and media revision UI which relates to "all the reasons I previously stated".
Comment #3
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedI'm working on this today - Closed #3226588: Allow revision view access for node and media when there is only one revision as a duplicate.
Comment #4
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedI've done a cursory run of Kernel/Functional tests and nothing else seems broken - let's see if I missed much.
Comment #6
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedInteresting... the line that's failing has this comment above it:
Does that mean the checkbox wasn't controlling this tab's access afterall and it was still dependent on the number of revisions?
Comment #7
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedThe above failing line in the test was added here https://www.drupal.org/project/drupal/issues/808730#comment-12630122
Comment #8
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedConfirmed 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 :)
Comment #9
bbralaSorry clicked make issue fork while scrolling.
Comment #10
BerdirI 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..
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.
Comment #11
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedOh 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 :)
Hopefully!
Honestly, me too XD
Comment #12
daffie CreditAttribution: daffie commentedNitpick: I think that the "or" from the second line can go on the end of the first line.
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.countDefaultLanguageRevisions()
any more, we should deprecate it.Comment #13
Berdir> 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.
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.
Comment #14
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFixed #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!
Comment #15
daffie CreditAttribution: daffie commentedWe 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.
Comment #16
BerdirWe'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.
Comment #17
Berdircopied 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"? ;)
Comment #18
bbralaComment #19
AaronMcHaleYeah 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.
Comment #20
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFixes #17
Comment #21
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedCR drafted https://www.drupal.org/node/3228833
Comment #22
mstrelan CreditAttribution: mstrelan at PreviousNext commentedThis passes without the patch. May need something like this:
Comment #23
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented😅 I'd prefer to just use patch #14 then...
Please ignore patch #20
Comment #24
darvanenWould 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:
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.
Comment #25
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented@darvanen I've fixed the CR. Feel free to make any additional edits yourself.
Comment #26
darvanenAmazing, 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.
Comment #28
catchCommitted 6fb71f0 and pushed to 9.3.x. Thanks!
Also published the CR.
Comment #29
bbralaNice :D