Show the Revisions tab/page even when only one revision exists.

This is a very questionable usability win, I could agree that sometimes it does show up sometimes it doesn't is going to cause some confusion. However the distraction of a somewhat useless tab, when there are no revisions other then the current is just as important. Although I can imagine with large sites or multiple development environments this could be more annoying, still to the user it could cause uneccairy distraction.

Lets see if we can fix this properly in D8.

subscribe (i would love to see this come to fruition, i just found myself scratching my head over why a node lacked a revisions tab, and it took me a few minutes to discover this was the reason.)

Still seems like a good idea to me too, patch rerolled for HEAD. Saving a database query is always helpful.

Patch #7 works.

Showing the tab is not the only use case:
#940242: Node revision link access
- Views has a "Node revisions: ..." view with links as node/$nid/revisions/$vid/view, which doesn't work for nodes with only one revision.
#138454: "Access Denied" on nodes with no revisions
- This issue handles symptoms if modules Diff and Revision Moderation even back in D5 & D6
#792202: Revisions access denied
- This issue handles the symptom in module Content moderation

As Bojhan said

However the distraction of a somewhat useless tab, when there are no revisions other then the current is just as important

Patch #7 now shows the tab for Every node, even if not even one node of that Content Type has a revision. So, for those content types, the Tab 'Revisions' indeed is an overkill.
The Tab only is necessary for entity types for which Revisions are actually used in the site.

Perhaps we need to enhance the "Create new revision" checkbox in the Content type settings:
Change the checkbox to a group of radiobuttons "Create new revision":
- Disabled
- Enabled
or even:
- Disabled
- Enabled, optional, (do not set "Create new revision" to yes on the when editing a node/entity)
- Enabled, optional, (set "Create new revision" to yes on the when editing a node/entity)
- Required (set "Create new revision" to yes - user cannot unset this option)

Or is there a way that you can enter the page from 'outside' and NOT have the tab on the node page when the node only has 1 revision?

Another option is to create a more granular permission per node type:
Another issue handles the addition of new permissions to create a more granular permission per node type:
- < Node type: > View content revisions
- < Node type: > Revert content revisions
- < Node type: > Delete content revisions

#880940: view/revert/delete revisions per content type

The initial patch from #7 shows a Revisions tab also for node types that have no revisions at all, which is not desireable - we need some extra work there to fetch the node type. That requires substantial rework of the function, but will be adequately handled by #880940: view/revert/delete revisions per content type.
Attached patch adds a differentiation between list and view :
- node/%node/revisions : list still only access with +1 revisions (this is the Revisions tab)
- node/%node/revisions/%/view : view now allows access, even with only 1 revision (this is accessed from contrib revision links)
- node/%node/revisions/%/revert : as before: only the revision is not the current one
- node/%node/revisions/%/delete: as before: only the revision is not the current one
The patch also saves a DB-query.

#1439336: _node_revision_access is executing an unnecessary query when $op equals 'delete' or 'update' changes the same line of code, and has extensive testing.

