hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways

During the last #drupal-entity IRC meeting we agreed to introduce a new hook to be called in addition to hook_entity_create() to allow to manipulate field default values without having to differentiate between entity creation and translation creation. The rationale is that there is no difference in initializing an entity object or a translation object wrt initial/default values. Suggested signatures:

hook_entity_field_default_values_alter($values, $langcode)
hook_entity_field_values_init($values, $langcode)
(others?)

Yep - the wrong hook being fired is the main issue that we have to fix here imo. Bumping priority for that.

We need to decide if simply not invoking a hook, from the wrong place, is an API break.

If you rely on that behaviour, your module might break, but the behaviour is not supposed to happen at all in the first place.

We need to decide if simply not invoking a hook, from the wrong place, is an API break.
If you rely on that behaviour, your module might break, but the behaviour is not supposed to happen at all in the first place.

Yep, form my POV it is not an API break, it's a plain bug fix since the intended behavior would be preserved by introducing a new hook and the buggy behavior would be removed.

Working on this

First attempt at implementing #8.

Spoke with @Berdir, this patch addresses also the related issue pointed out in https://github.com/md-systems/pathauto/issues/90#issuecomment-154173677. We agreed that a single issue makes sense in this case, since we are basically completing the Entity Translation API wrt entity translation creation.

Reviews welcome :)

