Node revision UI reverts multiple languages when only one language should be reverted

The proposed UI does not handle the case where a revision may change multiple languages at once. Eg. migrated entities. How would this UI display multilingual revision changes (where multiple language content changed)?

@Gabor Hojtsy: My last patch for #2453153: Node revisions cannot be reverted per translation already listed every language that has been changed in a revision and offered a revert link per language. So it's an improved version of the screenshot posted above.

Would be great to get that screenshot here then so we have the up to date baseline for the discussion :) Can you help with that?

.

Updated issue summary with the new image.

As for the suggestions in the issue summary, while the view and edit pages can clearly only allow for viewing or editing one language variant, the revisions page is a different animal IMHO. It may very well not be evident if we limit the list by language and require you to use the regular language selector. IMHO it would be better to expose a select list on the page with languages and filter using that. We may filter the revisions by the current content language by default and allow an option to filter by none. My 2 cents.

Yes good idea to filter the whole revision page by language and set it to current language by default.

In addition to see current language I think it would make sense to see changes in global values (non-translatable values) by default as only seeing the translatable values can make you loose some context.

"Global values" could be an option on the select list, then you could even see revisions in non-translatable values only.

Maybe make it possible to choose several options on the select list, like an autocomplete widget, then you could choose if you only want to see revision for current language or also have global values. Or even to check revisions for two languages at the same time: It can make sense, let's say the source language have changed and you want to check revisions of the current language side-by-side with the source language to understand why another translator did what she did.

For the people at the dev days who want to test the UI as shown in the screenshots at #7:

Apply these patches to the current core dev version:

• #89 of #2297817: Do not attempt field storage write when field content did not change
• #53 of #2428795: Translatable entity 'changed' timestamps are not working at all
• #49 of #2453153: Node revisions cannot be reverted per translation

The UI shown in the screenshots in #7 could be easily extended after the patches I mentioned in #10 are applied.
For example it's possible to show the last author and the last date for every translation edit listed in the "Language Edit" column. All the required information is already stored in the database after the patches are applied.
If we want to this could be offered as a novice task/issue for devdays.

I tested patch in:
https://www.drupal.org/node/2453153#comment-9989239

It works great!

- I think it would be most intuitive to see revisions for UI language only, or make it possible to filter the revision log by language (to choose only one or several languages).

- would it be possible to get an option to revert "global values" (not translatable fields)? If a person edit global values while editing a translation this will now not be marked clearly in the revision log, so 1 it makes it more difficult to keep overview of changes in revision log and 2 it should also be possible to revert "global value" too.

- On node edit form: could it be possible to make an option "Create new revision (this translation only)" on the side of "Create new revision (all languages)"?

I have one more comment on:
https://www.drupal.org/node/2453153#comment-9989239

- When a translation is reverted, maybe it should be clearly marked in the log? Like "Revertion of English translation 06/13/2015"

- Would also be nice to be able to edit/add the "Revision log message" (now it just copy the old revision log, but in many cases it would make more sense that a person write a comment on why they revert the translation).

I removed all the UI related code from #2453153: Node revisions cannot be reverted per translation and created an initial patch here that only contains that UI code (2465907_16-do-not-test.patch).

The code has been adjusted to the current draft of #2453153: Node revisions cannot be reverted per translation and should be independent from the ongoing discussion about "filed API vs. storage", or in other words work with both approaches.

In order to have first test results here I also uploaded 2465907_16_including_2453153_105.patch which includes patch #105 of #2453153: Node revisions cannot be reverted per translation.

The useful suggestions by @matsbla are not taken into account yet.