A COUNT() query that is currently badly indexed (at least in D8(, so this would be a performance improvement.

We stumbled upon this issue while porting the diff module. While reviewing the diff port UI, i was confused by the tab behavior.
See also related contrib task: #2424249: Node revision tab visible with only one revision

Starting this from the scratch..
Why we just cant remove the check for multiple revisions in the NodeRevisionAccessCheck::checkAccess() ? since all nodes can have revisions. It's not required to check the box "Create new revisions" when creating node content in order to create a revision later. So can we display the revisions tab regardless of it or do we have to depend on the revisions ID?

Uff... So the problem here is that every content type supports revision.
And if we always show the tab, it also appears although revisions are never created (by default) because the "Create new revision" checkbox is never checked.

What we possibly need is
- a "Support revisions" checkbox instead that enables the revisions tab.
-- Without this checkbox set, no one can create a revision per node.
- a second checkbox "Create new revision (by default)" that controls if revisions are created.
-- Still a user can disable this when creating content. Similar to the current checkbox.

That would be a new feature then IMHO, which isn't going to happen for 8.0.x.

The only alternative that I can think of is that we show it all the time if revisions are enabled by default, and fall back to the query only if it is not, and then show it only if there are revisions, as it will be the exception.

We still have the query then, but only when it is really needed.

we show it all the time if revisions are enabled by default

Yeah that makes sense to avoid the feature problem...
So trying to summarise the facts:

• For creating / updating nodes:
  • A node revision is created if this checkbox "create new revision" is set.
  • The checkbox is only visible if a user has "administer nodes" permission.
  • Unprivileged users will create revisions, based on the content type setting "create new revision"
• Display revision tab...
  • If content type enables revision by default and
    • user has "view all revisions"
    • or user has "view $bundle revisions"
    • or user has "administer nodes"
  • If there is more than one revision stored

Downside is that only if for every single node create/update new revisions are created, we get rid of the count() query pointed out by Berdir.

The proposal above makes the revision tab no more language specific, so there are extra lines to remove in checkAccess.

Yes, I'm not sure what the language check there is supposed to do exactly? The revisions are shown/shared across translations, so showing it or not based on the language doesn't make sense?

Berdir is right, revisions overall are not language specific, so access checking should not depend on language.

Ok, the language stuff is because the same access check is used for viewing a specific revision, which would be shown in a specific language then. That makes sense.

Then we should try something similar as the old patches with, with a list operation for overview, where we don't care about the language...

As discussed with Berdir,
If the revisions checkbox is enabled in content type, then display the revisions tab. unless fall back to the query and checking multiple revisions.

Still applies. And #24 also looks fine to me as a status quo.

This patch also still applies. Does "OK for now" mean we should set it to RTBC?

Patch "show_the_revisions-808730-24.patch" passes testing.
Result: For both article and basic page content type, revisions tab is displayed only when the "Create new revision" checkbox is checked. The basic problem is also addressed that is revisions tab is displayed only if there is one revision. Attaching screenshot.

Tests should be updated to reflect the new behavior, so we can be sure we keep it that way. (Like with every behavioral change).

This patch is not working as expected, When creating a revision and the node at the same time the instance the tab doesnt show up. The create revision button is always present as well. Once you delete a previous revision then the tab is shown even if there is only one revision.

After discussion with @Berdir and @miro_dietiker this patch would create a new issue since if we enforce the revision tab all the time then if the user didn't enable revisions the issue would not be solved therefore we must enforce that revisions are always active, following CRAP. But this might be a feature that will no be needed all the time. Then this might me worth for discussion on 8.1.x in any case.

patch#11 rerolled for HEAD of 7.x

Any news about this bug/feature for Drupal 7?

In my 7.56 installation I see that the tab is hidden when there is only the initial revision (and that's pretty reasonable in my opinion). The problem is that it's been suggested that this behaviour could be the root cause for links to revisions not working in views:

https://www.drupal.org/project/views/issues/940242

Regards

re-rolling for 8.6

fixing some tests

addressing test failures

Views was failing the tests because part of the test suit is to create a view of revisions and test the revision link and revision delete handlers.
I Addressed the views test failures by:

• Link the delete revision to node delete when it's the default revision (original).
• do not render the revert link for default revision: not an ideal but I could not figure out a better solution

Removing "Needs tests" tag b/c I believe that has been addressed.

Code generally looks good to me, and I decided to play around with this a little on simplytest. I created two sites, one with the patch and one without. On both sites, I created a "basic page" with a commit message, published, and my results are below...

On the current 8.6 site, you can see there is no "Revisions" tab listed.

On the patched 8.6 site, you can now see there is a Revisions tab listed when you create the node...

And here is the actual revisions page with the 1 revision:

I believe this patch is accomplishing what the issue summary states, so I'm going to move to RTBC status to get the next round of reviews on the patch.

added views integration tag.

fixed few styling issues and comments.

1. +++ b/core/modules/node/src/Plugin/views/field/RevisionLinkDelete.php
   @@ -20,7 +20,9 @@ class RevisionLinkDelete extends RevisionLink {
   -    return Url::fromRoute('node.revision_delete_confirm', ['node' => $node->id(), 'node_revision' => $node->getRevisionId()]);
   +    return !$node->isDefaultRevision() ?
   +      Url::fromRoute('node.revision_delete_confirm', ['node' => $node->id(), 'node_revision' => $node->getRevisionId()]) :
   +      Url::fromRoute('entity.node.delete_form', ['node' => $node->id()]);
   

   This feels like quite a big change and not entirely related to issue. Why don't we take the same approach as with the revert and not present a link if it's the default revision. Also if this is the correct behaviour then we need to add a test for this.

2. +++ b/core/modules/node/src/Plugin/views/field/RevisionLinkRevert.php
   @@ -20,7 +20,24 @@ class RevisionLinkRevert extends RevisionLink {
   +      // @todo: A link to default view node is not an ideal solution.
   

   We need to link a drupal.org issue to address this. Otherwise the @todo will remain there forever.

3. I think if possible this patch should only make changes that solve the problem of the revision tab being hidden when there is only one revision.

I see. 1 & 2 can be addressed but as for:

I think if possible this patch should only make changes that solve the problem of the revision tab being hidden when there is only one revision.

Tests would fail without addressing the views issue. So I am not sure what the best course of action is. Should we let the tests fail and open another issue to address views integration?

@awm why do the tests fail?

@alexpott part of views test is set up a revisions view and test for the delete and revert link handlers. The view can be found here.
As far as I could tell, the test fail because the delete/revert/view links are printed for the default revision:

node/1/revisions/id/delete
node/1/revisions/id/revert

So without changing the views handlers we end up with a view like:
Screenshot: https://i.imgur.com/Wpu8tHK.png

+++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
@@ -119,22 +119,30 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
-      // not good.
-      if ($node->isDefaultRevision() && ($this->nodeStorage->countDefaultLanguageRevisions($node) == 1 || $op == 'update' || $op == 'delete')) {
-        $this->access[$cid] = FALSE;
-      }

This part is moved into the else of if shouldCreateNewRevision().

That's the problem. update/delete access is never allowed on the default revision. If views is the only thing that is failing then we seem to have insufficient test coverage.

@Berdir, you are right, thank you. This is why views test started failing. I am attaching an updated patch. In it:
1. Added to check if $op == 'view'.
2. I removed views integration added in previous patch.
3. Added testes to verify node/defaulatNodeId/revisions/[revert|delete] are inaccessible.

patch in #69 worked for me on 8.5.3
also working for 8.5.4

putting it in reviewed & tested since it's been tested by #72.

Just some nits here, looking great

1. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -119,22 +119,30 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
   +      if ($bundle_entity->shouldCreateNewRevision() && $op == 'view') {
   

   Nit, can we use === here because a) we know they're strings and b) this is access code so let's be safe

2. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -119,22 +119,30 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
   +        // different revisions so there is no need for a separate database check.
   

   dreditor is showing this as >80 chars

3. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -119,22 +119,30 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
   +        if ($node->isDefaultRevision() && ($this->nodeStorage->countDefaultLanguageRevisions($node) == 1 || $op == 'update' || $op == 'delete')) {
   

   same here on ===, I realise this is existing code, but while we're moving it around

4. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -119,22 +119,30 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
   +          // node passed in is not the default revision then access to that, too.
   

   this one showing as > 80 chars too

Fixing nits. Not changing the status here, but should this really have gone from RTBC > Needs review? Should it go back to RTBC now? I am changing no (minimal, == to ===) logic with the nit fixes.

Yeah, it should've come back to needs work actually. RTBC is only for issues that can go in without any more changes, since this needed changes, Needs work should've been the correct status.

Anyway, back to RTBC!

Can we get a change record here please? thanks

Here it is https://www.drupal.org/node/3001801

Thanks, looks like someone else created one too, but didn't re-rtbc.

I merged the two

Updating issue credits for @miro_dietiker and @Berdir whose input and mentoring shaped the patch.

Crediting @alexpott for review

No longer applies since #2996030: Convert web tests to browser tests for node module - round 2

error: core/modules/node/src/Tests/NodeRevisionsTest.php: does not exist in index

Here's a reroll.

Re-rerolled #78.

tested patch #87

Committed as 37c7dfa and pushed to 8.7.x.

Published change record.

Only took us 8 years ;)

In our 7.x distribution, we want users to always be able to access the "Revisions" tab even if there is only one revision. So, for our purposes, the original patch all the way back in #7 fits the bill.

Attached is a re-roll of that patch for D7, for anyone else who needs that approach.