Node revisions cannot be reverted per translation

1. +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,158 @@
   +    $language_name = t(\Drupal::languageManager()->getLanguageName($this->langcode));
   ...
   +    $language_name = t(\Drupal::languageManager()->getLanguageName($this->langcode));
   

   manager should be injected

2. +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,158 @@
   +    return t('Are you sure you want to revert to the revision from %revision-date for %language?', array('%revision-date' => format_date($this->revision->getRevisionCreationTime()), '%language' => $language_name));
   

   $this->t()

3. +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,158 @@
   +    $form_state->setRedirect(
   +      'entity.node.version_history',
   

   could be exposed as link template

4. +++ b/core/modules/node/src/NodeStorage.php
   @@ -29,6 +29,13 @@ public function revisionIds(NodeInterface $node) {
   +  public function revisionLanguages(NodeInterface $node) {
   

   needs @inheritdoc

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -162,16 +162,22 @@ public function revisionOverview(NodeInterface $node) {
-    $vids = $node_storage->revisionIds($node);
+    $vids = $node_storage->revisionLanguages($node);

revisionIds() is only now used by core/modules/quickedit/src/Tests/QuickEditLoadingTest.php:153

I have tested it successfully and code looks good.

I would be great to have input from either @plach or @berdir

@berdir in IRC said "plach is probably more qualified to review that" ... assigning to @plach

wow, very nice idea and solution to a definitely big problem.

I tested the UI and found one small issue:

I would not show any language in the current revision, as it could imply that the current revision only has one language, which is actually not true. And as we cannot do anything with this revision I would not show anything.

Another problem I've found:
the UI implies, that for every change we create a new revision. But you can edit nodes without adding a new revision:
1. Have a node in DE and EN (Revision Nr 2)
2. Edit EN, create a new revision (revision Nr 3)
3. Edit DE, do not create a new revision
4. Edit EN, create a new revision (revision Nr 4)
5. Edit EN, create a new revision (revision Nr 5)
Now the UI shows the Revision Nr 4 as "Language: English", but it also contains some changes to German.
If the user now reverts to version Nr. 3, his german changes are not lost (because we only revert the English translation) but it's still confusing as the revision list implies there are only english changes.
But actually I don't really know how to fix that, other systems like Workbench in D7 force the creation of new revisions whenever you like to have some workflow based systems. Would there be a way in the example above to show "English, German" in the language column for Revision 4?

The suggested approach looks sensible to me, my main concern is that in the current implementation we are changing the meaning of the langcode column in the revision table, which is meant to store the entity default language per revision. This would work only for the SQL entity storage, but the Mongo storage would save only one value for the langcode field, hence the current code would break.

To address this and #10, we could define an additional revision_translation_changed field (name TBD), which would hold the entity modification timestamp per language. Translation would need to be always enabled for this field, Content Translation would take care of that by not listing it in the settings UI, as it does for its own fields.

The new field would be stored in the field data and field revision tables as usual, and would allow us to track which translations were edited for each revision by simply comparing the revision translation timestamps with the revision timestamps. The drop-button in the revision list could be kept as it is, and in the confirmation form we could display a list of translations to be reverted with a checkbox for each one. I'd make them all checked by default.

Here's a very quick code review:

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -162,16 +174,22 @@ public function revisionOverview(NodeInterface $node) {
   +      $language_name = $this->t($this->languageManager->getLanguageName($langcode));
   
   +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,175 @@
   +    $language_name = $this->t($this->languageManager->getLanguageName($this->langcode));
   

   Language data is stored in config, hence to translate language names we should rely on config translation rather than string translation.

2. +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,175 @@
   +    $old_revision = $this->nodeStorage->loadRevision($node_revision);
   +    $latest_revision = $this->nodeStorage->load($old_revision->id());
   +    $tmp_revision = NULL;
   +
   +    if ($old_revision->language()->getId() == $langcode) {
   +      // Revert the default language.
   +      $tmp_revision = $old_revision;
   +      $translation_languages = $latest_revision->getTranslationLanguages(FALSE);
   +      foreach (array_keys($translation_languages) as $translation_langcode) {
   +        if ($tmp_revision->hasTranslation($translation_langcode)) {
   +          $tmp_revision->removeTranslation($translation_langcode);
   +        }
   +        $tmp_revision->addTranslation($translation_langcode, $latest_revision->getTranslation($translation_langcode)->toArray());
   +      }
   +    }
   +    else {
   +      // Revert a translation.
   +      $tmp_revision = $latest_revision;
   +      $tmp_revision->removeTranslation($langcode);
   +      $tmp_revision->addTranslation($langcode, $old_revision->getTranslation($langcode)->toArray());
   +    }
   +
   +    $this->revision = $tmp_revision->getTranslation($langcode);
   

   Why do we need all this processing in the build phase? Wouldn't it make more sense to perform it on submit?

3. +++ b/core/modules/node/src/Form/NodeRevisionRevertLanguageForm.php
   @@ -0,0 +1,175 @@
   +    // The revision timestamp will be updated when the revision is saved. Keep the
   +    // original one for the confirmation message.
   
   +++ b/core/modules/node/src/NodeStorageInterface.php
   @@ -24,10 +24,26 @@
   +   * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
   +   *   Use array_keys(NodeStorageInterface::revisionLanguages(NodeInterface $node)) instead.
   
   +++ b/core/modules/node/src/Tests/NodeTranslationRevisionTest.php
   @@ -0,0 +1,150 @@
   +      // Iterate through all the methods in this class, unless a specific list of
   +      // methods to run was passed.
   ...
   +          // Avoid to run these tests implemented in NodeTranslationUITest twice.
   

   80 chars limit exceeded

4. +++ b/core/modules/node/src/NodeStorage.php
   @@ -32,6 +32,16 @@ public function revisionIds(NodeInterface $node) {
   +      'SELECT vid, langcode FROM {node_revision} WHERE nid=:nid ORDER BY vid',
   

   We usually have spaces around operators.

5. +++ b/core/modules/node/src/NodeStorageInterface.php
   @@ -24,10 +24,26 @@
   +   * Returns a list of node revisions and their languages for a specific node.
   

   Missing blank line after introduction line.

6. +++ b/core/modules/node/src/Tests/NodeTranslationRevisionTest.php
   @@ -0,0 +1,150 @@
   +   * @inheritdoc
   

   Missing braces

Btw, it's true this functionality was available in D7 but this almost looks like a new feature to me.

Uff.. The title focusses on reverting a single language.
I would understand if one argues, this is a new D8 feature, as it was never seen in D8 before.

However, note that the revisions tab / looking at previous revisions is currently unusable in a setup with multiple languages enabled where translations happen.
The user is not aware of his current language context, but all output only represents the current language. If i compare two revisions (given that the newer added a translation into a new language) with the UI set to a different language, i see no change at all between the revisions.

Yes, revisions can not be limited to one language only. The best we can do with current design, is outline what languages are affected per revision. This, however, either needs comparision, or a per-language-per-revision timestamp as suggested by plach. We could then outline the language affected in the UI.
Alternatively, we could also just add a flag per revision per language that is 1 if a field changed in that language.

In my opinion, we should output the affected language column if we can.
As soon as you start offering this, a good UI would allow a user to filter revisions for a specific language. And similarly, allow reverting a certain language only.

The flag approach would make us most flexible since it can be used reliably for output without significant processing. It needs a lookup to the previous revision when saving though. This is permanent overhead though (for every save), since we never know if there are any previous revisions... See also #808730: Show the Revisions tab/page even when only one revision exists.

Yes, we just started to build workarounds for issues that are created because we did not manage to limit a revision to one language only. We need to be careful that this problem and related workarounds are not adding more complexity than fixing the original problem.

I would understand if one argues, this is a new D8 feature, as it was never seen in D8 before.

I second miro_dietiker that the issue discussed here is not a "new feature".

One clarification: I said this «almost looks like a new feature» because fixing this requires some significant UI changes. That's why I was trying to suggest an approach minimizing the impact on the UI (and why I didn't change the category to "feature" and postponed this to 8.1.x).

[...]
In my opinion, we should output the affected language column if we can.
As soon as you start offering this, a good UI would allow a user to filter revisions for a specific language. And similarly, allow reverting a certain language only.

These are all good points, and UX improvements are not frozen yet, so something like this would still be feasible. However It would be good to get some advice from the UX folks. What about providing a minimal fix for the bug reported here, that is preserving unaffected revision translations during reverts, and open a follow-up to implement a better UI?

Not sure about the flag approach, timestamps wouldn't require additional processing on save, I think, and determining which translations were affected for each revision should be feasible in a single iteration, while fetching revisions from the DB, so it shouldn't be a big overhead.

Yes, we just started to build workarounds for issues that are created because we did not manage to limit a revision to one language only. [...]

I'd be happy to be proved wrong, but IMHO the current approach (revision translations) is the only viable one, if we want to keep the complexity of the Entity API inside reasonable limits. Having translation revisions with the current architecture, where a single entity has multiple language variants, would mean having per language revision ids, and getting rid of per-language ids is one of the main reasons I proposed the translatable fields model in the first place. This is certainly one of the cases clarifying that picking a single translation model forces us to accept some tradeoffs, however all considered I think pros still outweigh cons. The solution proposed by @mkalkbrenner is simple and elegant and, once it's coupled with a proper UI to support it, I don't think we will have significant regressions in the user-facing functionality.

From my point of view there's a big mistake in the architecture regarding revisions and translations - like miro_dietiker described.

From my POV it's a tradeoff and not a mistake, but I'm not very interested in arguing about this, unless you can suggest an alternative solution that fixes this use case and keeps all the rest working, in which case I'm all ears :)

I thought about a field to store translation specific timestamps on changes.
I recognized the columns "changed" in the tables "node_field_data" and "node_field_revision".
My first assumption was that this one could be simply used for our needs. But I realized that there's always the same value stored across all languages - no matter which translation has been changed really.

The reason why I proposed a separate field is twofold:

• We need to support any translatable and revisionable entity type, not just nodes, so we cannot rely on the changed field. I expect the revision UI to be generalized much like the translation UI and be applied to any revisionable entity type sooner or later, so we need to be prepared for that, even if Node is the only entity type currently exposing a revision UI, at least in core.
• The changed field might or might not be translatable (#2428795: Translatable entity 'changed' timestamps are not working at all looks like a bug to me), but we need a field that's always marked as translatable in this case.

We need to support any translatable and revisionable entity type, not just nodes

We started a first approach are pretty near to to build a revision tab for all entities in the diff module. yay!
#2452523: Offer a revisions tab for all entities

This is certainly one of the cases clarifying that picking a single translation model forces us to accept some tradeoffs

There are many relevant trade offs, as pointed out in my blogpost. The most concerning one for me is that with every single update of a language, all translations are written. This simply results in severe scaling / performance issues with platforms containing many languages. Combined with inline editing and enabled revisioning, this is going to be fun. The current cross language locking (if multiple translators add translations at the same time) is still circumventable technically.

A customer's perspective

On osce.org we are working with about 100,000 entities in default language English and entity translations in over 50 languages. Translations are usually done by separate editors, not working in parallel, but in general after the source translation - which may also be Russian, Armenian etc. - has been completed. There is no link or common revision between translations, each translation has its own workflow, pitfalls, false friends and typos.

Having dependent revisions in Drupal 8 would force us to think to upgrade to another CMS. IMHO the demand for such a feature is very strange. I assume that people who have only worked with a single language, e. g. English, can come up with such an idea.

Guys, please don't overdo things here. Multilingualism has matured a lot in Drupal 6 & 7, just stick to it.

As @plach said, revisions per language would re-introduce per-language identifiers, which come with their own problems.

If you have 50 translations, then you will have different problems. All 50 translations are stored and loaded as part of the same entity. Obviously, that would be a huge amount of data.

Yes, the new translation model in core doesn't really work for that. It doesn't have to. There is AFAIK already a project that wants to support a translation model that is similar to the old translation module and uses translation sets. If that works better for your use cases, use that. Problem solved.

There is no chance that the core content translation module or content entity storage will change completely in Drupal 8.

If you have 50 translations, then you will have different problems. All 50 translations are stored and loaded as part of the same entity. Obviously, that would be a huge amount of data.

That sounds scary and like a questionable design. It's like if you are just inviting a single friend to your party bringing his 50 cousins with her/him. It might be fun, but also crowded in your small apartment.

I am not aware of the code details and why this heavy loading has to be done, but why don't you introduce some lazy loading? Translation pointers which are loaded on demand? Or are you also storing 50 translations in a single Word document file?

If you want to group translations you should have translation sets. Overall I smell here an anti pattern God Object

There is no chance that the core content translation module or content entity storage will change completely in Drupal 8.

I guess that includes any 8.x version not just 8.0.x.
All the more important that something like the above per-language reversion workaround gets in.

We started a first approach are pretty near to to build a revision tab for all entities in the diff module. yay!

Great news :)

There are many relevant trade offs, as pointed out in my blogpost. The most concerning one for me is that with every single update of a language, all translations are written.

Well, we did not get to that yet, but having a proper Entity API baked in core will allow us to implement some cool optimizations. One of them is writing to the DB only the pieces of the entity that actually changed. This would solve your concern, I guess. IIRC in your article you were also pointing out that having many translations in a single entity object may result in memory issues. This might be trickier to address, but I think we could experiment with lazy loading entity fields per language. This would require some tweak to the entity caching logic, but I think it would be feasible. OTOH having all field data in all languages readily available is very useful to provide efficient language fallback, which we could not have with the node translation model, above all when listing entities. Hence I guess this could be an optional behavior, or even an alternative entity cache backend to be used only in very specific conditions.

The current cross language locking (if multiple translators add translations at the same time) is still circumventable technically.

Yep, we certainly need to improve that, but it's definitely feasible.

On osce.org we are working with about 100,000 entities in default language English and entity translations in over 50 languages. [...]
Having dependent revisions in Drupal 8 would force us to think to upgrade to another CMS.

Well, the current approach does not necessarily imply having dependent revisions, as @mkalkbrenner has just demonstrated with his patches. We simply did not get to make every single bit work yet. Help is more than welcome.

I assume that people who have only worked with a single language, e. g. English, can come up with such an idea.

Actually I worked almost only on multilingual projects for many years, however TBH none of them had 50 languages installed. OTOH this scenario is definitely not the most common, so not the one the default Drupal configuration should target. I'd like to point out that the current system is fully compatible with the D6/D7 translation sets approach: reimplementing it can be done in one working day, and AFAIK the Translation project aims to do exactly that. If even with every possible optimization, the core approach did not fit your use case, node translation will still be there to serve you and anyone else needing it :)

Guys, please don't overdo things here. Multilingualism has matured a lot in Drupal 6 & 7, just stick to it.

Thanks for the suggestion, I'm sure D8MI will benefit a lot from it.

OK, but what does that mean for the approach discussed here which is usable if you use only a few different languages?

I don't think so: what we have in core is the evolution of the D7 Entity Translation project, that is being used right now in very high profile sites with many languages installed. I'm confident that most of the issues we have identified can be successfully solved.

That sounds scary and like a questionable design. [...]
I am not aware of the code details [...]
If you want to group translations you should have translation sets.
Overall I smell here an anti pattern God Object

I'm trying hard to take this comment seriously, and consider it an attempt to provide a constructive criticism, but do you realize there are literally years of work and discussions behind the current implementation?

@all:

I don't think this is the right issue to rehash the revision/translation topic in general (let alone discussing the core content translation model). It would be great if we could create a meta issue listing all the concerns raised so far about the revision/translation topic, and possibly already create the related sub-issues.

Now can we please focus on implementing what's needed to fix the issue reported in the OP, possibly punting any UI improvement to a separate major task?

I agree with @plach. Most components of the Drupal 8 multilingual system are not brand new at all, they evolved from existing Drupal 7 solutions. http://drupalwatchdog.com/volume-4/issue-1/drupal-babel has a comparison table of modules in Drupal 7 vs. core features we built-in. The content translation solution in Drupal 8 is especially built based on the experience we had with title module, workbench and entity_translation. I would be among the first to readily admit that not all aspects are fully covered, but (a) Drupal 8 is not yet released (b) now is your time to help out to cover your use cases. One way to help out is to help implement https://www.drupal.org/project/translation if you are interested in having a translation set option with different entities. We the entity translation method for core in part because that is more flexible in terms of configurability and especially compatibility with contributed modules (which historically had problems being compatible with both translation methods).

Thanks for the clarification, fair enough.

It wasn't clear from your posts that the patch of mkalkbrenner and other performance improvements have a chance to be accepted. That communication problem being sorted out I will have a look if I can help out.

Thanks! I will have a look to this later today

Another possibility to solve this is to make revert fill in an edit form with the old revision of the entity. We use this approach (indirectly) for the CPS module (by design).

That allows to keep UI changes small and just bases reverts off an older entity. Still need a little special code to only replace those fields with data from the revision that is different and only for the loaded language, but then the language is explicit.

Just tossing in as a suggestion.

On the performance and memory concerns:

- Performance fixes in general are at the moment very viable and a lot of lazy loading could be done without any API changes, so can happen in any version of Drupal 8.
- Given the decoupled nature of Drupal 8, adding lazy loading for multiple languages within an entity is also feasible as all uses proper getter() and setter() now

Also optimizing storage writes is a feasible possibility without any API changes needed.

And given enough interests / use cases this might / will one day be implemented.

But of course if you chime in with help, it will get done faster.

That communication problem being sorted out I will have a look if I can help out.

Sounds good :)

Another possibility to solve this is to make revert fill in an edit form with the old revision of the entity. We use this approach (indirectly) for the CPS module (by design).

This is an interesting solution, however it's not very common to end up in an edit form when trying to revert a revision, at least in my experience. This could use some UX expert feedback, that's why I'm suggesting to provide a minimal fix here, so that the functionality is not completely broken, and discuss a more complete solution in a separate task.

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -104,6 +104,12 @@ public function getFieldDefinitions() {
   +    $definitions['content_translation_edited'] = BaseFieldDefinition::create('timestamp')
   

   As I mentioned in #2428795-9: Translatable entity 'changed' timestamps are not working at all, this field cannot be defined by the Content Translation module, because the API allows to create/update translations programmatically without needing CT to be installed. It should be provided by the Entity Manager like we do for the default_langcode field.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -537,6 +543,7 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
        $metadata->setChangedTime(REQUEST_TIME);
   +    $metadata->setEditedTime(REQUEST_TIME);
   

   We don't need two methods, we just need to make ::setChangedTime() use the new field.

3. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapper.php
   @@ -75,6 +75,21 @@ public function setOutdated($outdated) {
   +  public function getEditedTime() {
   ...
   +  public function setEditedTime($timestamp) {
   
   +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapperInterface.php
   @@ -56,6 +56,24 @@ public function isOutdated();
   +  public function getEditedTime();
   ...
   +  public function setEditedTime($timestamp);
   

   As a consequence, these become redundant.

4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -550,6 +557,26 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +      $metadata->setEditedTime($metadata->getCreatedTime());
   ...
   +      $metadata->setEditedTime(REQUEST_TIME);
   

   This should be handled by the entity storage handler directly, we cannot rely on a form workflow.

5. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -162,7 +174,12 @@ public function revisionOverview(NodeInterface $node) {
   +    $header[] = $this->t('Revision');
   +    if ($node->isTranslatable()) {
   +      $header[] = $this->t('Languages Edited');
   +    }
   +    $header[] = $this->t('Operations');
    
   

   As I was saying above, please, let's try not to rush a new UI in. A dedicated issue where we can get proper UX reviews would have way more chances to obtain a better result (and be accepted by core committers). Can't we just assume that all the affected languages need to be reverted for now? This would allow us to skip any UI change.

6. +++ b/core/modules/node/src/NodeStorageInterface.php
   @@ -28,6 +28,17 @@
   +  public function revisionAffectedTranslations(NodeInterface $node);
   

   We normally use a "get" prefix for this kind of methods, what about getAffectedRevisionTranslations()?

Created meta, feedback welcome.

This is actually postponed on #2428795: Translatable entity 'changed' timestamps are not working at all.

The issue summary needs to be updated with the proposed resolution.

Specifically, I could not find an answer to the following question in the patch, although I must admit I didn't spend a super large amount of time on this: The normal changed field is already marked translatable, so why can't we use that and make sure that it's updated correctly for each translation. Semantically I don't see any difference, but again, I am probably missing something.

Ahh OK thanks for that clarification, I will have a look.

#2428795: Translatable entity 'changed' timestamps are not working at all was committed

Sorry, I had to read again the whole issue and the related ones to refresh my memory and I wasn't able to do a proper review.

I'm a bit confused because I thought we agreed to provide a minimal fix here, and instead the patch seems to do way more than what's strictly required to make revision reverts work with translations. It's clear you spent a lot of time dealing with real-life issues (this is hugely helpful, thanks :), but I'm having a hard time understanding the big picture.

I thought we agreed on using the (translatable) changed timestamp to figure out which translations were affected for each revision, but I guess the fact that two revisions can have the same timestamp kind of make this approach not viable. It would be great if you could summarize what's the currently implemented solution, so we can discuss it a bit before I go back reviewing the code.

A few things that concern me are:

• We are hardcoding more and more logic into field item plugins: as wonderfully summarized by @yched in #2478459-39: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks, using field items as "behavior units" rather than plain "data modelling units" is not the currently recommended way. Do we really need that?
• We are relying on a field provided by Content Translation, but this functionality needs to work even if CT is not enabled, as translations can be created programmatically. I know of a high profile D7 site that does exactly that with Entity Translation.
• We have a lot of UI code in the patch: can we leave the UI unaltered and just revert all the affected translations for now? UI improvements should be the goal of #2465907: Node revision UI reverts multiple languages when only one language should be reverted.

@mkalkbrenner:

Thanks for the very detailed explanation :)

I had another look to the last patch and manually tested it to get a better sense of the implementation details. The end result is great, although I have some minor doubts about the final UX, which is why I think we should move all the UI code to #2465907: Node revision UI reverts multiple languages when only one language should be reverted. It's completely fine to keep posting patches including dependencies over there, as we did here so far, if we want to ensure all tests are still green. However this will make focusing on the low-level stuff here easier.

Aside from that, I still have some doubts about the actual implementation:

I choose ContentTranslation because that's where it's ensured that a ChangedItem is added to an entity. If we want to move the revision pointer to ContentEntity we need to be consequent and require a ChangedItem at this layer as well if the entity is revisionable.

Actually all revisionable entity types need to manually define revision metadata fields to work correctly, so adding a couple of further requirements does not really change the overall situation. Revision and translations cannot work properly if we don't have a changed field and a revision pointer, so we need to make them required, no point in trying to get around that. What we could to loosen these constraints could be falling back to the current core behavior (i.e. reverting all languages) if one of the two fields is not defined. In this scenario, revisionable and translatable entity types will be required to define an additional (translatable!) revision metadata field, e.g. revision_last_affected (instead of content_translation_revision). Name to be defined, but I'd prefix it with revision_ as we do that for all the other ones.

In this scenario, I think we no longer need the ContentTranslationRevisionPointerItem: revision and translation business logic are the domain of the core (Content) Entity API: they both are natively handled by all the Entity API subsystems, thus properly handling revision translations should be part of that. Specifically, properly calculating/storing the revision pointer values would be the storage handler's responsibility. Additionally, moving that logic in the storage layer may make things slightly easier to implement.

Going further with this scenario, we may not even require the changed field: if I'm not mistaken, right now we are using it just track whether an entity (translation) object has changes wrt to its stored version. However it seems that the logic currently implemented in ChangedItem::hasEntityChanges() has nothing specific to the changed item, and could easily live in the (Content) Entity object itself. In this case, the storage handler should have everything it needs to calculate and store the revision pointer value.

What do you think?

Not too familiar with the contents of the patch, but @plach's #66 sounds appealing. +1 to anything that lets us move behavioral logic out of specialized subclasses of 'timestamp' field type.

Coding behavior / value in the field type is an antipattern we should try to avoid.

Spoke with @catch about this during the critical issues call. The proposed solution will have an impact on the upgrade path, and I'd really like not to have to provide an upgrade for this. Since the bug is borderline critical, we agreed to make it tentatively critical, so maintainers will have a chance to discuss it in the next triage round and decide whether it actually is.

Assigning to catch because we need his feedback also on the proposed solution (see #65 and following).

OK so there are two things to consider regarding whether this is critical or not:

1. The inadvertent data-loss between translators when reverting revisions. This is similar to #2484037: Make Views bulk operations entity translation aware. The difference here is that the old revision information isn't completely deleted, but it's not possible without manual editing/comparison to get both translations to the desired state. I don't personally have a strong view on this.

2. If it is critical, then we should try to fix it before we start supporting the upgrade path officially, since the upgrade path required here will be non-trivial and could end up delaying the patch overall and hence the release. I'd really like to avoid that extra work.

So I think we need to decide now that the issue is critical and get it fixed before the upgrade path is officially supported. Or we can decide it's major - and possibly too disruptive to commit to 8.0.0 and defer to 8.1.x. But what we definitely shouldn't do is decide this is critical after supporting the upgrade path, since that's adding extra scope compared to the proposal.

#66 also appeals to me as a solution. - standardizing across revisionable/translatable entity types is fine - and better to have a couple of metadata fields required if that simplifies the handling.

Thanks @catch, +100 on #69. Personally I think we should do this before we release 8.0.0. and thus we should keep this as critical, but I may be biased :)

@mkalkbrenner:

If you are ok with #66 too, we could have this RTBC pretty quickly, since that would mean mostly moving some code around, and then have it ready before the upgrade path is. At that point it doesn't really matter whether this is critical or not, because it would no longer be an upgrade path blocker.

To be clear: I can make this my current top priority, but we need to agree on a way forward.

As mentioned in #72 I'm fine with your approach in #66!

Sorry, somehow it wasn't completely clear to me :)

Even more if you help implementing it to get it RTBC soon :-)

Well, I could certainly work on this, but then we'd need to find someone to review/RTBC my patches. If you have time to work on this, I think it will be faster if you work on #66 and let me review the result as soon as it's ready. If you are not planning to work on this soonish, let me know and I'll try to move it forward during the weekend.

It should be just matter of manually adding a revision_last_affected (or whatever it will be called) base field definition to:

• BlockContent::baseFieldDefinitions()
• Node::baseFieldDefinitions()
• EntityTestMulRev::baseFieldDefinitions()
• EntityTestMulRevChanged::baseFieldDefinitions()

I think we should move ChangedItem::hasEntityChanges() to a new ContentEntityInterface::hasChanges() method (we can move it to EntityInterface in a separate issue, as that would be BC change). And then we should move the code that's currently implemented by ContentTranslationRevisionPointerItem in ContentEntityStorageBase.

And move the UI changes to #2465907: Node revision UI reverts multiple languages when only one language should be reverted :)

CNW to split the patch.

I'll give #80 a try.

@mkalkbrenner thanks!

1. +++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
   @@ -37,4 +37,21 @@ public function getChangedTime();
   +
   +  /**
   +   * Determines if the entity has changes.
   +   *
   +   * @return bool
   +   *   TRUE if the entity has changes, FALSE otherwise.
   +   */
   

   Sorry, probabaly I didn't explain myself clearly. The ::hasChanges() method does not need to be tied to the changed timestamp: any entity can have changes, because it can differ from the stored version. This method should live on ContentEntityInterface, and should be moved up the hierarchy tree into EntityInterface in a followup implementing it also for config entities (which should be easier).

2. +++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
   @@ -37,4 +37,21 @@ public function getChangedTime();
   +  public function hasChangesAcrossTranslations();
   

   I'd avoid generalizing the ::*AcrossTranslations() pattern, as then we'd needed it everywhere. After all looping on the available translations is quite easy and natural.

3. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -28,4 +28,76 @@ public function getChangedTimeAcrossTranslations() {
   +        // The originalEntity will be reset by $this->preSave().
   ...
   +    $language_id = $this->language()->getId();
   +    // @todo How to detect the right field name? Or is the hard-coded value
   +    //   sufficient for a trait?
   +    $changed_field_name = 'changed';
   +    $changed_field = $this->get($changed_field_name);
   +    $translatable = $changed_field->getFieldDefinition()->isTranslatable();
   +    if ($translatable) {
   +      $original = $original->getTranslation($language_id);
   +    }
   

   If we don't invoke the ::hasChanges() method from within a save process (or we do before ChangedItem::preSave() is invoked), the following logic does not need the changed field: if any field has changes, the entity (translation) object has changes.

   #60 is suggesting to calculate which translations have changes for the current revision (and update the revision pointer field) in ContentEntityStorageBase exactly for this reason: if we override EntityStorageBase::save(), compute the revision_last_affected value, and then invoke the parent ::save() method, we should be able to have our values in place before ChangedItem has a chance to do any harm.

4. +++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
   @@ -28,4 +28,76 @@ public function getChangedTimeAcrossTranslations() {
   +        // The originalEntity will be reset by $this->preSave().
   

   Is this still true?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/RevisionPointerItem.php
   @@ -0,0 +1,47 @@
   +class RevisionPointerItem extends IntegerItem {
   

   And thus this can go away, which lets us not depend on #2478459: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks and avoid a rewrite.

I made a few "on-paper" tests to verify this:

The implementation you might have in mind is about an absolute decision whether an entity has any changes compared to the stored one or not.
From a revertable translation revision only perspective you have to exclude various fields from that implementation, for example the non-translatable fields.

It came quite natural to me to flag things the way Miro was suggesting in #13. Discussing with @mkalkbrenner pros and cons of the two approaches.

I had a long discussion about this with mkalkbrenner and hchonov, but no final conclusion yet, here's the log.

Discussed with plach, and did a very superficial review of the patch.

I think I'd keep this in the storage layer. This is specifically tracking whether something we've stored has changed internally. Revisions and translations are central to the entity system. Contrib doing special handling has access to field types and hooks, so baking this in doesn't stop them doing what they need to for other kinds of changed tracking.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1012,4 +1013,60 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
+  /**
+   * {@inheritdoc}
+   */
+  public function hasChanges() {
+    static $original = NULL;
+
+    if ($this->isNew()) {
+      return TRUE;
+    }
+
+    if ($this->original) {

This should also check untranslatable fields, otherwise it will report that things haven't changed when they have.

We can discuss the name of the function but if it checks for untranslated fields it's unusable to solve our issue here.

I don't think we should have a public method that only checks translatable fields.

All we need is when saving a translation revision, to see a boolean whether it changed or not - that's something we should be able to do in a protected method somewhere without anything in the public API.

Nevertheless the translatable changed timestamp is an important information that will later be used to build a sophisticated UI or workflow modules in contrib.

That's fine but I think anything to do with this should be split out to another issue, it's very confusing trying to determine what's dealing with setting the translation changed boolean vs. the timestamp and which should be used where.

I've just read again the logs of all our previous discussions and I think I've changed my mind on a point that might allow us to reach a consensus on the way forward. I wanted to speak with @catch about it but I wasn't able to, so we still need his approval.

What is bothering me about the current approach is introducing a public API method named ::hasChanges() inspecting only translatable fields. Additionally I was not happy with ChangedItem automatically updating its value only when translatable fields are changed.

After inspecting some core use cases, I realized that there is no way to hard-code a single behavior that would work in all cases. Given that, I think now I'm fine with the current ChangedItem behavior, since it is possible to manually set a different value for it depending on the use case. What would be horribly wrong would be hard-coding the current behavior without the possibility to override it.

That said, I think this could be a way forward:

• Define a new revision_translation_affected boolean translatable field.
• Move the logic in ChangedItem::preSave() to a new ContentEntityBase::hasTranslationChanges() method (and define ContentEntityInterface::hasTranslationChanges()).
• Populate $entity->revision_translation_affected in ContentEntityStorageBase::save() and invoke EntityStorageBase::save() subsequently. To make sure the flag value can be manually overridden we should populate it only if its current value is NULL, which should be the initial value for all flag translations after a new revision is created.
• To avoid performing ::hasTranslationChanges() twice, we could statically cache its value and invalidate it in ContentEntityBase::onChange().

I think #113 works for me, but I'm not sure why hasTranslationChanged() in the last bullet would be called twice? Is this just in case some other code calls it?

I think that's the explanation if we simply follow the bullets in #113 ;-)

Yep, that's what I had in mind.

Both approaches will have the same impact on performance. Thoughts?

The idea behind caching/invalidating on ::onChange() is not assuming a workflow: since ContentEntityInterface::hasTranslationChanges() could be invoked in any context, it should be re-calculated as soon an entity field value changes.

Just had this chat with @catch in IRC:

catch plach : so with the translation check static caching, could we not put an additional, temporary property on the entity?
catch And then check if that is set?
plach catch : wouldn't that be exactly the static caching I was proposing?
catch plach : if that's what you meant by the static cache, then yes :P
plach catch : sorry; I should have been more explicit :) I meant we'd have a $translationChanged protected property on the entity object that would store the check result and the would be reset to NULL in ::onChange()
catch plach : OK so yeah sounds fine :)
plach catch : however, I think I found a problem with my suggestion
WimLeers|afk is now known as WimLeers
plach catch : if any code running in ::preSave() methods or presave hooks implementation changes the entity we wouldn't record the translation as affected, becuase the related field has already been popoulated
catch plach : good point
plach catch : the only reason I was suggesting to do that *before* pre-save was to that the ChangedItem would always be changed
plach either manually or automatically
plach so every translation would always be marked as affected
plach catch : however if ChangedItem is not updated if there are no actual changes, then it should be safe to populate the field *after* the pre-save phase
plach catch : does it make sense?
catch plach : so presave either sets it to true, or something could set it to FALSE explicitly.
catch plach : and after presave, if it's null, set it to TRUE?
plach catch : the flag field? yes
plach (revision_translation_affected)
catch plach : bit distracted with crying baby but sounds OK
plach ok :)

@mkalkbranner:

So, with the corrections above to #113, I'd say we have a plan ;)

+++ b/core/modules/node/src/Entity/Node.php
@@ -494,6 +494,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['revision_last_affected'] = BaseFieldDefinition::create('changed_flag')
+      ->setLabel(t('Revision last affected'))
+      ->setDescription(t('Indicates if the last edit belongs to current revison.'))
+      ->setReadOnly(TRUE)
+      ->setRevisionable(TRUE)
+      ->setTranslatable(TRUE);
+

Given #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29 do these need update hooks, or is the plan to get this in prior to June 29?

do these need update hooks

Possibly not, even if this lands after June 29th, and even if it's not granted special exemption.

Because entity schema changes are run by update.php automatically. The code for that is update_entity_definitions(), and DbUpdateController::triggerBatch() makes it run prior to hook_update_N() functions, which is good, because it means you can also have a hook_update_N() that loads and saves entities and it will do so with the correct database schema already in place.

However:

• This patch adds a ChangedFlagItem field type, but update.php does not appear to clear the discovery cache prior to checking what updates are available. That's not this patch's fault. If we don't already have an issue for that, we should add one. Until that, in order to test an install of HEAD followed by applying this patch followed by running update.php, you need to clear cache between applying the patch and running update.php.
• If you already have existing nodes or custom blocks, then update.php will initialize the new revision_translation_affected column with a value of NULL for those existing entities. I did not test whether the patch then works correctly with that as the value. It would either need to be able to handle that, or else the field definition needs to be given some other default/initial value, or else we'd need a hook_update_N() that loads each entity, gives the field a value that is correct for that entity, and saves that entity. That last option, however, would have serious scalability problems for a site that has millions of entities, so if it needs to be a calculated value, perhaps better would be to calculate as part of saving a revision rather than needing to do it all during the update?

Given the above, I also think it wouldn't be catastrophic if this didn't make it into 8.0.0 and ended up getting fixed in 8.1.0. So, I question whether it should remain a Critical. I'm happy for it to make it into 8.0.0 if it does, but I don't think it's worth holding up 8.0.0's release on it.

Aside from the stuff below, I think this is as good as it gets with the field type approach. I'd still like to explore the no-field-type approach, though :)
I will have a try tomorrow.

Also, for this to be complete we should make node reverting actually work. Without introducing any UI changes I think it should be easy to revert all the affected translations. This would fix the worst part of this issue, and if we weren't able to complete all the other stuff in the meta before 8.0.0 is released, we would still have a functional UI, although less than optimal. However improvement would definitely be possible in 8.1.0.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1012,4 +1014,45 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +    static $original = NULL;
   

   This looks very fragile: I'm wondering whether we can avoid this by checking for the existence of translations, as pointed out below.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1012,4 +1014,45 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +    $translatable = $this->isTranslatable();
   

   I think what we really want to know is whether the entity is translated, because all this behavior is useless on translatable but not translated entities. So what about:

   $translated = count($this->translations) > 1;
   

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1012,4 +1014,45 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +    $translation = ($translatable) ? $original->getTranslation($this->language()->getId()) : $original;
   

   If I get the comment above right, we modify $this->original when we try to instantiate a translation that was not originally stored. Would the following work?

     public function hasTranslationChanges() {
       /** @var static $original */
       $original = &$this->original;
   
       if ($this->isNew()) {
         return TRUE;
       }
   
       if (!$original) {
         $id = ($this->getOriginalId() !== NULL) ? $this->getOriginalId() : $this->id();
         $original = $this->entityManager()->getStorage($this->getEntityTypeId())->loadUnchanged($id);
       }
   
       $translated = count($this->translations) > 1;
       if ($translated && !$original->hasTranslation($this->activeLangcode)) {
         return TRUE;
       }
   
       /** @var static $translation */
       $translation = $translated ? $original->getTranslation($this->activeLangcode) : $original;
       foreach ($this->getFieldDefinitions() as $field_name => $definition) {
         if (!$definition->isComputed() && (!$translated || $definition->isTranslatable())) {
           $items = $this->get($field_name)->filterEmptyItems();
           $original_items = $translation->get($field_name)->filterEmptyItems();
           if (!$items->equals($original_items)) {
             return TRUE;
           }
         }
       }
   
       return FALSE;
     }
   

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1012,4 +1014,45 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   +    foreach ($this->getFieldDefinitions() as $other_field_name => $other_field_definition) {
   

   Can we rename these to $field_name and $definition? See above.

@eff:

Given the above, I also think it wouldn't be catastrophic if this didn't make it into 8.0.0 and ended up getting fixed in 8.1.0. So, I question whether it should remain a Critical. I'm happy for it to make it into 8.0.0 if it does, but I don't think it's worth holding up 8.0.0's release on it.

I'm not sure I agree with this conclusion: supporting an upgrade for this would be way worse in 8.1.0, because every 8.x website out there would be affected by the upgrade. Instead if we do this before 8.0.0 only a (way) smaller set of sites would be affected.

Ideally I would like this to be fixed before supporting the upgrade path: you are right the schema would be updated automatically, but updating data would be tricky because we cannot rely on the Entity API in update functions and thus we could provide an update function only for the default SQL storage.

It would be nice if could provide an self-healing behavior, so that field values would be fixed by just saving entities, or at least we could provide some sort of fallback behavior if data for affected translations is missing, like reverting all translations as we currently do in HEAD.

I'm fine with clone, I was scared by the static. What about the node UI?

Also, for this to be complete we should make node reverting actually work. Without introducing any UI changes I think it should be easy to revert all the affected translations. This would fix the worst part of this issue, and if we weren't able to complete all the other stuff in the meta before 8.0.0 is released, we would still have a functional UI, although less than optimal. However UI improvements would definitely be possible in 8.1.0.

It would be nice if could provide an self-healing behavior, so that field values would be fixed by just saving entities, or at least we could provide some sort of fallback behavior if data for affected translations is missing, like reverting all translations as we currently do in HEAD.

So I think this would probably be fine as an 'upgrade path' for existing data - since reverting is not that common an operation, only goes back one or two revisions usually, and the data is there, just not very accessible.

If we do that, then the upgrade path here in general looks much less scary, and I'd agree with effulgentsia that this could go back to major (since we don't actually lose data here, just bury it a bit). I'd also be much happier committing it any time up to RC in that case. However, would really like to be sure about that before downgrading, since having to upgrade this to critical later and write the migration path for all the old data after all would not be nice.

This patch builds on top of #131 but does not rely on new field types. As agreed during our last discussion, it implements a minimal fix for the node revision UI to cover the use case described in the OP and provides test coverage for that.

Notes:

• I removed the code implementing manual setting detection for field item values: I think it's a valid approach but it could use a dedicated issue to allow the key people (@yched, @fago, @berdir) to review it and discuss the implementation details. We could repurpose #2329253: Allow ChangedItem to skip updating the entity's "changed" timestamp when synchronizing for that. Meanwhile I special-cased a couple of fields to make things work, I think this is fine, since fixing that will be just an internal change.
• I was able to remove the various clones: not sure whether that indicates a lack of test coverage of whether I was able to find a proper fix.
• I changed one assertion in the existing test coverage:
  

      $this->assertTrue(
        $this->getRevisionTranslationAffectedFlag($entity),
        'Changed flag of original language is set also for new revision without changes.'
      );
  

  The reason is that to improve performance for untranslated entities we now always set translations as affected. I think this is an acceptable change, but obviously totally open to discuss that.

• This also makes sure the revision_translation_affected field cannot be configured in the Content Translation admin UI.

Some comment clean-ups

Interdiffs look good to me, thanks :)

@plach: does your approach bring back the requirement for API changes to solve #2329253: Allow the ChangedItem to skip updating, or do you already have a nice solution in mind? We need it for both. The flag and the timestamp.

Well, if the final solution is something along the lines of the removed code, it should be a completely internal change. Maybe it would imply an API addition, but that should be fine.

I'm now happy with the patch, but can no longer RTBC it. I pinged @catch, hopefully he can review it.

Looks great to me.

Let's get this in then :)

never too early to write a change record

;)

Markus just created it, back to RTBC!

https://www.drupal.org/node/2513894

Displayed again the failing test-only patch that demonstrates that the new test coverage captures the reported bug.

@mkalkbrenner will certainly not be able to commit it.

A couple of minor nits.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -26,4 +26,33 @@
   +  /**
   +   * Marks the current revision translation as affected.
   +   *
   +   * @param bool $affected
   +   *   The flag value.
   +   */
   +  public function setRevisionTranslationAffected($affected);
   

   We pass a null in here - also lets return $this to make the interface fluent.

2. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -138,6 +138,28 @@ function _content_translation_form_language_content_settings_form_alter(array &$
   +// Allow to configure only fields supporting multilingual storage. We skip our
   +// own fields as they are always translatable. Additionally we skip a set of
   +// well-known fields implementing entity system business logic.
   +return
   +  $definition->isTranslatable() &&
   +  $definition->getProvider() != 'content_translation' &&
   +  !in_array($definition->getName(), [$entity_type->getKey('langcode'), $entity_type->getKey('default_langcode'), 'revision_translation_affected']);
   

   Needs to be indented 2 spaces.

Addressed #154, thanks.

Updates look good. I first understood the phpdoc was not updated but its on the interface lower down in the interdiff.

Getting this in sooner rather than later so that it's in before the next beta.

Committed/pushed to 8.0.x, thanks!

Yay, awesome work @mkalkbrenner!

Thanks all, especially @mkalkbrenner and @plach!

We have a side by side language1 / language2 interface, our client is used to pressing 'save' once for all translations and would like to revert to a revision for all translations.

For the revisions form I'm thinking of adding a form alter and a checkbox option for 'All Translations at once'

has anyone else already thought of this?