I will adjust the existing patch to the latest changes in core introduced by #2453153: Node revisions cannot be reverted per translation to have a good starting point for further discussions.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedFlagItem.php
   @@ -0,0 +1,77 @@
   + *   label = @Translation("Changed Flag"),
   
   +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -162,21 +174,37 @@ public function revisionOverview(NodeInterface $node) {
   +      $header[] = $this->t('Languages Edited');
   

   "Edited" need to be lowercase. "Flag" , too.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -196,8 +229,22 @@ public function revisionOverview(NodeInterface $node) {
   +                    'title' => $this->t('Revert !language', array('!language' => $language_name)),
   

   Language names can be user entered. This placeholder allows XSS I think.

@hass: thanks for your review, but the patch is totally outdated ;-)
I'm working with @cederic_a on a new version since 5 hours at the drupal con sprint. We hope to upload a new patch soon. It would be really nice if you could review that one again.

We removed the parts now implemented in the core and upload to see what the testbot says

The last patch uploaded by @cedric_a accidentally included a different patch. Here is the right version.

patch in #29 contains a copy&paste bug.

Added new screenshot from @mkalkbrenner to the "UI Changes" section of the issue description.

Edit collision? Fixed what should have been done by my last change to the issue description.

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -167,74 +171,85 @@ public function revisionOverview(NodeInterface $node) {
+              '#markup' => $this->t('current revision'),

This string need to be ucfirst! -> t('Current revision')

@hass: thanks.

This string need to be ucfirst! -> t('Current revision')

Mayby you're right but that patch didn't touch that ;-)
We can touch this on commit, but it will break existing string translations.

It will not break anything and it is incorrect. Additional translations are not done. The latest placeholder change with :placeholder will break thousands of strings... much worse than this minor fix here.

Let's fix this single bug now or we may lose track of this again.

just re-rolled against current HEAD and upper-cased the "C" in "Current revision".

Patch "2465907_46.patch" has fixed the ucletter 'c' bug in current revisions. Attaching screenshot.

This is looking good. @mkalkbrenner could you upload the test-only patch to illustrate the current behavior and the fix?

@plach and I talked about this at DrupalCon, and he walked me through the problem space.

From a "where we are in the release" standpoint, I think I'd like to see us simply return to the Drupal 7 behaviour. It seems like it simplifies things significantly, and "Revert all translations" could always be something we added in a later minor version if we decide that is a desired feature.

My concern is that if we keep it, in order to really support it well, and without causing a problem, we would need to greatly expand the overview table to include a bunch of information to prevent people from accidentally delete more than they intended. For example, they can only see the English revisions on the node revision page when their current language is English, but this option would also delete DE and IT translations, but there'd be nothing to alert them to this fact until they switched their interface language, which is an additional 3+ clicks.

I realize that's how HEAD works now and it's a bit weird to change that, but I think if we restore D7 behaviour, that will likely more closely match user expectations. And even if there is a use case for it, it seems like something more advanced that could live in contrib for now.

So D7 behavior is: 1. filter list to specific language 2. when rolling back to a revision, roll back only the fields in this language, not any others, right?

Yeah I can see how thats the simplest solution for core for now and then we can expand on it later.

My last patch brings the behavior of "Revert" for entity translations very close to the known D7 behavior:

• The listed revisions are filtered by the current content language.
• Only translatable fields are reverted.

But it's impossible to 100% recreate the original D7 behavior because entity translation is fundamentally different from node translation (and somehow incompatible, which does NOT mean that it is worse.)

To get back all features from D7 (and more new advantages) we need to create a new sophisticated UI. But that's out of scope for 8.0.0.

The reason why we can't rebuild the UI as it was in D7 are untranslated fields. If we revert an untranslated entity it is obvious to revert all fields, no matter if they are defined as translatable or not. That's 100% like D7. But as soon as the entity is translated, we have two possibilities:

1. Revert only translated fields.
2. Revert translated and untranslated fields.

I'm convinced that option #1 matches at least 80% of the use-cases and should therefor be what happens on "Revert".
Option #2 is a no-go because it basically reintroduces the issues we wanted to fix. This will cause unexpected overwrites of field values with probably very old values.
If I look at the last comments, I think we're all of the same opinion.

But if we go for that approach, there will be no possibility to revert a complete entity including all translations or to revert untranslated fields via the UI in core. That's why I introduced at least an additional "Revert all translations" action that includes the untranslated fields. From my point of view that is the minimal version of a new sophisticated UI in core. More than that should be build in contrib first. There you should be able to select the exact fields you want to revert and not rely on a guess by the core or its developers ;-)

So I suggest to keep the new action as well to at least have "something" in core for untranslated fields. But I would introduce a new dedicated permission for that action. This way it could be reserved for experienced users that know what they're doing.

If you agree with my explanations in #58 I will add the details @plach addressed in #52 and introduce a new permission for "Revert all translations".

I discussed the issue again with @plach in IRC. He second me to go for the solution I suggested in #58.
I adjusted the patch accordingly and addressed all the small things mentioned in #52, except the trailing dots mentioned in number one. These dots are not present in other routing.yml files as well and the breadcrumb doesn't look consistent if we add them.

Regarding the revision log message I reproduced the issue as shown in the screenshots in #52. But that's a different issue and out of scope here. The code here is correct and the revision log message should be correct if #2480921: Make the node entity's revision_log untranslatable and #2506213: Update content entity changed timestamp on UI save are addressed.

I extended the patch to list of affected translations as soon as a node has translations.
In addition I "fixed" NodeRevisionAccessCheck to deal with translations (the optional langcode paramter was already in place).
So far there's no explicit test coverage for the new permission. But I want to know if anything else breaks in the test bot run and provide a new version of the patch that could be used for manual tests.

What if the revision contain changes *only* on untranslatable content? Do I then need to check the "Revert also untranslatable content" check-box that is turned off by default, even though the revision only contain changes on untranslatable content?

I think it would be better to remove "also" and write:

- "Revert untranslatable content"

and accompany it with another checkbox like:

- "Revert translatable content"

That "translatable" check-box should maybe contain the specific language name like "Revert English content", "Revert French content", to make it more clear.

Then only show the check-boxes that can be applied (and make them mandatory if only on of them can be applied).

In this way a user can also choose to keep the translations and only revert the untranslatable content (it will be difficult without the second check-box). The way it is suggested now it can only be done from another language version than the change was originally made on (and remember to check "Revert also untranslatable content" even though you in fact only revert untranslatable content).

And why not show both check-boxes also on source language? If not a user can believe they revert changes only on the English node, while they in fact make changes (that will create new revisions) on all translations of the node. They will also not be able to revert only translatable or untranslatable content if the revision contain both.

From where I sit, "revert" simply reverts all changes made in a given revision. That's how it behaved in D7, which didn't have this notion of translatable vs. untranslatable content. That's also how it behaves even in a non-translation context. For example, if you update the issue summary and the tags of an issue in the same revision, and someone likes your issue summary update but hates your tags, they can either revert your revision and re-input the issue summary updates you made by hand, or they can leave the revision in place and fix the tags, creating a new revision.

There is no notion of a "partial" revert anywhere else in the system, and it complicates things significantly. I definitely would love to see us provide a proper "untrusted translator" workflow, but that's a lot more work than what we can realistically do in 5 days. It's a lot easier to iterate on that in contrib, where we can change the UI cheaply as we come up with better ideas, and target for core in 8.1 or 8.2.

AKA, "what plach said." :)

