NodeController::revisionOverview() shows no revisions if node has no translation for current language

Hm, that may be a bit confusing, given the view tab deals with fallback, so you would see something, go to its revisions and see something else :/

So the current behavior may have flaws, but what you see in View is what you see in Edit is what you see revisions for in Revisions, right? I think that is generally a good pattern. I can see how that is problematic when View falls back on other things but Revisions does not. Since you are using admin language as well, do you think this is related to #2677778: Administration pages language setting makes it impossible to edit content translations for some roles and/or #2189267: When content language detection is different from interface language detection, the detected language is not applied to the rendered content?

I think we agree that if there is only one language, it should not filter. I am not sure about including eg. the current revision if its not in the current language, if you revert to there and loose translations, that will be a data loss. (Or that revert needs to work differently from the others).

Can reproduce. Here a patch and test. Needs to be improved, but now I have to leave. I'll do it tomorrow.

Testing with new node (langcode as UND), site langcode as EN, edit node settings langcode as EN and create new revision, checking revisions overview where both revision messages are displayed.

Done.

Changed comments and dropped some unneeded lines.

@berdir asked me to review this issue. The meat of the fix is as follows:

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -175,11 +176,14 @@ public function revisionOverview(NodeInterface $node) {
-      if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)->isRevisionTranslationAffected()) {
+      // Show node revisions either there are revisions of the same language or
+      // there are translations.
+      if ($single_language || ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)->isRevisionTranslationAffected())) {

I agree this fix is an improvement. The comment needs English cleanup though. (Update: crossposted, seems like this got fixed in the meantime).

First, showing all revisions when the node is 'untranslated' is how it is meant to work!
So we don't need to discuss that :-)

But I'm not sure if this "cosmetic" patch is the right solution.

Obviously function hasTranslation() doesn't handle LANGCODE_NOT_SPECIFIED or LANGCODE_NOT_APPLICABLE:

public function hasTranslation($langcode) {
  if ($langcode == $this->defaultLangcode) {
    $langcode = LanguageInterface::LANGCODE_DEFAULT;
  }
  return !empty($this->translations[$langcode]['status']);
}

I wonder if the change timestamp tracking works for these nodes where the revision table is currently erroneously empty. The ChangedItem code uses hasTranslation() as well.

As far as I remember plach and myself were of the opinion that there's always a "translation". I wonder if the LANGCODE_NOT_SPECIFIED key exists in the $this->translations array above.

However, if you go one step further, you can just as easily run into the following scenario if you have 3 languages: You have an EN node with a DE translation and your current content language is ES. What should you see then?

Which node view is shown in that case? I second Gabor that the revision table should be in sync with the view.

OK, like expected, my test only patch from #20 fails.
I think it shows that the problem with the revisions table is just one symptom.
When saving a new revision of a ContentEntity having LANGCODE_NOT_SPECIFIED its changed timestamp isn't updated.
I'm sure we'll find more issues regarding the languages LANGCODE_NOT_SPECIFIED and LANGCODE_NOT_APPLICABLE.
I think that requires a deeper inspection of the issue...

also, your test if flawed. ChangedItem uses the REQUEST_TIME constant. sleep(1) does nothing to change that.

Yes, you're right! My fault.

I had a closer look at the test. It first creates a node in 'en' and then switches its langcode to 'und'.
It seems that the function call hasTranslation('und') returns TRUE afterwards. So the ChangedItem seems to work as it should.

So when your content language is 'es' and there's no corresponding translation of the node, it falls back to its 'und' translation, right? (At least in common setups.)

Lets assume that a site has three languages: 'en', 'de' and 'es'.
A node is "translated" to 'en' and 'und'.

If the content language is set to 'en' the English translation will be shown. If the content language is set to 'de' or 'es' the node's 'und' translation will be shown as fall back, right?

If we agree that the content of the revision table should be in sync with the current node view, the revision table has to show the revisions that affected the 'und' translation if the content language is set to 'de' or 'es'.

This will not happen with the latest patch which just handles the case that the node only consists of a single language (better: a single translation).

From my point o view the right solution would be to apply the language fallback first if necessary before filtering the content of the revisions table. This should solve the initial bug report here and my more complex example.

BTW I proposed a more sophisticated revisions tab when we worked on translation revisions. But this has been refused by the maintainers. I therefor created https://www.drupal.org/project/revision_ui to incubate a really good solution. I hope to find the time to work on this soon.

I don't think its valid that an entity has a 'und' language and something else. It either does not have a 'und' language or it only has a 'und' language, right?

In the code: get the node language instead of current content language.
In the test:

• switched UND to EN langcode for the first node example
• added a second node example in EN, translated in DE, view revision in ES, DE and EN

Now what is displayed...

• ... in the UI (see also screenshots)
• "es/node/ID/revisions": original EN revision only
• "de/node/ID/revisions": original EN revision + translated DE revision
• "en/node/ID/revisions": original EN revision only
• ... in local test
• "es/node/ID/revisions": original EN revision only
• "de/node/ID/revisions": translated DE revision only
• "en/node/ID/revisions": original EN revision only

IMO it should display all language revisions at least in one place (e.g. for the original node revisions, like I also check in the test), but agree that if there are a lot of revisions it could be a "noisy view" then sure we need to display also the "revision langcode" for each row. What should be the correct result ?

IMO it should display all language revisions at least in one place

We had a long discussion about this before 8.0.0 release. The consensus was not to do that because Drupal 7 didn't do that as well. In Drupal 7 every translation had it's own revisions.
From the user's perspective displaying all revisions across all translation would be a new feature compared to Drupal 7.

As I already mentioned in #18 initially I totally agree, that we have a bug here, but it should not be mixed with that new feature.

That new feature is not that easy, because after just displaying all revisions across the translations you have to decide about the revision revert strategy. Could you revert hardly to any previous revision even if it means that you remove translations?

Again, a lot of questions like that have been discussed already.

Solving that requires a sophisticated UI protected by access rights.
I can invite you to join the Revision UI module for that.

Lets fix the bug now.

Yep, agree it should be a separate issue.

… displaying all revisions across the translations you have to decide about the revision revert strategy. Could you revert hardly to any previous revision even if it means that you remove translations?

Sorry, missing explanation: what I mean is that could be nice to just show all available revisions of a node, somewhere, like a "neutral history" of all revisions made, like chronological revisions overview (host or translated node). I don't know, it's just a personal opinion about a nice and clean overview feature I appreciate to use in any app/in general. Or maybe I'm just strange ;P

So, back to this bug issue, in the end what it is supposed to display is like it is in the test, right ? Why is it different for the UI... ?

Done.

Done.

Just to verify, is the difference described in #28 between the UI and the tests gone?

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -157,11 +157,11 @@ public function revisionPageTitle($node_revision) {
   -    $node_storage = $this->entityManager()->getStorage('node');
   +    $node_storage = $this->entityTypeManager()->getStorage('node');
   

   The deprecated entityManager() method is used multiple times in this controller. We should replace them all or none of them.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -211,6 +213,64 @@ function testRevisions() {
   +    // Enable additional languages.
   +    ConfigurableLanguage::createFromLangcode('de')->save();
   +    ConfigurableLanguage::createFromLangcode('es')->save();
   

   The setUp() of the test already contains ConfigurableLanguage::createFromLangcode('it')->save().
   I suggest to remove these lines here and to add one additional language in the setUp() function.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -211,6 +213,64 @@ function testRevisions() {
   +    // View the revision UI in 'DE', only the translated node revision is shown.
   +    $this->drupalGet("de/node/" . $node->id() . "/revisions");
   

   Maybe that's too much, but I would like to see a test for the expected behavior if the languages of TYPE_CONTENT and TYPE_INTERFACE are set to different values.

is the difference described in #28 between the UI and the tests gone?

Yep, now everything works as in the test.

1. I guess we want these fixes in another separate issue. Reverted it.
2. Done.
3. Discussed with @Berdir and he said that it is not necessary, we don't do anything here about TYPE_CONTENT and TYPE_INTERFACE that needs to be tested.

Created follow-up #2721573: Replace deprecated entityManager() in NodeController for comment #39.1

Looks good.

So for me personally, I think we should just show all revisions - that's the actual revision history of the node.

Also:

I am not sure about including eg. the current revision if its not in the current language, if you revert to there and loose translations, that will be a data loss. (Or that revert needs to work differently from the others).

Reverting doesn't delete anything, it creates a new revision and sets that as the default, so you can always get back to the previous state. Also what if you wanted to revert specifically to drop a translation you just added?

Patch itself looks fine, and I'm OK committing this - but can we open a follow-up - I don't think the issue summary nor the patch really covers everything going on in this issue.

I agree with @catch that the IS needs updating to reflect the current patch.

Reading through all the issue comments makes me feel a follow up issue to address the originally proposed solution of showing all revisions somewhere should be created - but as pointed out several times on issue that'd be a new feature.

Opened the follow-up #2722307: Consider showing all revisions on the revision overview.

IS updated.

I have the same problem with the Diff module: #2716087: Needs tests: Language is chosen from content not from context

@tudog has updated the issue summary.

But isn't it easier?

Problem/Motivation

NodeController::revisionOverview() is supposed to filter the list of revisions to only show those revisions that affect the currently shown translation of the node.
In case that the requested node translation doesn't exist and a language fallback was applied, the list of revisions is erroneously empty instead of showing the revisions that affected the fallback translation.

Proposed resolution

Filter the list of revisions by the translation language of the currently shown node instead of the requested translation.

Updated IS as suggested in the comments above.

I triggered a re-test in #40 and removed the Remaining tasks from the IS because this potential issue already exists regardless of the proposed patch here and is covered by #2722307: Consider showing all revisions on the revision overview.

re-test of #40 passed

FILE: ...ore/modules/node/src/Controller/NodeController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 100ms; Memory: 6.75Mb


FILE: ...core/modules/node/src/Tests/NodeRevisionsTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed the 2 marked sniff violations. Needed a while to figure out that it was only about the unused import line :p

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Yay, thanks!