Node revisions tab have "Current Version" on every Page

I don't understand what the problem is, please describe what you think is wrong and how it should be.

Ah, now I get it, you mean different pages of the pager. I missed the page argument in the URL.

Your fix isn't correct though, I think what we likely need is to add an *additional* check instead that excludes page > 0. That would theretically still be breakable if you have so many drafts that the actual current revision is the on the second page, but that seems like a pretty unlikely scenario.

I have this issue as well, and can confirm that the patch does indeed fix it.
I'm curious however, how there's so little feedback as this still seems to be an issue?

It's not just a visual issue, as this issue makes it impossible to revert back to any of those revisions that are displayed incorrectly as 'Current revision' ( as there are no available 'Operations' in that case ).

The patch in #7 assumes that the default revision on a node is displayed in the revisions table--but the revisions table filters to only show revisions where the the current translation is affected. This gets caught by the failing test in NodeRevisionsUiTest.

There is an identical bug reported in #3082614: Node revisions: in paged lists, the first revision in any page is always marked as default, but the patch there has the same problem and doesn't fix the issue if translations are enabled.

I'm taking a different approach: to try to determine which revision ID is the current revision ID by querying the database for the latest revision ID for the current langcode that is both a default revision and affects the current translation. Best I could tell, there was no method in the TranslatableRevisionableStorageInterface API so I'm having the controller do an additional query to the database to find the revision.

Fixing code style in #15.

Bumped to 9.4.x which is the current development version

On the one hand, the patch in #16 does the job, and maybe there's no need to over-engineer it if it's the only use-case in core where we need "Give me the current revision ID for the current translation of an entity".

On the other hand, I'm wondering if we should offer this method for all translatable and revisionable entities and add a getCurrentRevisionId() to TranslatableRevisionableStorageInterface. It has more side effects in core and contrib code extending it though.

reviewing this

Verified patch for Drupal 9.5.x dev version
https://www.drupal.org/files/issues/2022-07-15/3021671-22.patch

Testing Steps:
* Install a new Drupal instance
* Created basic page
* Edit it multiple times till pages for revisions not create

Problem:
Each revision page's first content shows current version

Test Result:
Verified that after applying patch only latest version content shows current version not other revision pages first row

Refer SS
Before apply Patch

After Patch

Test Result Pass
Can be move to RTBC

Thanks for working on this bug!

However, core committers won't commit this (or most any other issue) without test coverage. As a bug, we want to make sure it never regresses as folks make further changes to the relevant code.

So we need:

1. A test that demonstrates the problem, uploaded as a failing test-only patch.
2. A full patch that includes the new test and the patch from #22 which should pass.

Thanks / sorry,
-Derek

Uploaded a test case.

This needs an issue summary update with, at least, before and after screenshots and proposed resolution.

@aarti zikre, thanks for the testing. When making screenshots put the latest before and after in the Issue Summary because it helps the reviewers and committers.

I ran the test locally and have some suggestions.

1. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
   @@ -187,4 +187,30 @@ public function testNodeRevisionsTabWithDefaultRevision() {
   +    // Create 75 revisions to generate revisions.
   +    $revision_count = 75;
   

   Why 75? Can we get the number per page from somewhere and use that?

2. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
   @@ -187,4 +187,30 @@ public function testNodeRevisionsTabWithDefaultRevision() {
   +      $node->setRevisionLogMessage($this->randomString());
   

   Since we already have a unique value, can we use $i here and skip making a random string.
   $node->setRevisionLogMessage((string) $i);

3. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
   @@ -187,4 +187,30 @@ public function testNodeRevisionsTabWithDefaultRevision() {
   +    $this->assertSession()->addressEquals('node/' . $node->id() . '/revisions?page=1');
   +    $this->assertSession()->pageTextNotContains('Current revision');
   

   If the revision count is less that the number of revisions per page, these assertions pass. I think there should be an assertion that fails if there are no revisions on page 1.

Addressed issues #29.1,2,3 and updated issue summary

Verified and tested patch #31 on Drupal 9.5.x-dev. Patch applied successfully and looks good to me.

Testing steps:
1. Go to Content
2. Click on + Add content
3. Select a Basic page then enter the title and Save the page
4. Now a new basic page is created. Edit this page again and again until pagination is created
5. Go to the revisions tab and observe the 1st page and 2nd page
6. Current revision is displayed on both page
7. Apply patch
8. Reload the page and observe
9. Now the Current revision is displayed only on the 1st page at the top

Testing Result:
1. After applying the patch the Current revision is displayed only on the 1st page at the top

Can be moved to RTBC

While applying comment #16 patch on Drupal v 9.4.3 and PHP 8.0, it's getting fail and composer gives below error:

The error was: Cannot apply patch https://www.drupal.org/files/issues/2022-01-15/3021671-16-determine_curr...
patching file modules/node/src/Controller/NodeController.php
Hunk #1 FAILED at 164.
Hunk #2 succeeded at 182 (offset 1 line).
Hunk #3 succeeded at 302 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file modules/node/src/Controller/NodeController.php.rej

Do we have any specific patch for drupal version 9.4.3 and PHP 8.0 ? How can we apply this patch for the mentioned setup ?

@Aamir M, Welcome to Drupal! Thanks for testing the patch, when the testing includes screenshots they should be added to the issue summary in the user interface section. Thanks

@smustgrave, thanks for the updated patch.

Just some minor things.
The changes made in the patch have not been reviewed and since there were changes made to the test can we have a fail patch.

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
@@ -210,6 +210,8 @@
+    // Verify the first revision exists on page1.

s/page1/page 1/

I have not reviewed the code or the changes to the test.

Actually rethinking this one I’m not sure it’s a bug? If the current revision isn’t on every page are you able to compare revisions across pages?

Test patch with Drupal v 9.4.3 and PHP 8.0

@Parashram can you please upload an interdiff if you are going to submit a patch.

Patch #31 Tested on Drupal 9.4.x-dev. Patch applied successfully.
Before the patch

After The Patch

Patch #31 working on Drupal 9.4.3, PHP 8.0

Patch isn’t passing core tests so testing manually doesn’t count

> Actually rethinking this one I’m not sure it’s a bug? If the current revision isn’t on every page are you able to compare revisions across pages?

FWIW, drupal core doesn't allow you to actually compare versions, that's diff.module which mostly replaces the current core functionality, as there's not really another way. With core, you can just manually compare by viewing the different revisions in separate tabs, that could be done across multiple pages.

That said, I do agree that maybe this needs a UX review. There might be (better?) alternatives to just removing it, maybe show it separately and not in the main list?

There's also something strange about the screenshots. It still shows 3 revisions on the second page, I would expect one less if it doesn't repeat the current one?

The current problem is that the second page is just labeling the first revision as "current revision" (though it is not). Essentially, the way core currently works is that the second and subsequent pages of revisions will always label the first listed revision as as "current."

So I think there is a discrete bug here that can still be addressed while maybe separately considering UX improvements.

Added test only patch and fixed one nit of patch #31 as mentioned in comment #35.

Hi All,

Is it working for Drupal 10?

Thanks!

Patch #46 Tested on Drupal 9.5.x-dev. Patch applied successfully.
For the Drupal 10 test case is failing. Need to fix the test cases.
Before Patch:

After Patch:

.

Re-roll of #46 against Drupal 10.2.x.

Updated the issue summary and rebased the MR to 11.x branch. I agree with #45 that doing more complete UX renew should be done on separate issue so we could fix this 6 years old issue first.

1 test I think we should consider is what if someone wnats to compare current revision with a revision on a 2nd page, will that still be possible.

we should consider is what if someone wnats to compare current revision with a revision on a 2nd page, will that still be possible.

Quoting @berdir from #43

drupal core doesn't allow you to actually compare versions, that's diff.module which mostly replaces the current core functionality, as there's not really another way. With core, you can just manually compare by viewing the different revisions in separate tabs, that could be done across multiple pages.

I assume we don't want to include Diff module into core on this issue, so setting back to Needs review. Diff module uses routeSubscriber so changes in this MR does not have effect on Diff module.

Feedback has been addressed.

All feedback appears to be addressed.

In #43 a UX review was asked for and I don't see that. Tagging for a review.

At least one thing that will be needed here is up to date screenshot available form the Issue summary. That same would be true for testing results. If they are in the issue summary, then the reviewer can find the correct work to review.

Is this worth spending time on given this will swap to the generic revision UI in #3153559: Switch Node revision UI to generic UI?

I also noticed other weird behaviour when testing this such as the "Current revision" on the second page shows the revision log message of a different revision.

Wanted to give 1 more bump for #63

Can confirm that #3153559: Switch Node revision UI to generic UI would make this obsolete.

Lets close this one as a duplicate then thanks for following up!