What if the revision contain changes *only* on untranslatable content?

@matsbla: Such a revision will not be listed at all if we filter be content language.

@webchick: I second you that we can't implement a good solution within 5 days. That's why I came up with my first UI proposal more than 5 months ago ;-)
But to be honest I don't understand your comment #67. What exactly are you proposing? Should we simply keep the current implementation in core? This one already implements a partial revert and is different from D7.
Or should we go back to a real "revert all" in core that reverts a complete entity as it was in core months ago but what is completely different from D7?
Or should we go for that checkbox thing, aka "what plach said."?

BTW I will start that contrib module now. Luckily all my patches that are essential to be able to that went into core over the last months :-)

@mkalkbrenner: Okay, so in other words, if a translator make changes to untranslatable content only they will not be able to see these changes in their revision log after? Should their revision then only be visible in the revision log of the source language, or not visible at all ("filter by content language")? Would that not be a new change from D7?

I know big changes will not happen in 5 days.. anyway look forward to check your contrib module!

@matsbla: I think our goal here is to have at least "something working" that is as close as possible to known behavior of D7. I know that this is impossible for multilingual content for 8.0.x in the remaining time frame :-(

I just started the Revision UI module to be able to still provide a usable solution for 8.0.x. It would be nice if you post your suggestions as feature requests there.

@webchick, @plach: Maybe we can support the 80%-use-case by going back to always list all revisions and revert the complete entity. In other words we don't support revisions of translations at all in the UI in core and point to the contrib module instead.

I am so sorry about my confusing comment in #67. I had to rush off right after plach and my discussion and didn't get back to my computer until 11pm, merely scanned plach's comment and said "what plach said" thinking that it was a summary of what we talked about. Oops. :\

I think the counter-suggestion that plach posted in #64, of a conditional checkbox on the confirm form that only appears on revisions that have the condition of affecting both translated and untranslated content, sounds great. That minimizes its effect on the overall revisions UI (which was my main concern, along with permissions bloat) for something that is hopefully a very small edge case, assuming you've set up your revisions correctly and are doing things in well-defined edits.

I get that it will still potentially allow a hypothetical translation intern to revert changes to a hero image when they go back to fix a spelling error in Russian or whatever, but IMO that's an acceptable risk. The stuff will still be around in past revisions if it needs to be restored.

The one nit-pick I'd have with the label is I don't think most people would know what "untranslatable content" means, since that's a site builder term, and this is a content editor UI. Would be ideal (only if it's easy) to label the checkbox like "Also revert changes to the Blarg, Blee fields" instead, since that will match labeling they saw in the node form. If that's not easy, no big deal. We can always adjust this label in 8.1.x or whenever.

