Move translation based conditions into database query on revisions overview page

Just showing all revisions is not a solution. That will reintroduce all the issues we fixed by limiting the shown revisions to the affected ones.

A bit of history:

• #2453153: Node revisions cannot be reverted per translation
• #2465907: Node revision UI reverts multiple languages when only one language should be reverted

What could be discussed is a language selector or a separate tab.

I already proposed something like that but finally webchick decided to go for the current solution because it "simulates" the D7 behavior where revisions of translations where decoupled.

@catch: I think we're basically of the same opinion.

There are both use-cases, one to show all revisions regardless of their translation, and another one to filter the revision to those that are really affected by the currently shown translation.

As far as I remember we agreed on a 20% vs. 80% split of these use-cases which was the reason for the current implementation.

To repeat one of my proposals:
Introduce a new tab that shows all revisions regardless of their translations and split the right "revert all revisions" into two rights (one for each tab) to have to possibility to hide the new tab for editors.

BTW In Revision UI we created a prototype that not just deitinguishes between translations but let you select the concrete fields you want to revert.

One thing to consider is modules like https://www.drupal.org/project/paragraphs.

Technically, that works with untranslatable reference fields. But as far as the user is concerned, it is translatable (we store the same reference and maintain translations of the referenced paragraph entities within the widget)

That's currently pretty weird with the core revision UI, as you have to check the untranslatable checkbox to have your "translatable" paragraphs reverted, and I've also had troubles getting revisions to show up as expected when you just change paragraphs.

I don't understand comment #5 :) but besides that I don't see any arguments against showing all revisions, only that we have to design how to do it.

A screenshot of how Revision does things could be helpful.

Another related problem at would be solved by this: The paging is done in NodeController::getRevisionIds(); that returns 50 revisions at a time. Then in ::revisionOverview(), some of the revisions may be skipped. This can result in an empty results page. I've seen a pager with several pages, but only two revisions that actually appear anywhere.

Is this something we still can/want to/should consider?

Added related/dupe issues for core and Diff. I think the main issue here is the isRevisionTranslationAffected check.

The generic revision UI also has this same check in VersionHistoryController::loadRevisions. Whatever fix we do for Node should apply there too.

Agreed that we should keep this one simple and potentially add a language filter in another issue if needed.

These conditions were added in #2465907: Node revision UI reverts multiple languages when only one language should be reverted so it will be crucial we don't regress on that bug

Commit https://git.drupalcode.org/project/drupal/-/commit/7940793ae0bbfdb64fb5e...

After debugging this more it's clear that the current code is definitely masking some pretty weird issues.

E.g set up the following:
1. Install standard
2. Install content_translation module
3. Add another language (e.g Dutch)
4. Edit Basic page content type and mark it translatable
5. Create a node, then translate it to the new language, adding a revision log of "create LANGUAGE revision".

At this point this will create 2 new revisions - one for English and one for Dutch. The English one will be marked with revision_translation_affected = NULL
The node_revision table will have 2 records, but node_field_revision will have 3. 2 of those will be for vid 2, one for each language. Vid 2 for english will have values copied from vid 1 (i.e created/changed dates).
Since the revision_log field is not translateable, the node_revision table will have "create LANGUAGE revision" for vid 2, which is the default revision for english.

6. Go to node/1/revisions (i.e the English revisions page). You'll see 1 revision marked as current, but this is actually revision 1. You won't see a revision log message, which IMO is incorrect. The reason this is incorrectly shown as the current revision is because of this code:

        // We treat also the latest translation-affecting revision as current
        // revision, if it was the default revision, as its values for the
        // current language will be the same of the current default revision in
        // this case.
        $is_current_revision = $revision->isDefaultRevision() || (!$current_revision_displayed && $revision->wasDefaultRevision());

This is green now.

Want to get a review on the code changes before going ahead with a full IS update + CR.

Can the summary be updated with what’s being proposed? Do we want a page with all revisions loaded? What if it’s 1000s of revisions

@smustgrave did you see #29?

Do we want a page with all revisions loaded

No, we still paginate revisions.

Yea but without a clear summary hard to review the code as it wasn’t clear what the we being accomplished

Any recommended way for testing this one?

@smustgrave it's a bit tricky because you have to generate revisions that are marked as non translation affecting.

The easiest way to do that is programatically generate some revisions similar to the tests.

Ran test-only feature

1) Drupal\Tests\node\Functional\NodeRevisionsUiTest::testNodeRevisionsTabWithDefaultRevision
Behat\Mink\Exception\ExpectationException: 2 elements matching css ".node-revision-table tbody tr" found on the page, but should be 5.
/builds/issue/drupal-2722307/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-2722307/vendor/behat/mink/src/WebAssert.php:441
/builds/issue/drupal-2722307/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php:176
FAILURES!
Tests: 4, Assertions: 40, Failures: 1, PHPUnit Deprecations: 10.
Exiting with EXIT_CODE=1

Tried to do the same as the tests manually but ended up borking it so going to lean on the test coverage here.

But overall believe this one is good and don't see any open items for it. Going to mark.

Is there a follow up for the point raised by catch in #24?