+++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
@@ -302,6 +302,7 @@ function testEntityTranslationAPI() {
+    $this->doTestEntityTranslationAPI($entity_type);

Unrelated

Fixed typo in tags

Rerolled, reviews welcome...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -822,37 +829,15 @@ public function addTranslation($langcode, array $values = array()) {
+    if ($storage instanceof ContentEntityStorageInterface) {
+      $this->translations[$langcode]['status'] = static::TRANSLATION_CREATED;
+      return $storage->createTranslation($this, $langcode, $values);
     }
...
+    else {
+      throw new \LogicException('The storage handler cannot create entity translations.');
     }

Why do we have to check here if the storage for the content entity implements the ContentEntityStorageInterace and do not instead enforce that content entity types define a storage class implementing the ContentEntityStorageInterace? We do exactly this in ConfigEntityType::__construct, where we enforce the defined storage class to be an instance of ConfigEntityStorage.

That's a good suggestion, but I'm not sure we can still do that.

I guess your concerns are that if we do the check in the ContentEntityType::__construct then there will be a greater impact, but actually no matter where we do it there is going to be an impact. Doing it in the ContentEntityBase::addTranslation would mean we are going anyway to force anyone having a custom storage not extending from ContentEntityStorageBase to implement the new interface as soon as they wann create new translations. And actually doing this check in ContentEntityBase::addTranslation will hide the change for a longer time.

So I think that if we are going to do this change then we have to do it the right way even if there is a disruption.

Implemented #24, let's see whether the bot is happy.

This looks great to me, and the sort of change we can't make in a patch release, so I'd support trying to get it in before 8.0.0.

I think we could make this change in 8.1.x, but there's more chance of modules relying on the current, buggy, behaviour by then than if we do it now.

So, I guess we need a separate key value storage for content entities as well...

Rrright

Mostly additive changes, a dedicated interface for content entity storage and a new hook_entity_field_values_init() hook to unify entity_create and translation_create. Nice work.

Everything looks good to me.

Thanks!

Added two change records:

https://www.drupal.org/node/2616318
https://www.drupal.org/node/2616322

Discussed with xjm and alexpott. Did't quite finish conversation with xjm, but alexpott agreed with getting this in before 8.0.0l on the basis the chance of disruption gets higher the longer core is doing the wrong thing,

Committed/pushed to 8.0.x, thanks!

Thanks, published the change records!

I'm reverting this because it seems to be causing random fails see #2616808: Slew of random test failures with UpdatePathTestBase when using is_subclass_of() in ContentEntityType::checkStorageClass()

Double checking...

I've just executed locally 15 times the following without a single failure:
php core/scripts/run-tests.sh --url "http://dev-d8.nauts.eu/" --class "\Drupal\system\Tests\Update\AutomatedCronUpdateWithAutomatedCronTest"

And this is the test, which is failing more than once on the test bot.....

An interessting fact as well is that all the tests that have failed on the test bot are using the dump drupal-8.bare.standard.php.gz.

One thing that has caused problems in the past on testbot is that it runs many tests in parallel. If you want to replicate that behavior, try something like --repeat 20 --concurrency 5 or even higher numbers.

But something like this, which smells like a segfault is also always very system/version specific. Any chance that we can get at the apache/php logs of the failing containers somehow?

Making some experiments in #2496337-78: [plach] Testing issue.

This seems to work.

Just atm I've tested locally but I gave by mistake the wrong --url and suprise... I got exactly the same errors like on the testbot...

Any clue?

as requested by @plach in IRC providing logs when submitting wrong --url

I thought I was on to something but I was wrong. However looking at this patch I think think interface inheritance is wrong. ContentEntityStorageInterface extends DynamicallyFieldableEntityStorageInterface? I don't see why that should be the case. Shouldn't ContentEntityStorageInterface extends EntityStorageInterface and then ContentEntityStorageBase implement both ContentEntityStorageInterface and DynamicallyFieldableEntityStorageInterface?

I modeled it after ContentEntityInterface...

I set up a local DrupalCI environment with Vagrant but no luck, the tests pass even with the exact same PHP version (5.5.23) on Ubuntu Trusty. Attached you can find the log...

The log...

@plach so what this might be is something to do with APC and concurrency - we did have random fails before to do with Entity storage classes - see #731724-527: Convert comment settings into a field to make them work with CMI and non-node entities

Here's a patch that shuffles the interfaces as suggested by #57 and doesn't suffer from the obscure random fail - see #2496337-122: [plach] Testing issue

The interdiff looks good to me, RTBC +1.

Changes are quite simple so I think it's ok for me to move this back to RTBC.

+1 RTBC

Here's the class diagram of everything that implements EntityStorageInterface on applying the patch. Note how the ContentEntityStorageInterface and ConfigEntityStorageInterface nicely imitate each other now. It looks correct to me.

I discussed this with the other committers, and we decided to allow this into 8.0.0, because:

• It was already committed and only reverted for a tangential reason that has since been resolved.
• We couldn't come up with an idea for how to do this in a minor release in a way that wouldn't introduce a slew of other problems with respect to BC.
• Leaving this unsolved for all of 8.x is problematic due to the likelihood based on past experience of modules (contrib and custom) being written without getting sufficiently tested in multilingual contexts, but then used on multilingual sites. And this WTF of hook_entity_create() is particularly likely for such modules to overlook (even if we document it) and as a result corrupt translations of those sites.

So, tagging, and also recategorizing as a bug and retitling accordingly.

I pushed #63 to 8.0.x. However, the following nits could use a follow-up issue/patch:

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -459,10 +462,21 @@ public function getStorageClass() {
   +   * Checks that the provided class is an instance of ConfigEntityStorage.
   ...
   +  protected function checkStorageClass($class) {
   +    // Nothing to check by default.
   +  }
   

   Doc doesn't match implementation.

2. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   @@ -0,0 +1,25 @@
   +    // @todo
   

   What should be to-done?

I also think it would be great to have a follow-up issue to add an integration test for what happens when hook_entity_create(), hook_entity_field_values_init(), and hook_entity_translation_create() are all implemented: i.e., that hook_entity_field_values_init() affects both entities and translations, and that the other two hooks only affect the corresponding one. Such a test should also test the expected order in which those hooks fire (i.e., hook_entity_field_values_init() before *_create()).

Thanks!

I'm creating the follow-ups.

• #2618418: Docs follow-up for "hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways"
• #2618420: Add integration testing for hook_entity_create(), hook_entity_field_values_init(), and hook_entity_translation_create()