Problem/Motivation

The \Drupal\diff\Controller\PluginRevisionController::compareEntityRevisions() calls getRevisionIds() for all languages and then determines if the language matches by loading the revision. This can be problematic on a site with lots of revisions.

Steps to reproduce

See #2722307: Move translation based conditions into database query on revisions overview page

Proposed resolution

Update \Drupal\diff\Controller\PluginRevisionController::getRevisionIds() to filter the query by "langcode" and "revision_translation_affected".

Remaining tasks

Do it

User interface changes

None.

Introduced terminology

N/A

API changes

Passing an entity id to PluginRevisionController::getRevisionIds is deprecated, pass the entity instead.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 diff-3418442-MR-109--20241003-4.diff3.86 KBrecrit
#8 diff-3418442-MR-109--20241003-1.diff2.7 KBrecrit
#5 interdiff-3418442-4-5.txt1.12 KBrecrit
#5 diff-3418442-5-filter-by-langcode.patch2.25 KBrecrit
#4 interdiff-3418442-2-4.txt1.54 KBrecrit
#4 diff-3418442-4-filter-by-langcode.patch1.97 KBrecrit
#2 diff-3418442-2-filter-by-langcode.patch1.51 KBrecrit

Issue fork diff-3418442

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

recrit created an issue. See original summary.

recrit’s picture

recrit commented
Status: Active » Needs review
StatusFileSize
new diff-3418442-2-filter-by-langcode.patch1.51 KB

The attached patch updates \Drupal\diff\Controller\PluginRevisionController::getRevisionIds() to filter the query by "langcode" and "revision_translation_affected"

Status: Needs review » Needs work

The last submitted patch, 2: diff-3418442-2-filter-by-langcode.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

recrit’s picture

recrit commented
Status: Needs work » Needs review
StatusFileSize
new diff-3418442-4-filter-by-langcode.patch1.97 KB
new interdiff-3418442-2-4.txt1.54 KB

updated the patch to check if the entity type is translatable before adding the new conditions to the query

recrit’s picture

recrit commented
StatusFileSize
new diff-3418442-5-filter-by-langcode.patch2.25 KB
new interdiff-3418442-4-5.txt1.12 KB

Updates to the patch: Remove loading all revisions - This is no longer needed since the query is already filtered by language and translation affected.

recrit’s picture

recrit commented

recrit opened merge request !109

recrit’s picture

recrit commented
StatusFileSize
new diff-3418442-MR-109--20241003-1.diff2.7 KB

Use the MR 109 for the latest changes applied to 1.8+. Attached is a static patch of the MR to use for composer builds.

recrit’s picture

recrit commented
StatusFileSize
new diff-3418442-MR-109--20241003-4.diff3.86 KB

code cleanup complete. All warnings on the MR are for files that are not changed in the MR.
Attached is a static patch of the MR to use for composer builds.

acbramley’s picture

acbramley commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Needs review » Needs work

MR needs to be against 2.x

recrit opened merge request !110

recrit’s picture

recrit commented
Status: Needs work » Needs review

MR110 created against 2.x

acbramley’s picture

acbramley commented
Status: Needs review » Postponed

Postponing this on #2722307: Move translation based conditions into database query on revisions overview page once that is in we can apply the same logic to Diff.

This may also just be a duplicate of #2882334: Revisions are not visible for some nodes

acbramley closed merge request !109

acbramley closed merge request !110

acbramley’s picture

acbramley commented
Title: Revisions controller getRevisionIds() should filter by language » Revisions should be filtered by language in the database query
Issue summary: View changes
Status: Postponed » Active
Related issues: +#2722307: Move translation based conditions into database query on revisions overview page

The core issue is close to being committed so we can apply the same logic here and clean up a lot of code.

acbramley opened merge request !149

acbramley’s picture

acbramley commented
Status: Active » Needs review

Changes are done and tests have been fixed, it would be good if anyone with a large multi-lingual setup could manually test these changes and report back :)

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Posted some minor feedback on the PR. I did no manual testing.

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

Discussed extensively in slack https://drupal.slack.com/archives/C1BMUQ9U6/p1768919644431179

Let's remove the deprecation in getRevisionIds and make the method private. Once merged we will release 2.0.0

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Made the changes @acbramley and I discussed on slack.

#20 is an excellent plan.

acbramley’s picture

acbramley commented

Created #3568877: Remove $previous_revision handling from RevisionOverviewForm after reviewing the latest changes. Should probably do that before releasing a stable too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@acbramley yeah - I noticed that too.- that was left in to consider reverting https://www.drupal.org/project/diff/issues/2880936 - but I agree #3568877: Remove $previous_revision handling from RevisionOverviewForm makes sense. There's also \Drupal\diff\DiffEntityComparison::summary() which is never called apart from by itself...

  • acbramley committed e0a1df26 on 2.x 
    fix: #3418442 Revisions should be filtered by language in the database...
acbramley’s picture

acbramley commented
Status: Needs review » Fixed

Thanks everyone!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.