@smustgrave, thanks for asking for an issue summary update, that is really helpful.

I have updated credit.

Using Umami, I created a new content type and created an English node. When a Spanish translation was added the English revision tab showed a new revision. Seeing this in action and from an editor perspective that was unexpected because the English version of the node was not changed. I added another language, bringing the total to 3. And confirmed that every save to the node in another language caused a revision to appear on the English language revision page. In my limited testing the English version revision page shows 7 revisions with this change and 1 revision without this change. That does reflect the database but will it make sense to the editor?

Like @smustgrave, I wasn't able to reproduce the test from the UI. So, I can't confirm what the test is doing. The test alone went from 2 revisions to 5 revisions displayed.

I would like to know more about how this will effect editors. There will definitely by an increase in the number of revisions shown on the revision tab for the default site language. And for a site with many languages and revisions on a node that is surely going to be a surprise. This would need a CR to explain that, I think.

So, if I am following this correctly then there should be a UX review of this.

Should be needs work for the usability review and a change record, and maybe even doc updates. I did bring this up with the other committers.

That does reflect the database but will it make sense to the editor?

I agree it's quite a large change, but if you look at all the related issues and #27 you'll see the current situation is really not ideal. I'd rather see more detail than not be able to see what's actually going on.

I think the reason why we see new revisions for the default language is because revisions are created for the default language as well?

revisiting this with fresh eyes I realised we needed to filter by revision_translation_affected, adding that means we are back to where we were in terms of revision displays (tests pass with no changes). Now we need test coverage for what we're fixing (e.g broken pagers with a lot of unaffected revisions)

Added test coverage for the issue, here's what the revisions page looks like on HEAD in that test case:

Now expanded to the generic revision UI as well.

Same failure as #41 with test-only changes "0 elements matching css "table tbody tr" found on the page, but should be 50."

Thanks for pushing it forward Adam!

Changes looks good and test coverage added. Patch applied to existing project and tested. All looks to be OK.

This looks pretty good to me - just a few questions / thoughts on the MR.

I think the issue summary needs an update as the solution in the MR and issue summary seem to have diverged.

I think this is good to go. Just fixed the issue summary to be inline with the MR.

I believe this is ready. Thanks everyone!

Committed and pushed aab88182a75 to main and 2e92f41d915 to 11.x and 4d6098cd456 to 11.3.x. Thanks!

I've backported to 11.3.x as this only make changes to behaviours of controllers which are not part of our BC policy and it does not make any API changes and it does fix a bug.

@alexpott it doesn't look like the commits were pushed to main and 11.x? They aren't linked here on the issue and when pulling locally I can't see the changes on those branches.

This change broke the Version History "Revisions" page for our custom entity types that do not include a revision_data_table. The revision page works after defining the revision_data_table and doing a fresh install, but not after simply doing a cache rebuild. See error details below.

Interestingly, there was a change to the entity type the RevisionVersionHistoryTest test is using: https://git.drupalcode.org/project/drupal/-/commit/4d6098cd4564f7d52f4d4... It was previously using EntityTestRev which does not define a revision_data_table, but now it uses EntityTestMulRev which does define a revision_data_table .. !

So... it is not clear to me if revision_data_table is a required attribute for revisionable content entity types?? If it is not required, then I think this issue introduced a breaking change.

Unfortunately, I cannot find good documentation about what is required with revision_data_table. This doc page Making an entity revisionable does not mention revision_data_table. FWIW our custom entity type that does not include this has been working fine for many years.

Drupal\Core\Entity\Query\QueryException: 'revision_translation_affected' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 376 of /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php).

#0 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php(268): Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable('', 'revision_transl...', 'INNER', NULL, 'base_table', 'revision_id', Array)
#1 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php(58): Drupal\Core\Entity\Query\Sql\Tables->addField('revision_transl...', 'INNER', NULL)
#2 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(193): Drupal\Core\Entity\Query\Sql\Condition->compile(Object(Drupal\pgsql\Driver\Database\pgsql\Select))
#3 /var/www/html/web/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(361): Drupal\Core\Entity\Query\Sql\Query->compile()
#4 /var/www/html/web/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php(228): Drupal\Core\Entity\Query\Sql\Query->__toString()
#5 /var/www/html/web/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php(258): Drupal\Core\Entity\Controller\VersionHistoryController->loadRevisions(Object(Drupal\ertcore_layer\ERT\Domain\LayerGroup))
#6 /var/www/html/web/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php(75): Drupal\Core\Entity\Controller\VersionHistoryController->revisionOverview(Object(Drupal\ertcore_layer\ERT\Domain\LayerGroup))

@paul121 it looks like that would only happen if the entity type is translatable AND does not have a revision_data_table since it looks like that is coming from the revision_translation_affected condition? I'm not entirely sure if that should be required or not, can you open a separate issue to address this?

This change broke the Version History "Revisions" page for our custom entity types that do not include a revision_data_table.

I can confirm that we ran into this error with the Log entity module. It is impossible to view Log entity revisions.

I opened a follow-up issue: #3596981: Uncaught PHP Exception: revision_translation_affected not found