Translatable entity 'created' and 'uid' fields not initialized properly during content translation 'Add'

The proposed changes make sense to me, thanks! However, it'd be good to get also Gabor's feedback about them, since the author part is a functional change.

Marking needs work for the missing test coverage.

.

@hchonov:

Gabor seems to be unavailable atm, are you willing to provide some additional test coverage?

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -57,7 +57,14 @@ public static function create(ContainerInterface $container) {
+    $target_translation = $entity->addTranslation($target->getId(), $source_translation->toArray());
+
+    /** @var \Drupal\user\UserInterface $user */
+    $user = entity_load('user', \Drupal::currentUser()->id());
+
+    $metadata = $this->manager->getTranslationMetadata($target_translation);
+    $metadata->setAuthor($user);
+    $metadata->setCreatedTime(REQUEST_TIME);
...
 

Actually I think it makes sense to do this only if the fields are translatable, otherwise we'd be overriding the original author/created time.

For that we need new methods on the translation handler, as that encapsulates field definition handling.

.

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -57,7 +57,14 @@ public static function create(ContainerInterface $container) {
+    $user = entity_load('user', \Drupal::currentUser()->id());

Let's inject the entity manager and the current user services and use them here.

However, it'd be good to get also Gabor's feedback about them, since the author part is a functional change.

Can you explain the change?

I'm fine with this new approach, however for consistency we should check field storage definition translatability for all metadata fields. Moreover we still need checks + exceptions in the MW, because the fact that a field storage definition is marked as translatable, only means it supports multilingual values. We need runtime checks on the field definiton, which tells us whether the field is actually translatable for the bundle it is attached on.

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -71,6 +79,7 @@ public function __construct(EntityTypeInterface $entity_type, LanguageManagerInt
   +    $this->fieldStorageDefinitions = \Drupal::entityManager()->getLastInstalledFieldStorageDefinitions($this->entityTypeId);
   

   I know we already have this code in HEAD, but as I mentioned in IRC the plan was to remove it. If it's going to stay we need to properly inject the entity manager, see ContentTranslationHandler::createInstance().

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -144,13 +154,22 @@ public function getFieldDefinitions() {
   +    //Check for field named uid, only in case the entity implements the
   +    //EntityOwnerInterface. This helps to exclude cases, where the uid is
   +    //defined as field name but is not meant to be an owner field
   +    //e.g. the User entity
   

   Coding standards mandate a space after // and a trailing dot. Moreover all lines should wrap as close to column 80 as possible.

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -144,13 +154,22 @@ public function getFieldDefinitions() {
   +    if (is_subclass_of($this->entityType->getClass(), '\Drupal\user\EntityOwnerInterface')) {
   +      if (array_key_exists('uid', $this->fieldStorageDefinitions) && $this->fieldStorageDefinitions['uid']->isTranslatable()) {
   +        return TRUE;
   +      }
   +    }
   +    return FALSE;
   

   Can we just return the value of the three conditions (joined via AND)? It's more compact and readable.
   Moreover we can just do $this->entityType->isSubclassOf('\Drupal\user\EntityOwnerInterface'), which is a bit shorter and readable itself.

4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -174,13 +193,17 @@ protected function hasChangedTime() {
   +   * Checks whether the entity type supports translatable
   +   * creation time natively.
   

   According to coding standards this should be a single line. I think we can get rid of translatable as it is now implicit.

5. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -174,13 +193,17 @@ protected function hasChangedTime() {
   +    if (array_key_exists('created', $this->fieldStorageDefinitions) && $this->fieldStorageDefinitions['created']->isTranslatable()) {
   

   As above, let's just return the test value instead of adding the if.

6. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -626,4 +649,15 @@ protected function entityFormTitle(EntityInterface $entity) {
   +   * @see ::baseFieldDefinitions()
   

   This is unrelated and should be removed.

7. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -626,4 +649,15 @@ protected function entityFormTitle(EntityInterface $entity) {
   +  public static function getCurrentUserId() {
   

   I think it would be better to rename this to ::getDefaultOwnerId(), IMO the fact that we return the current user is an implementation detail and should not leak into the public API.

8. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -626,4 +649,15 @@ protected function entityFormTitle(EntityInterface $entity) {
   +    return array(\Drupal::currentUser()->id());
   

   We should inject also the current user service, see above.

9. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -57,7 +57,14 @@ public static function create(ContainerInterface $container) {
   +    $user = entity_load('user', $this->currentUser()->id());
   

   We should use the (injected) entity manager here, global functions should be avoided in OO code as they make it harder to unit test.

10. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -57,7 +57,14 @@ public static function create(ContainerInterface $container) {
    +
    

    Surplus blank line, please remove it :)

11. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
    @@ -73,12 +73,39 @@ protected function doTestBasicTranslation() {
    +    $current_user = $this->loggedInUser;
    +    $new_user = $this->drupalCreateUser($this->getTranslatorPermissions(), 'test_translation_author');
    +    $this->drupalLogin($new_user);
    +
    ...
    +    //switch back to the previous user
    +    $this->drupalLogin($current_user);
    

    Why do we need to create a new user? ContentTranslationTestBase already provides three test users, if we wish to test that translations are created with a different user, we could log in with $this->editor at the very beginning of the test and then log in with $this->translator here. No need for all this code then.

12. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
    @@ -73,12 +73,39 @@ protected function doTestBasicTranslation() {
    +    //Get the entity and reset it's cache, so that the new translation gets
    +    //the updated values
    

    Space after //, missing trailing dot.

13. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
    @@ -73,12 +73,39 @@ protected function doTestBasicTranslation() {
    +
    

    Surplus blank line :)

Just one notice - getDefaultOwnerId is a static function and as such it can not access the injected current user service, so I have to use the global function.

Oh, you're right. Can you please post an interdiff (a diff between #19 and #22)? We always do that at every iteration when dealing with patches bigger then a couple of lines, it makes things easier to review.

Wow, that' s bigger than the patch, I'm afraid it won't be so useful then, but thanks anyway ;)

I'll have a look to this later.

This is starting to look really good :)

I have only a few more minor remarks and just one actual objection (see 2):

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -180,7 +211,7 @@ protected function hasChangedTime() {
   +    return array_key_exists('created', $this->fieldStorageDefinitions) && $this->fieldStorageDefinitions['created']->isTranslatable();
   

   Can we add a protected helper method to avoid repeating this logic for all fields?

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -533,10 +563,25 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    // It is possible, that not all the metadata fields are translatable on the
   +    // current entity bundle, so just catch the exception and continue.
   +    try {
   

   Really? I thought the point of introducing exceptions here was to make the site blow up in the user face when it's configured the wrong way, exactly to avoid silently failing.

   Well, maybe a friendlier way could be adding some checks during the validation of the field translatability settings form.

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -626,4 +671,13 @@ protected function entityFormTitle(EntityInterface $entity) {
   +    return array(\Drupal::currentUser()->id());
   

   Why an array? We now use square brackets, btw.

4. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapper.php
   @@ -138,8 +139,24 @@ public function getChangedTime() {
   +   * Private helper method to check if a bundle field is translatable before
   +   * updating it and throw an exception if not.
   +   *
   +   * @param $field_name
   +   * @throws \Drupal\content_translation\Exception\NonTranslatableBundleFieldException
   

   Coding standards mandate a single introductory line, shorter than 80 chars, followed by an optional longer text.

   Also, we are missing @param type, @param description, and exception description.

5. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapper.php
   @@ -138,8 +139,24 @@ public function getChangedTime() {
   +  private function ensureBundleFieldIsTranslatableBeforeUpdating($field_name) {
   

   What about just ::checkFieldTranslatability()? It's a bit long now, and bundle field has a slightly different meaning in the Entity Field API.

   Also, no private visibility allowed, except for vary rare cases, let's make this protected.

6. +++ b/core/modules/content_translation/src/Exception/NonTranslatableBundleFieldException.php
   @@ -0,0 +1,14 @@
   + * is called for a non translatable bundle field.
   ...
   +class NonTranslatableBundleFieldException extends \RuntimeException {}
   

   Per above, let's remove the bundle word, and just leave field. Field definitions are per-bundle, no need to specify it. Bundle fields are fields that may be attached only to certain bundles, unlike base fields (like nid) which are attached to all bundles of a certain entity type. The most common example of bundle fields are our beloved Field UI fields :)

Very sorry for losing track of this, DrupalCon sent it completely out of my radar :(

I have a last small set of remarks and then we should be ready to go, I think:

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -533,10 +578,25 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    // It is possible, that not all the metadata fields are translatable on the
   +    // current entity bundle, so just catch the exception and continue.
   

   Ok, now makes sense to me. We should add a brief explanation also in this comment though.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -533,10 +578,25 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    try {
   +      $metadata->setAuthor(!empty($values['uid']) ? User::load($values['uid']) : User::load(0));
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   +    try {
   +      $metadata->setPublished(!empty($values['status']));
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   +    try {
   +      $metadata->setCreatedTime(!empty($values['created']) ? strtotime($values['created']) : REQUEST_TIME);
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   +    try {
   +      $metadata->setChangedTime(REQUEST_TIME);
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   

   I'm wondering whether it would make sense to add a $skip_untranslatable = FALSE parameter to skip the exception on fail (and the set operation of course), to make this code cleaner.

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -626,4 +686,13 @@ protected function entityFormTitle(EntityInterface $entity) {
   +    return [\Drupal::currentUser()->id()];
   

   Is it mandatory to return an array of values? The method name is a bit confusing wrt the returned value atm.

4. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapperInterface.php
   @@ -69,6 +69,9 @@ public function getAuthor();
   +   * @throws \Drupal\content_translation\Exception\NonTranslatableFieldException
   +   *   If the author bundle field is not translatable.
   

   Here and below: let's remove the bundle term here too, we can use "field definition" instead.

5. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -57,7 +58,23 @@ public static function create(ContainerInterface $container) {
   +    try {
   +      $metadata->setAuthor($user);
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   +    try {
   +      $metadata->setCreatedTime(REQUEST_TIME);
   +    }
   +    catch (NonTranslatableFieldException $e) {}
   

   If we add the parameter we should use it also here.

Sorry, it seems the NonTranslatableFieldException class file was lost in the reroll, but the patch is green, which means we are lacking test coverage for that. Can we add a few lines in the test so that a metadata field is configured as not translatable and then test the setter with the two values for $skip_untranslatable?

Other than that this looks ready to me. However I'm attaching a diff with a few nitpick fixes and some suggested changes for comments and PHP docs, which looked a bit verbose to me. Feel free to include it in the next patch if you are ok with it.

We should also update the issue summary with the implemented solution.

Awesome, thanks!

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -534,10 +579,16 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +    $metadata->setAuthor(!empty($values['uid']) ? User::load($values['uid']) : User::load(0), TRUE);
   +    $metadata->setPublished(!empty($values['status']), TRUE);
   +    $metadata->setCreatedTime(!empty($values['created']) ? strtotime($values['created']) : REQUEST_TIME, TRUE);
   +    $metadata->setChangedTime(REQUEST_TIME, TRUE);
   

   Are these set methods ever called with FALSE anywhere in the code (apart from the test)? Perhaps we don't need the flag - Martin Fowler would argue that flags are a code smell - should just have two methods. Reading back through the comments @plach wrote:

   Really? I thought the point of introducing exceptions here was to make the site blow up in the user face when it's configured the wrong way, exactly to avoid silently failing.

   Hmmm... can the user configure the site through the front end and end up with exceptions? That's not exactly user friendly.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -634,4 +685,13 @@ protected function entityFormTitle(EntityInterface $entity) {
   +  public static function getDefaultOwnerId() {
   +    return \Drupal::currentUser()->id();
   +  }
   
   +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -57,7 +57,18 @@ public static function create(ContainerInterface $container) {
   +    // Update the translation author to current user, as well the translation
   +    // creation time. It is possible, that not all the metadata fields are
   +    // translatable on the current entity bundle, in that case we just avoid
   +    // overriding the original values.
   +    $metadata->setAuthor($user, TRUE);
   +    $metadata->setCreatedTime(REQUEST_TIME, TRUE);
      }
   

   Afaics one of these is setting of author ID is tested - not completely sure which one - does the other have test coverage?

3. +++ b/core/modules/content_translation/src/ContentTranslationMetadataWrapperInterface.php
   @@ -84,10 +91,17 @@ public function isPublished();
   -  public function setPublished($published);
   +  public function setPublished($published, $skip_untranslatable = FALSE);
   
   @@ -120,9 +141,16 @@ public function getChangedTime();
   -  public function setChangedTime($timestamp);
   +  public function setChangedTime($timestamp, $skip_untranslatable = FALSE);
   

   I think these changes are missing test coverage... I can only see test coverage for setCreatedTime and setAuthor. I might be missing something though.

4. +++ b/core/modules/content_translation/src/Tests/ContentTranslationMetadataFieldsTest.php
   @@ -0,0 +1,109 @@
   +  protected function testSkipUntranslatable() {
   

   Test methods should be public

5. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -80,6 +85,39 @@ protected function doTestBasicTranslation() {
   +      $message = format_string('Author of the target translation @langcode correctly stored for translatable owner field.',
   +        array('@langcode' => $langcode));
   +      $this->assertEqual($metadata_target_translation->getAuthor()->id(), $this->translator->id(), $message);
   ...
   +      $message = format_string('Author of the target translation @target different from the author of the source translation @source for translatable owner field.',
   +        array('@target' => $langcode, '@source' => $default_langcode));
   +      $this->assertNotEqual($metadata_target_translation->getAuthor()->id(), $metadata_source_translation->getAuthor()->id(), $message);
   ...
   +      $message = format_string('Translation creation timestamp of the target translation @target is newer than the creation timestamp of the source translation @source for translatable created field.',
   +        array('@target' => $langcode, '@source' => $default_langcode));
   +      $this->assertTrue($metadata_target_translation->getCreatedTime() > $metadata_source_translation->getCreatedTime(), $message);
   

   I think the creation of the $message variable in these instances doesn't really help - might as well just have a one line assertion which includes the message creation. Also format_string should just be SafeMarkup::format().

6. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -80,6 +85,39 @@ protected function doTestBasicTranslation() {
   +      $message = format_string('Author of the entity remained untouched after translation for non translatable owner field.');
   +      $this->assertEqual($metadata_target_translation->getAuthor()->id(), $this->editor->id(), $message);
   ...
   +      $message = format_string('Creation timestamp of the entity remained untouched after translation for non translatable created field.');
   +      $this->assertEqual($metadata_target_translation->getCreatedTime(), $metadata_source_translation->getCreatedTime(), $message);
   

   There's no point to using format_string() here and the creating a $message variable is not needed either.

@alexpott:

Are these set methods ever called with FALSE anywhere in the code (apart from the test)? Perhaps we don't need the flag - Martin Fowler would argue that flags are a code smell - should just have two methods.

Personally I think having a bunch of ::setXOnlyIfTranslatable() methods wouldn't be great for DX either.

Reading back through the comments @plach wrote:

Really? I thought the point of introducing exceptions here was to make the site blow up in the user face when it's configured the wrong way, exactly to avoid silently failing.

Hmmm... can the user configure the site through the front end and end up with exceptions? That's not exactly user friendly.

It's meant to be developer friendly: by default methods will blow up with an expeption so code not taking all possible scenarios into account will break. It should be the developer responsibility to mark its code as "safe" by setting the flag to TRUE.

@hchonov:

Let's just remove the exception and make only one method without the $skip_untranslatable parameter and this method will by default skip setting an untranslatable field? This addresses only fields that are provided by the entity's base fields definitions.

I agree this could be a viable solution if @alexpott is not convinced by our explanations.

Looks great, thanks!

I made just a few code style/cosmetic adjustments, nothing that should prevent me from RTBC-ing this :)

We just need an issue summary update to bring it up-to-date with the latest change. The API section is out of date.

Made a small clarification in the IS.

I'm pleased we're fixing this major bug with no disruptive API change. Thanks for updating the issue summary. Committed 6588593 and pushed to 8.0.x. Thanks!

Yay, amazing! Thanks all!

Yay!

@hchonov:

Thanks for your patience and perseverance!

hchonov++