Maybe it could be a good idea to use same expression in the label as on the edit form:
#2290101: UI telling you a field is shared across languages is way too subtle
"All languages"

OK, I implemented the checkbox solution.
As long as a node isn't translated, the complete entity gets reverted => like D7.
If a node has translations,

• the revisions on the Revisions tab are limited to the current content language => like D7
• an additional checkbox is shown on the revert confirmation form to select if the non-translatable fields should be reverted as well

BTW the interdiff doesn't make that much sense ;-)

I addressed the findings in #77.

... the only remark I have is that we should probably make sure the two revert routes are accessible only in the correct case, that is the global revert route is accessible only for untranslated entities and the translation revert route is accessible only for translated entities. Otherwise a user might inadvertently revert the entity to a state where some translations were missing and completely hide them, in fact they would appear as lost (I tested this case).

I don't agree to limit access this way.

I wonder how a user should accidentally call this route. He must explicitly type this URL including the node id. If he finds it in the browser history he already did it ;-)
And the data isn't really lost. It's still in the database as an older revision.

If you have a look at the test case implemented, I already included this use-case as a "hidden" feature. Using this route it's possible to revert a complete node. I assume that someone who manually enters this URL knows what he's doing. Personally I will support this use-case in Revision UI and don't want to be limited by a hard-coded rule for node-translations in the access check that will force my to swap out the access check. What do you think?

Checkbox label looks great.

On the access issue, I don't think that we should blow it off necessarily since it requires manually typing a URL; malicious users craft explicit URLs all the time for use in XSS attacks, etc.

However, looking at the existing permissions for node.revision_revert_confirm in HEAD, surprisingly the only thing it checks for is:

  requirements:
    _access_node_revision: 'update'

...not even the "revert Foo revisions" permission. (Though maybe that's the same thing, just wrapped in Entity API-ish language.)

So I guess if that's a problem, we should be fixing it in a separate issue for all "revisiony" routes; this patch merely makes the new route consistent with the existing behaviour.

@webchick:
The 'update' permission is mapped to 'revert Foo revisions' within \Drupal\node\Access\NodeRevisionAccessCheck::checkAccess():

  public function checkAccess(NodeInterface $node, AccountInterface $account, $op = 'view', $langcode = NULL) {
    $map = array(
      'view' => 'view all revisions',
      'update' => 'revert all revisions',
      'delete' => 'delete all revisions',
    );
    $bundle = $node->bundle();
    $type_map = array(
      'view' => "view $bundle revisions",
      'update' => "revert $bundle revisions",
      'delete' => "delete $bundle revisions",
    );
    ...
  }

Changing this out of scope for this issue here.

This patch here is RTBC again. But in combination with the commit of #2090983: ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified this afternoon my tests included here discovered another issue. I opened another issue. So we're currently blocked by #2579187: Revert to an older entity revision with less translations leads to fatal error caused by EntityStorageException :-(

Awesome, thanks for all the hard work on this one. Glad we were able to come to a good solution here!

Committed and pushed to 8.0.x. Thanks!

Content editors rejoice! Thanks folks!

I just realized that you gave me credit on this commit, thank you so much.
I want to thank you mkalkbrenner once again for the time you spent to explain me the issue and to make me step in Drupal contribution and D8, it was a great experience!
Following your discussions in this issue was very interesting too, thank you all for what you give to Drupal.

@cedric_a: thank you too!