Add TranslationStatusInterface to ask for the status of an entity translation and fix statuses in ContentEntityBase

Reviewing the issue, I did not find anything that does not sound like oversights or where you are not fixing inaccuracies. Keep in mind I am not very well versed in the internals of this code, but the changes look very logical. Indeed tests are still needed.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -857,6 +853,16 @@ public function removeTranslation($langcode) {
+    if ($langcode ==  $this->defaultLangcode) {

Nitpick: spacing.

The biggest concern is changing an existing interface in core could very well be problematic for backwards compatibility. I think we ideally want to have a new interface that inherits from the old one and deprecate the old one. Otherwise this patch may start to WSOD contrib modules, which does not satisfy backwards compatibility.

+1 for the new feature!

I think you should create a change record for the new API and point out that the move of the constants is backwards compatible.

Beside that => RTBC

Repeating myself from two months ago:

The biggest concern is changing an existing interface in core could very well be problematic for backwards compatibility. I think we ideally want to have a new interface that inherits from the old one and deprecate the old one. Otherwise this patch may start to WSOD contrib modules, which does not satisfy backwards compatibility.

@hchonov: you are adding a new method. Consider this:

1. core provides an interface
2. contrib implements the interface
3. core adds a new method on the interface
4. site updates to new core
5=> whitescreen because contrib does not yet implement the new method

I don't think that matches the definition of backwards compatibility we intend to support. See https://www.drupal.org/core/d8-allowed-changes#bc-break

A backward-compatibility break is a type of API change that requires changes in code implementing a public API.

Does this change require changes in code implementing a public API? Yes.

Gabor's argumentation is correct.
If someone implements the TranslatableInterface in contrib, it will broken until public function getTranslationStatus() is implemented. Regarding the moved constants, everything is OK.

So a backward compatible implementation will be to create a TranslationStatusInterface for the constants and the accessor function.

Yeah @mkalkbrenner's solution sounds almost elegant :) Also ContentEntityBase would then implement that interface thus implementing that method and receiving the constants.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
@@ -20,7 +21,7 @@
-interface ContentEntityInterface extends \Traversable, FieldableEntityInterface, RevisionableInterface, TranslatableInterface {
+interface ContentEntityInterface extends \Traversable, FieldableEntityInterface, RevisionableInterface, TranslatableInterface, TranslationStatusInterface {

I don't think this resolved any of the concerns with backwards compatibility. The same problem applies to ContentEntityInterface, making it composite does not make a difference.

Making ContentEntityBase implementing it would still let anyone else impplementing ContentEntityInterface to be backwards compatible with code extending from ContentEntityBase would get the new method/interface.

Also I don't think its correct to mark something as deprecated if we don't have a replacement for it in core and we want people to use it. Things that are deprecated are supposed to be not used in favor of something better already available or in favor of being not used because they are bad.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -20,7 +21,7 @@
   +interface ContentEntityInterface extends \Traversable, FieldableEntityInterface, RevisionableInterface, TranslatableInterface, TranslationStatusInterface {
   

   Like Gabor already pointed out, ContentEntityInterface must not extend TranslationStatusInterface to keep BC. Instead, ContentEntityBase should implement the interface.

2. +++ b/core/lib/Drupal/Core/TypedData/TranslationStatusInterface.php
   @@ -0,0 +1,40 @@
   + * @deprecated in Drupal 8.3.0, will be merged into
   + * Drupal\Core\TypedData\TranslatableInterface before Drupal 9.0.0.
   

   The merge should be discussed after the new interface has been committed in a different issue.
   From my point of view it's just a nice to have.
   If someone currently implements the TranslatableInterface directly without extending ContentEntityBase, I bet he's not tracking the translation state in his implementation ;-)

The feature itself is very useful for us and we should get it to a committable state quickly :-)

Yay, looks all good now.

The patch had more change than I anticipated, so just want to clarify exactly what's happening:

1. Right now there's no way to get this information (except in subclasses of ContentEntityBase maybe)

2. If we just add a way to get the information, it will be wrong in some cases (presumably these are cases ContentEntityBase itself does not care about).

3. So the patch both fixes the statuses, and adds the way to get them at the same time.

I don't see a way to disentangle these, tried an issue re-title to clarify what's happening a bit more.

Passes again.

@catch: any concerns that would stop committing this? :)

I don't have anything concrete, but also haven't managed to commit it yet. I pinged xjm and alexpott for a second look just in case.

Changes look reasonable to me too, thanks!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -368,6 +354,25 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +
   +    // Update the status of all saved translations.
   +    $removed = [];
   +    foreach ($this->translations as $langcode => &$data) {
   +      if ($data['status'] == static::TRANSLATION_REMOVED) {
   +        $removed[$langcode] = TRUE;
   +      }
   +      else {
   +        $data['status'] = static::TRANSLATION_EXISTING;
   +      }
   +    }
   +    $this->translations = array_diff_key($this->translations, $removed);
   +  }
   

   I looked at this and worried about whether the statuses should be set before calling parent::postSave() but looking at the code this is fine because the only that occurs in the parent is:

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

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -220,7 +206,7 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   -    $data = array('status' => static::TRANSLATION_EXISTING);
   

   This looks like this is fixing a bug / changing behaviour? Also I think we should call $this->isNew() instead of isset($values[$this->getEntityType()->getKey('id')]) since if we're enforcing newness then the status should be TRANSLATION_CREATED no?

So the patch from #27 still applies.

@hchonov: In this case, patch #27 should be uploaded again to be the latest one to not confusing people.

As good as it was in #31 :)

Sorry to knock this back again, but let's add a comment in the constructor explaining that we can't use isNew() there, then no-one will wonder next time they look at it.

Thanks for the fix!

Committed a469aa2 and pushed to 8.3.x. Thanks!

Yay thanks, made a minor modification to the change notice and published at https://www.drupal.org/node/2789903.