Problem/Motivation

While migrating a large (active) site with important revision history from D7 to D9, the revision IDs would be out of order as they came in, leading to a broken interface from a user perspective. Considering that the revision ID isn't really the important part from an admin user perspective (and isn't displayed in the admin interface anywhere), changing the sort to timestamp (which is the text to the revision link already) seems reasonable.

Steps to reproduce

(Simplest)
1. View a node with multiple revisions.
2. Change one of the revisions in the `node_revision` table - either swap the IDs or swap the timestamps. (Also easier to see the difference if the timestamps are >1 minute apart).
3. Note that the ordering on the `Revisions` (version-history) tab of the node is out-of-order by date.

Proposed resolution

Add an initial sort on getRevisionMetadataKey('revision_created') and use getKey('revision') as the secondary sort.

Remaining tasks

  • Add timestamp as primary sort
  • Get all existing tests passing
  • Add new test coverage?
  • Reviews / refinements
  • RTBC
  • Commit

User interface changes

The revisions tab will be sorted by timestamp instead of revision ID.

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#15 3276716-15-sort-by-revision-timestamp-8.x-1.1.patch2.47 KBgdeguzman
#14 3276716-14.patch2.47 KBdww
#9 sort_by_revision_timestamp_combined.patch2.46 KBgdeguzman
#6 sort_by_revision_timestamp_fixed_test_9.4.patch2.37 KBgdeguzman
#5 sort_by_revision_timestamp_fixed_test.patch1.25 KBgdeguzman
#4 sort_by_revision_timestamp_with_fallback.patch644 bytesgdeguzman
sort_by_revision_timestamp.patch661 bytesgdeguzman

Issue fork diff-3276716

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:

About issue forks

Comments

gdeguzman created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review

In principle, this makes total sense to me.

I wonder if there's a more generic way we should determine what revision_timestamp really is, or if we can just hard code it. The existing code does seem a little bit pointless generalization, if we're only doing this on nodes. ;) But if we're trying to be generic, perhaps something like $node->getEntityType()->getRevisionMetadataKey('revision_created')...

Status: Needs review » Needs work

The last submitted patch, sort_by_revision_timestamp.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gdeguzman’s picture

gdeguzman commented
StatusFileSize
new sort_by_revision_timestamp_with_fallback.patch644 bytes

This patch keeps the revision ID as a fallback after sorting by timestamp - particularly useful with tests, which may generate revisions very quickly.

gdeguzman’s picture

gdeguzman commented
StatusFileSize
new sort_by_revision_timestamp_fixed_test.patch1.25 KB

Due to changing the sorting, a test was failing because it was looking for the `Current Revision` in the second row versus the first row.

gdeguzman’s picture

gdeguzman commented
StatusFileSize
new sort_by_revision_timestamp_fixed_test_9.4.patch2.37 KB

This patch adjusts how the revision is obtained as suggested in #2, and fixes tests for Drupal 9.4.

Test changes for 9.4:

  • 'of' was changed to 'the' in Drupal 9.4 for revision deletion confirmation text. Removed the article entirely as it was irrelevant to the part being tested (making sure the revision was deleted
  • The order of 'Delete' links changed, leading to the `Delete` key at index 0 being the parent node `Delete` rather than any individual revision. Changed how the link was selected by using the `$rows` variable to click the `Delete` link within the table rather than on the overall page.
dww’s picture

dww
we/he/they
commented
Priority: Minor » Normal
Status: Needs work » Needs review

Thanks for getting the tests passing again! Back to NR. This seems normal, not minor. 😉

dww’s picture

dww
we/he/they
commented
Title: Sort revisions by timestamp » Sort revisions first by timestamp, then revision ID
Issue summary: View changes

Updating the summary to reflect what the code is now doing, and to update remaining tasks.

I don't think we need a config knob for this behavior. I'm in favor of making this the new default hard-coded sort. However, I'm torn, since flexibility is so important to so many folks around here. 😉 I wonder if there's a clean way to make this query more easily alterable when needed. Probably that part is better as a followup, but worth considering here as we change the default...

Thanks,
-Derek

gdeguzman’s picture

gdeguzman commented
StatusFileSize
new sort_by_revision_timestamp_combined.patch2.46 KB

Updating the patch to set the `Revert` and `Set to current revision` labels according to the revision timestamp and then the ID, similarly to how the sort is handled.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The combined patch doesn't apply to the 8.x-1.x branch, but the approach sounds good.

1 minor code style nit:

+++ b/src/Form/RevisionOverviewForm.php
@@ -255,8 +256,14 @@ class RevisionOverviewForm extends FormBase {
+              } else {

}
else {

Seems like there should be a more simple way to write this code, but nothing immediately springs to mind. 😅

Thanks!
-Derek

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !20

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work
Issue tags: -Needs reroll
StatusFileSize
new 3276716-14.patch2.47 KB

Thanks for opening the MR and fixing the else, @rpayanm!

Sadly, this shows that the changes here are breaking some existing tests, so needs work for that.

Meanwhile, here's a version of the MR diff as a patch that applies cleanly to 8.x-1.0 for anyone who needs it.

gdeguzman’s picture

gdeguzman commented
StatusFileSize
new 3276716-15-sort-by-revision-timestamp-8.x-1.1.patch2.47 KB

Rerolling patch for 8.x-1.1!

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

We hit something like this as well, but I want to question the premise of the original report.

Why would a revision id that is higher than another revision have a timestamp that is earlier?

Since revision_id auto increments, there is no reason for this to be the case under normal operations.

This seems like a bug in the way migrations may be handled rather than something that needs to be addressed here.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Preferable change -- actually print the revision ID next to the timestamp. (This is part of #3269933: Inconsistent moderation state for translated content) and could be applied here.

Status: Needs review » Needs work

The last submitted patch, 15: 3276716-15-sort-by-revision-timestamp-8.x-1.1.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

+1 on #17/18.

acbramley’s picture

acbramley commented
Status: Needs work » Closed (won't fix)

This seems like a bug in the way migrations may be handled rather than something that needs to be addressed here.

I agree, core's Node and generic entity revision ui both just sort on revision id. I would like to keep Diff the same.