Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations

This patch missed the required "use" statement and therefor didn't change anything for real ;-)
The patch and the interdiff in #7 include it.
And the test result in #7 is green as well.

This patch simple adds the NodeSaveTest::testPreSaveSetsOwner() to demonstrate the problem.

The Node::preSave() method should ensure a valid user on each translation of the node. But it's actually only executed on the translation on which you call save():

public function preSave(EntityStorageInterface $storage) {
  ...
  // If no owner has been set explicitly, make the current user the owner.
  if (!$this->getOwner()) {
    $this->setOwnerId(\Drupal::currentUser()->id());
  }
  ...
}

The test leads into a fatal error because the owner is not a valid object in all translations.

Now see how NodeSaveTest::testPreSaveSetsOwner() acts including the patch from #1

@Berdir
Basically I want to point out a problem that might cause strange problems in the future, especially in contrib.
I run into this issue when using the change tracking proposed by @alexpott in #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work. This doesn't work for translated entities:

public function postSave(EntityStorageInterface $storage, $update = TRUE) {
  parent::postSave($storage, $update);
  $this->resetChangedFieldList();
}

I'll try to explain why.
The patch by @alexpott introduces a new property for entities:

/**
  * The list of field names of any fields that have changed.
  *
  * @var string[]
  */
protected $changedFields = array();

If on purpose or not, this property is not one of the "synchronized" ones across the cloned translations in ContentEntityBase::initializeTranslation().
Therefor the tracking of changes in onChange() and the accessor getChangedFields() work separately on each translation (which is great BTW).

But as a consequence the resetChangedFieldList() method needs to called individually per translation!

If we keep the not obvious nature of methods like preSave() in combination with cloned translated entity objects we have two ways to fix #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work:
1. synchronize the changedFields property across translations in initializeTranslation()
or
2. loop over all translations in postSave()

Nevertheless, many contrib developers might step into that trap when they add properties to custom content entities. Especially if they don't think about translation up front.

@plach: Just to clarify, even if I'm already convinced that you understood my motivation for the patch :-)

The failing test provided in #4 doesn't make a lot sense to me ... this is not reliable, since that implementation may change, nor sensible, since if you wish to set the current user you can just specify the current user. If I were a core committer I would never allow such a test to be added to our test suite, since it would provide a bad example to developers.

I don't want to get the test included in the patch committed to core! I just looked for something in core to demonstrate the issue I see regarding DX and the potential bugs that might be caused by it.

From my point of view the next step should be to discuss how to solve the issue for real. I see these options:

1. Create a patch that at least documents the nature of these kind of callbacks in combination with entity translation. Additionally implementations of postSave() like in #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work need to be adjusted to iterate over translations. I don't think that we could provide a test that ensures that up front.
Additionally we should create an API to "register" properties for synchronization across translations or point out in documentation that initializeTranslation() has to overwritten to synchronize an additional property.

2. Extend the patch in #1 to call such callbacks on all translations. In this case we need to modify the existing implementations of preSave() and postSave() in core that expect to be called just once per entity. (In fact that are most of them!) They need to trigger a state by themselves that they only act once per $entity->save() or do a switch statement on the translation language if the multiple execution causes a problem or slows down the performance too much.

Both approaches have their own ugly drawbacks. But we have to ensure a better DX here to avoid bugs, especially in contrib modules. Does anyone have another / better idea to solve this issue?

Any suggestions how to proceed here?

I created a more readable failing test.

Here's a proposed solution to this problem as discussed with @catch on IRC:

I extended the documentation of EntityInterface:preSave() and EntityInterface:postSave() and modified the implementation of Node::preSave() accordingly to iterate over all translations.

This is one of the reasons why I was leaning towards having ::preSave() / ::postSave() (and hook_entity_update()) invoked on every translation object.

In the load/write workflow we act on every translation, as we did in D7, so I'd expect the system to do that consistently.

I would like to see that as well. But this means more code to fix in core. For example Node::postSave():

public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    // Update the node access table for this node, but only if it is the
    // default revision. There's no need to delete existing records if the node
    // is new.
    if ($this->isDefaultRevision()) {
      \Drupal::entityManager()->getAccessControlHandler('node')->writeGrants($this, $update);
    }

    // Reindex the node when it is updated. The node is automatically indexed
    // when it is added, simply by being added to the node table.
    if ($update) {
      node_reindex_node_search($this->id());
    }
  }

This code is not meant to be run for every translation. It needs to be wrapped like this:

public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    if ($this->isDefaultTranslation()) {

        ...

    }
  }

It would be really good to get some more opinions here.

Since #2566419: Using the "Autocomplete (Tags style)" widget for the author field does not save the submitted user has been committed, node's without a valid owner will be assigned to the anonymous user instead of the current user during save. Therefor I had to adjust the failing test to still demonstrate the architectural issue here.

And now the adjusted patch.

@hchonov is right. It seems like #2566419: Using the "Autocomplete (Tags style)" widget for the author field does not save the submitted user accidentally changed the strategy for the revision author fallback in Node::preSave(). I set it back to the state it was before.
So it seems there's a lack of test coverage for the revision author. But that isn't directly related to this issue here.

OK, the only error message in the single failing test is

Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/node/src/Tests/Migrate/d7/MigrateNodeTest.php on line 115
D

This test assumes that the node owner and the creator of a revision are identical. While this is correct for D7 where every translation is a single node, this is incompatible with D8!

In D8 every entity translation revision has its individual owner (like every node in D7). But the creator of a revision is a unique user ID, a non-translatable field shared across all translation. That's different from D7!
Migrate has to deal with that somehow.

The current patch just discovered that issue and it is not causing it.

I removed the "fix" for the revision Author that is not directly related to this issue here. We should open a folloe-up for migrate.

Shouldn't we also document why this difference exists?

I don't see any reason why it's implemented like this. I only went this way to just document the difference, because @catch told me to that in IRC. Maybe he should express why.

If anyone else is of the opinion that both APIs should behave the same way, that will be a pretty easy change.
Some existing implementations of preSave() in core should not run multiple times. But it will be a very simple patch to add a check for the default translation to each of them to avoid multiple execution.

How can we decide what to do? I already asked that multiple times ;-)

Instead of adding a todo I would like to see that change before RC1.

As @catch suggested I created #2577609: Add per-entity-translation pre/post save methods (and hooks?) so that preSave() and postSave() implementations don't need to iterate all translations for 8.1.x and added the todo to the patch.