Programmatically created translatable content type returns SQL error on content creation

I have the same issue. The workaround is to go to admin/config/regional/content-language and click 'Save configuration'. This adds two columns, content_translation_source and content_translation_outdated, to the table node_field_data.

I noticed the exact same behavior. The workaround works.

Got this issue, there's no way to ship language.content_settings.taxonomy_term.navigation.yml to enable translation of vocabulary

PHP message: Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper: "SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression
FROM 
{taxonomy_term_field_data} t
WHERE ( (content_translation_source IS NOT NULL ) )
LIMIT 1 OFFSET 0; Array
(
)

Suppose config import event should fire table rebuild

That's one of these annoying little bugs, that only concerns developers and site builders, but in that case it's really annoying. This would surely be an easy fix. Does anyone have some time to isolate the bug in a test case?

My 'solution' for this issue: after enable my custom module, run drupal update:entities in command promt (console). After that the missing fields are created in the database.

Hi,
For those coming to this issue looking for a code solution, create module.install file and add this to the hook_install():

function MODULE_install(){
  // Ensure the translation fields are created in the database.
  \Drupal::service('entity.definition_update_manager')->applyUpdates();
}

Where MODULE should be replaced by your module name.
Hope this helps...

Both #8 and #9 prevent the error from occurring.

Hit this as well, this seems to be working for me.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -192,6 +193,38 @@ function content_translation_entity_base_field_info(EntityTypeInterface $entity_
   +  // @see ContentTranslationManager::isEnabled()
   

   I think @see should contain the whole path to the method - inclusive the namespace of the class ?

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -192,6 +193,38 @@ function content_translation_entity_base_field_info(EntityTypeInterface $entity_
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
   +    /** @var \Drupal\content_translation\ContentTranslationUpdatesManager $updates_manager */
   +    $updates_manager = \Drupal::service('content_translation.updates_manager');
   +    $updates_manager->updateDefinitions([$entity_type_id => $entity_type]);
   ...
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
   +    /** @var \Drupal\content_translation\ContentTranslationUpdatesManager $updates_manager */
   +    $updates_manager = \Drupal::service('content_translation.updates_manager');
   +    $updates_manager->updateDefinitions([$entity_type_id => $entity_type]);
   

   This is a code duplication in both hooks - we should create a helper method instead.

Re #14:

1. I don't feel strongly about this, but since we're not allowed to do /** */-style comments in core anyway, you won't be able to click this in your IDE anyway, so I don't think there's much value to using the namespace.

2. Where would you propose to add such a method? And with which parameters?

This fixes #14.1.

I also realized that since we are now hooking into the regular entity save process I think we no longer need to custom event subscriber for migration and config import. Both of those subsystems go through $entity->save() so we should be fine. Both of those things are tested, so let's see if anything blows up.

This is the minimal change for now, I left the actual methods and everything in place, not sure how far we want to go in 8.2. That's also why I didn't implement 2. yet. The event subscribers are the only other usages of ContentTranslationUpdateManager::updateDefinitions() so with this we could deprecate that and provide a similar method with a different signature that is more easily used from the entity hooks á la #14.2. However, I am - again - not sure how far we actually can go with that in 8.2, so not doing any of that yet.

I would put the new function into content_translation.module.
What about content_translation_update_entity_type_definition($entity_type_id);?

Ok, implementing EventSubscriberInterface and returning NULL in getSubscribedEvents() really wasn't that smart... Let's see. Still leaving the actual subscribers around for BC. We can discuss that more once this is green...

Not my day today.

OK, will try to find some time to write dedicated tests for this, but in the meantime tagging Needs release manager review so I know how we can proceed.

Here's the quick run-down of this issue in regards to release management

• This is a major (albeit not triaged) bug, so it needs to go all the way back to 8.2
• Content Translation has a helper method to install the base fields on entity types (ContentTranslationUpdatesManager::applyUpdates())
• That method is currently called from 3 places. The patch centralizes this (in hook_entity_insert/update()) so that all three current usages are obsolete. The usages are:
  1. ContentTranslationManager::setEnabled() which in turn is only called from the UI and in tests. The behavior of calling ::setEnabled() does not change with this patch as the entity is saved and the new hook implementations are, thus, called.
  2. An event subscriber for config import (ContentTranslationUpdatesManager::onConfigImporterImport(). This is obsolete with the patch as config import calls ::save() on config entities already
  3. An event subscriber for migrations (ContentTranslationUpdatesManager::onMigrateImport()). This is obsolete with the patch as migrations call ::save() on config entities already.

Removing the ::updateDefinitions() call from ContentTranslationManager::setEnabled() is uncontroversial because it is neither an API nor a behavior change. For the event subscribers we can take a number of different routes to clean this up, now that they are obsolete. I'm not sure which are allowed (ordered by by ascending invasiveness and ascending cleanliness):

• Empty out the method bodies of the obsolete event subscriber methods
• Remove the events from getSubscribedEvents() so that the methods are no longer called
• Remove getSubscribedEvents(), implements EventSubscriberInterface and the event_subscriber tag from the service
• Remove the obsolete methods

Since the newly introduced hooks are now the only usages of ContentTranslationsUpdateManager::updateDefinitions() we could consider renaming it and or changing its signature assuming that it's not considered an API function. Instead of ::updateDefinitions(array $entity_type) we could have ::installFieldStorageDefinitions(string $entity_type_id) or similar. That would allow nicely fixing the issue brought up in #14.4 which we could otherwise fix not so nicely by simply introducing a helper function.

Chatted with @catch in IRC and cleared up the path to go which I implemented now. There is slightly more duplication between the two hooks which @hchonov will not be happy about, but apart from @catch signing off on that the diff stat of the patch is still 55 insertions(+), 124 deletions(-), so I think that's acceptable. (That will of course change negatively with added test coverage, but still.)

Note that the interdiff does not contain the deletion of ContentTranslationUpdatesManager because PhpStorm tried to be smart with the deletion.

Still needs tests and needs an issue summary update for the proposed resolution.

So one thing I did not mention is that ContentTranslationUpdatesManager::updateDefinitions() contained a check for EntityDefinitionUpdateManager::needsUpdates() which seemes unnecessary to me, because we already check explicitly if field storage definitions are missing. That internally, though, reset's the static field storage definition cache by calling EntityFieldManager::useCaches(FALSE). Making this more explicit now.

OK, this should be green.

Currently the tests that migrate the language settings (with Content Translation on) don't have the node schema installed. I was wondering how the tests pass currently and I found out that - while updateDefinitions() does get called correctly - the Content Translation fields are never installed due to some race condition in caching. So the fact that we have to modify those tests for this to pass is in fact a deficiency in those tests themselves.

We can always add more assertions or improve the tests in other ways, but that's not in scope for this issue.

I'd like to add a bit more information to this just to ensure the fix is complete, as I think I've found a related situation.

I have a content type that I'm defining in a custom module. I've got my_module/config/optional/language.content_settings.node.my_module.yml which contains

langcode: en
status: true
dependencies:
  config:
    - node.type.my_module
  module:
    - content_translation
third_party_settings:
  content_translation:
    enabled: true
id: node.my_module
target_entity_type_id: node
target_bundle: my_module
default_langcode: site_default
language_alterable: true

When I run my Functional test and create pieces of content I get the same error about the unknown column.

If I create a my_module.install that contains the my_module_install as suggested in #9 the error goes away.

This error happens only in some circumstances.
Right now I had this error after a git pull/drush fia. Patch #27 didn't fix the problem.
When I install everything from scratch, it happens without the patch #27, it does not happen if previously I apply #27.
This always fixes the problem:
$ drush entity-updates

About the patch, it works. Once it's there since the beginning the problem won't happen.

Re-roll only.

+++ b/core/modules/content_translation/content_translation.services.yml
@@ -23,10 +23,4 @@ services:
-  content_translation.updates_manager:

removal of service does not fits into BC

had the same issue when installing the site via config_install.
Just wanted to say thanks and the patch in #31 works for me.

#31 works for me also. Using it with a multilingual install profile, with content translation and various entity translations enabled.

Looks good to me. Applied the patch cleanly and the error is gone.

@andypost #32 the service being removed is only an event subscriber, so it should be OK for bc.

Uploading a test-only patch since there isn't one here.

I don' think the test-only patch is testing anything other than the entity schema update, we should add an explicit test for the content creation (also uploading a patch with only the test changes this time).

Also the event subscriber being removed is injected into one class and used as an API, so there's an argument it's not just an event subscriber and we should deprecate it.

Same here, D8.4.x. heavy installation with multiple translatable entity types and 4 active languages.
After installation with "drush site-install --config-dir="../config"" I ran in the same error while creating new contents.

#31 solved it, thanks ! :)

I faced the same issue when importing page manager config using Drupal version 8.3.9 and #31 solved it for me thanks a lot.

For anyone who, like me, has been having trouble applying both this patch and the one from 2868294, here's a reroll that addresses the bugs raised in that issue.

Here's a patch that deprecates instead of removes ContentTranslationUpdateManager per #38.

We still need to add a dedicated test for this per #38.

Also added a change notice (just created the node for now to get a nid for the patch, will fill with some text in a second).

When writing the change notice, I looked around the code again and realized that this leaves around some obsolete migration code.

Ahh, too much copy-paste with the deprecation notices. Let's see.

Fixed PHPCS errors. Sorry about that.

Updated the issue summary

...and here's a dedicated test for the behavior. The tests-only patch is also the interdiff. We currently don't have a translatable test entity type with bundles, so I chose to make entity_test_with_bundle translatable rather than adding a whole new type.

I've tested 2599228-51.patch in #51. It fixes the problem.
Code and tests are satisfactory. Marking as RTBC.

Same error here. I'm using a profile and was trying to import view config but the kernel test were failing. Applied the patch and it's working now. Thank you!

I have also tested Patch 51 and working.

Just adding another +1 to the patch on #51. Tested on 8.7 and working properly.

Rollup the patch

Unless I'm missing something, the hunk in core/modules/content_translation/migrations/d7_node_translation.yml is missing from the patch.

Patch on #58 applied cleanly on 8.5.6 for me, and fixed issue importing view config.

Alright, here's a re-roll. git rebase was nice enough to do the work for me. However, since #2073467: Migrate Drupal 7 Entity Translation settings to Drupal 8 there's another hunk we can remove. Going back to RTBC per #52.

We are developing a custom profile that enter in this error when call to:

\Drupal::entityDefinitionUpdateManager()->applyUpdates();

If we call that after install modules in a bath process, it stop with:

[Mon Sep 17 10:02:13.676392 2018] [php7:notice] [pid 279] [client 10.0.3.1:45962] Drupal\\Core\\Entity\\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression
FROM
{node_field_data} t
WHERE content_translation_source IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)
 in /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 1481
#0 /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(1430): Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorage->wrapSchemaException(Object(Closure))
#1 /var/www/html/web/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php(126): Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorage->onFieldStorageDefinitionUpdate(Object(Drupal\\Core\\Field\\BaseFieldDefinition), Object(Drupal\\Core\\Field\\BaseFieldDefinition))
#2 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityManager.php(586): Drupal\\Core\\Field\\FieldStorageDefinitionListener->onFieldStorageDefinitionUpdate(Object(Drupal\\Core\\Field\\BaseFieldDefinition), Object(Drupal\\Core\\Field\\BaseFieldDefinition))
#3 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(260): Drupal\\Core\\Entity\\EntityManager->onFieldStorageDefinitionUpdate(Object(Drupal\\Core\\Field\\BaseFieldDefinition), Object(Drupal\\Core\\Field\\BaseFieldDefinition))
#4 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(131): Drupal\\Core\\Entity\\EntityDefinitionUpdateManager->doFieldUpdate(2, Object(Drupal\\Core\\Field\\BaseFieldDefinition), Object(Drupal\\Core\\Field\\BaseFieldDefinition))
#5 /var/www/html/web/profiles/custom/sdos_scity/sdos_scity.profile(271): Drupal\\Core\\Entity\\EntityDefinitionUpdateManager->applyUpdates()
#6 /var/www/html/web/core/includes/batch.inc(294): sdos_scity_fix_entity_update(true, Array)
#7 /var/www/html/web/core/includes/batch.inc(137): _batch_process()
#8 /var/www/html/web/core/includes/batch.inc(93): _batch_do()
#9 /var/www/html/web/core/includes/install.core.inc(679): _batch_page(Object(Symfony\\Component\\HttpFoundation\\Request))
#10 /var/www/html/web/core/includes/install.core.inc(584): install_run_task(Array, Array)
#11 /var/www/html/web/core/includes/install.core.inc(125): install_run_tasks(Array, NULL)
#12 /var/www/html/web/core/install.php(44): install_drupal(Object(Composer\\Autoload\\ClassLoader))
#13 {main}, referer: http://lxc.scityram/core/install.php?rewrite=ok&profile=sdos_scity&langcode=es&id=5&op=start

but can continue if reload the page.

If we put the update at the end of install, in hook_after_install_finished() it stop too, but can't continue. The trace is:

The website encountered an unexpected error. Please try again later.
Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression FROM {node_field_data} t WHERE content_translation_source IS NOT NULL LIMIT 1 OFFSET 0; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1481 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException(Object) (Line: 1430)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->onFieldStorageDefinitionUpdate(Object, Object) (Line: 126)
Drupal\Core\Field\FieldStorageDefinitionListener->onFieldStorageDefinitionUpdate(Object, Object) (Line: 586)
Drupal\Core\Entity\EntityManager->onFieldStorageDefinitionUpdate(Object, Object) (Line: 260)
Drupal\Core\Entity\EntityDefinitionUpdateManager->doFieldUpdate(2, Object, Object) (Line: 131)
Drupal\Core\Entity\EntityDefinitionUpdateManager->applyUpdates() (Line: 377)
sdos_scity_after_install_finished(Array) (Line: 709)
install_run_task(Array, Array) (Line: 584)
install_run_tasks(Array, NULL) (Line: 125)
install_drupal(Object) (Line: 44)

I changed the issue status to "Need work", please, If my use case is out of the scope of this issue then change status again.

If I remove all entity updates in profile, and then call drush entity-updates I get the same error:

developer@scityram:/var/www/html (develop)*$ drush entity-updates
The following updates are pending:

node entity type : 
El campo Origen de la traducción necesita ser actualizado.
El campo Traducción anticuada necesita ser actualizado.

 Do you wish to run all pending updates? (yes/no) [yes]:
 > 

 [error]  Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression
FROM
{node_field_data} t
WHERE content_translation_source IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1481 of /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). 
Array	Array
 [success] Cache rebuild complete.
 [success] Finished performing updates.

Hmm... calling EntityDefinitionUpdateManager::applyUpdates() (or the equivalent drush entity-updates) is really not supported outside of development, so #62 and #63 do not necessarily mean that something is wrong with the patch. I guess you have hit the situation caused by the bug, that Drupal thinks the field is installed, but it never actually created the respective database tables. You should be able to rectify this by manually disabling content translation for all node types, and then manually re-enabling it again afterwards.

If you do not want to do it manually you could try to write an update hook that uninstalls the field storage definition using EntityDefinitionUpdateManager::uninstallFieldStorageDefinition() and then re-installs it (with the corresponding method) afterwards. That should also work.

Let me know if that worked.

Without the patch, if I disable translations in admin/config/regional/content-language and re-enable it again manually the problem persist:

developer@scityram:/var/www/html (develop)*$ drush entity-updates
The following updates are pending:

node entity type : 
El campo Origen de la traducción necesita ser actualizado.
El campo Traducción anticuada necesita ser actualizado.

 Do you wish to run all pending updates? (yes/no) [yes]:
 > 

 [error]  Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression
FROM
{node_field_data} t
WHERE content_translation_source IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1481 of /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). 
Array	Array
 [success] Cache rebuild complete.
 [success] Finished performing updates.

With the patch in #61, and manually disable and re-enable, I have the same problem:

developer@scityram:/var/www/html (develop)*$ drush entity-updates
The following updates are pending:

node entity type : 
El campo Origen de la traducción necesita ser actualizado.
El campo Traducción anticuada necesita ser actualizado.

 Do you wish to run all pending updates? (yes/no) [yes]:
 > 

 [error]  Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'where clause': SELECT 1 AS expression
FROM
{node_field_data} t
WHERE content_translation_source IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1481 of /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). 
Array	Array
 [success] Cache rebuild complete.
 [success] Finished performing updates.

:(

Thanks @psf_ for testing that! That's unfortunate that it didn't work. I will try to reproduce the error and then see if I can manage to get back out of it. I still think the patch itself may be fine, but I still think we should have this figured this out before the patch gets in.

Thanks, I see extrange that the definition is ok but the database it is not update too.

@Berdir pointed me here via #2994398: Not properly clearing EntityFieldManager's fieldMap leads to fatals, often after migration or bundle creation.

From my testing, that issue solves this problem completely.
Running the test from this issue (without the other changes) but with the fix from that issue, everything passes.
So I'm postponing this one for now.

Just for the record, here is a script that fixes it for me. In case this can help anyone:

$kv = \Drupal::service('keyvalue')->get('entity.definitions.installed');
$schema = $kv->get('node.field_storage_definitions', []);
unset($schema['content_translation_source']);
$kv->set('node.field_storage_definitions', $schema);

Then I run entity updates.

Hello,

I've made some changes on this patch because, it is not really applied on the Drupal's version 8.6.x, and now, it's working fine.

Here's this patch before and after rebasing now that #2994398: Not properly clearing EntityFieldManager's fieldMap leads to fatals, often after migration or bundle creation is in.

~/www/d8$ ./vendor/bin/phpunit -c core core/modules/content_translation/tests/src/Kernel/ContentTranslationModuleInstallTest.php
PHPUnit 6.5.12 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\content_translation\Kernel\ContentTranslationModuleInstallTest
E                                                                   1 / 1 (100%)

Time: 3.32 seconds, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\content_translation\Kernel\ContentTranslationModuleInstallTest::testFieldUpdates
InvalidArgumentException: Field content_translation_source is unknown.

/Users/tim/www/d8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:587
/Users/tim/www/d8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:568
/Users/tim/www/d8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:628
/Users/tim/www/d8/core/modules/content_translation/src/ContentTranslationMetadataWrapper.php:51
/Users/tim/www/d8/core/modules/content_translation/tests/src/Kernel/ContentTranslationModuleInstallTest.php:78

ERRORS!
Tests: 1, Assertions: 1, Errors: 1.
~/www/d8$ git rebase origin/8.7.x 
First, rewinding head to replay your work on top of it...
Applying: patch
Using index info to reconstruct a base tree...
M	core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentSettingsTest.php
M	core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageContentSettingsTest.php
Falling back to patching base and 3-way merge...
~/www/d8$ ./vendor/bin/phpunit -c core core/modules/content_translation/tests/src/Kernel/ContentTranslationModuleInstallTest.php
PHPUnit 6.5.12 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\content_translation\Kernel\ContentTranslationModuleInstallTest
.                                                                   1 / 1 (100%)

Time: 2.11 seconds, Memory: 6.00MB

OK (1 test, 2 assertions)

So while this test seems like good addition, this indicates there is no longer a bug here.
If there are still additional problems not resolved by the above issue, then this needs new tests.

Looked into this. Both the patch and test are fine, as far as I can tell, but it looks like #2994398: Not properly clearing EntityFieldManager's fieldMap leads to fatals, often after migration or bundle creation exposed a separate bug. Previously the test would fail because the field content_translation_source was unknown, because the list of field storage definitions was still cached. Now that that's being cleared, what should happen is that the test should fail because it tries to save a record to the content_translation_source column which doesn't exist. The column does not, in fact, exist in the test. The reason that the test still passes is that - even though the field storage definitions in EntityFieldManager are now rebuilt correctly, the storage handler itself still holds a reference to a DefaultTableMapping object with the old storage definitions. So when saving the entity and building the database record to save in SqlContentEntityStorage::mapToStorageRecord(), $table_mapping->getFieldNames($data_table) will never contain the new field names.

So attached is a hacked together interdiff to make the test fail by rebuilding the table mapping when a bundle is created (or deleted). Because re-initializing the table mapping requires fetching the field storage definitions from EntityFieldManager I also needed to twist the order of the cache clearing. This should be fixed in a separate issue, though. Attaching here for now, though, in case someone wants to tinker with this.

I think we can still go ahead here independently of that, though, by simply extending the test. Since the translation metadata is never persisted to the database, we can trigger a test fail simply by asserting that the correct values are returned after saving and re-loading. So attached is a patch for that, that should fail. I didn't rebase the patch that includes the actual fix yet.

Opened #3002138: Table mapping is not re-initialized after bundles are created/deleted for #72 and rebased patch. This already includes the interdiff from #72, so should be RTBC-able as it was already reviewed above, but would be good to get some confirmation here that my thinking on #72 is accurate, so going to "needs review" for now.

I encountered the same error after trying to install a site from config and the config contains a view with a dependency to a non-existing (content) term:

dependencies:
  config:
    - node.type.subsite
    - taxonomy.vocabulary.subsite_type
  content:
    - 'taxonomy_term:subsite_type:3017e7d3-395a-4e66-a6c9-3b43f59b4393'
  module:
    - node
    - taxonomy
    - user

The error:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'content_translation_source' in 'field list': INSERT INTO {taxonomy_term_field_data} (tid, vid, langcode, status, name, description__value, description__format, weight, changed, default_langcode, content_translation_source, content_translation_outdated, content_translation_uid, content_translation_created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13);

I tried fixing it with a hook_install() that creates the term with that specific uuid.

Not sure what just happened but I did not mean to set this issue to needs rework nor did I want to re-add the label needs followup. Restoring both.

Patch for work with this issue :

https://www.drupal.org/project/drupal/issues/2868294#comment-12645399

How can I runnapudate if in the update.php page I have this alert "No pending updates."

How can I runnapudate if in the update.php page I have this alert "No pending updates."?

I have re-created the patch to support Drupal 8.6.1 version.

I have updated the patch for the version 8.6.1

Tested the #83 patch and it worked for me on 8.6.1. Setting this as Needs Review so other people can test it out.

Thanks @nkoporec .

Re @psf_ and @Poornachandran: I have just tested the following:

1. I applied the "tests-only" patch from #72
2. I enabled the content_translation_test module so that I had a translatable entity bundle but not the database columns such as content_translation_source (which is what this bug report is for)
3. I then removed that patch and applied the "full" patch from #74
4. I then disabled the translatability of the bundle on /admin/config/regional/content-language and pressed Save configuration
5. I then re-enabled the translatability and pressed Save configuration again. Now the database columns such as content_translation_source were properly created in the entity_test_with_bundle_field_data table.
6. I then ran drush entity-updates but there were no remaining updates.

So as far as I can tell this patch solves both the original bug, but also allows people to recover a broken site by disabling and re-enabling the translatability of each bundle. If that's not the case for anyone please provide additional details.

Otherwise someone maybe can RTBC this one. ;-) Re-uploading the latest patch for 8.7.x for that purpose.

Looks good to me. This also fixed a problem in my project where I have complex kernel tests that install a whole bunch of modules including content_moderation and those tests are failing on inconsistencies in field definitions when rebuilding views data.

Just wondering about whether we also need to take care of *uninstalling* those fields in case content moderation is disabled again? If this has been discussed before, just point me to the comment :)

Re #89:

1. I assume you mean s/content_moderation/content_translation/?
2. Not sure which of the following you are talking about:
   1. When uninstalling the content_translation module, all fields will be removed again by ModuleInstaller::uninstall()
   2. When disabling translatability for a bundle we purposely leave around the fields to preserve the translation metadata in case translatability is re-enabled. I'm not sure that's necessarily a good idea, but that's how it is and it is explicitly documented as a feature currently. See the following hunk in content_translation_entity_base_field_info():
      

          // We return metadata storage fields whenever content translation is enabled
          // or it was enabled before, so that we keep translation metadata around
          // when translation is disabled.
          // @todo Re-evaluate this approach and consider removing field storage
          //   definitions and the related field data if the entity type has no bundle
          //   enabled for translation.
          // @see https://www.drupal.org/node/2907777
          if ($manager->isEnabled($entity_type_id) || array_intersect_key($definitions, $installed_storage_definitions)) {
            return $definitions;
          }
      

Does that answer your question?

Yes, 2. is exactly what I was asking about, thanks.

Looks good then!

Hello,

tested the patch https://www.drupal.org/files/issues/2018-10-01/2599228-SQL_error_on_cont... against core at version 8.6.2. Unfortunately it doesn't work for me as somehow when content_translation_language_content_settings_update runs $installed_storage_definitions end up being exactly the same as the ones in $storage_definitions. Can't say yet where the problem is, but this is the field_data table i end up having for my custom entity:

+------------------+------------------+------+-----+---------+-------+
| Field            | Type             | Null | Key | Default | Extra |
+------------------+------------------+------+-----+---------+-------+
| id               | int(10) unsigned | NO   | PRI | NULL    |       |
| langcode         | varchar(12)      | NO   | PRI | NULL    |       |
| user_id          | int(10) unsigned | NO   | MUL | NULL    |       |
| name             | varchar(50)      | YES  |     | NULL    |       |
| status           | tinyint(4)       | NO   |     | NULL    |       |
| created          | int(11)          | YES  |     | NULL    |       |
| changed          | int(11)          | YES  |     | NULL    |       |
| default_langcode | tinyint(4)       | NO   |     | NULL    |       |
+------------------+------------------+------+-----+---------+-------+

so seems i am missing the columns content_translation_source and content_translation_outdated? or is there anything more i should i guess add manually with a hook update?
thanks for helping me out with this :)

edit: actually, when reading better at the error, the fields content_translation_uid, content_translation_status, content_translation_created, content_translation_changed are also missing, so, well... a lot of adds to be done!

edit again: disabling and re-enabling the custom module and then resaving the form at "admin/config/regional/content-language" fixed the issue. Leaving the comment here to add the suggestion then... if you still have problems, try to disable your custom entity module and redo the thing :)

Re-roll only patch for 31st comment . This is compatible with latest 8.6.x branch.
Thanks

Looks like this also fixes #2868294: Call to a member function getThirdPartySetting() on null in ContentTranslationManager. The latest patch has php errors, so should not be used.

Solved problem with duplicated methods in .module that causes that PHPLint failed.

Patches in #93 and #96 are smaller in 5k comparing to #86

Re-uploading the RTBC patch.

Here's the properly-rerolled patch now that #3008740: ContentTranslationManager::setEnabled no longer clears the bundle info cache when changing a bundle's translation setting. already introduced the insert and update hooks for language content settings. I didn't look at the failures in #93 so I ignored #96 which fixed those - because my patch from ##86 still applied. That was ignorant of me, I'm sorry for that. Those patches did miss the additions to the content_translation_test module, though. Also #96 incorrectly removed the (now) pre-exixting hooks which caused the test failures. So theoretically this one should again be green, and - but for the rebase - is identical to #86, so I'm marking this RTBC again.

+++ b/core/modules/content_translation/content_translation.module
@@ -177,7 +178,29 @@ function content_translation_entity_type_alter(array &$entity_types) {
+    /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $field_manager */
+    $field_manager = \Drupal::service('entity_field.manager');
+    /** @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface $schema_repository */
+    $schema_repository = \Drupal::service('entity.last_installed_schema.repository');
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $entity_type_id = $settings->getTargetEntityTypeId();
+
+    $field_manager->useCaches(FALSE);
+    $storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type_id);
+    $field_manager->useCaches(TRUE);
+    $installed_storage_definitions = $schema_repository->getLastInstalledFieldStorageDefinitions($entity_type_id);
+
+    foreach (array_diff_key($storage_definitions, $installed_storage_definitions) as $storage_definition) {
+      /** @var $storage_definition \Drupal\Core\Field\FieldStorageDefinitionInterface */
+      if ($storage_definition->getProvider() == 'content_translation') {
+        $definition_update_manager->installFieldStorageDefinition($storage_definition->getName(), $entity_type_id, 'content_translation', $storage_definition);
+      }
+    }

@@ -189,7 +212,32 @@ function content_translation_language_content_settings_insert(EntityInterface $e
+    /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $field_manager */
+    $field_manager = \Drupal::service('entity_field.manager');
+    /** @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface $schema_repository */
+    $schema_repository = \Drupal::service('entity.last_installed_schema.repository');
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $entity_type_id = $settings->getTargetEntityTypeId();
+
+    $field_manager->useCaches(FALSE);
+    $storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type_id);
+    $field_manager->useCaches(TRUE);
+    $installed_storage_definitions = $schema_repository->getLastInstalledFieldStorageDefinitions($entity_type_id);
+    foreach (array_diff_key($storage_definitions, $installed_storage_definitions) as $storage_definition) {
+      /** @var $storage_definition \Drupal\Core\Field\FieldStorageDefinitionInterface */
+      if ($storage_definition->getProvider() == 'content_translation') {
+        $definition_update_manager->installFieldStorageDefinition($storage_definition->getName(), $entity_type_id, 'content_translation', $storage_definition);
+      }
+    }

This code looks to be exactly the same. I wonder if for now we should add an _SOME_NAME_METHOD() to the module file as a bug in one is likely to be a bug in the other.

Also in the original code that does this has an @todo to https://www.drupal.org/node/2346013 which for me is still relevant.

Good point, that's a great idea to introduce a common function. Since the common logic only depends on the entity type ID I used that as an argument so that the new function does not need to know about the LanguageContentSettings entity.

Also good point regarding the @todos. Moved the one that was still valid over and removed the other one. I think that can be seen as part of the deprecation done here, as it doesn't really make sense to have @todos on deprecated code, so I hope is not considered out of scope.

Sorry.

I have the same issue on 8.6.x and #104 works fine with a re-work patch. DO NOT use this in production, it's a port patch for 8.6.x on an on progress issue.

@GoZ if you've verified that the patch was updated and correctly, maybe you can set it back to RTBC, since it was already reviewed multiple times above and the latest comments have been adressed? That would be awesome.

There is a piece of code, which can be put into /Drupal/Core/Database/Connection.php::handleQueryException() after $message=...;
It's a ugly part of code, highly misplaced, but it's doing one job - fixing broken database at first error encounter. This is not a solution, just workaround.

Note, that mysql database driver is using a quirk, to check that field exists in database, in web/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php::fieldExists(), which catch's exemption as a sign, that this field not exists.

        if( strpos($message, '_translation_source') ){
            if( isset( $GLOBALS['skip_trans_fix'] ) && $GLOBALS['skip_trans_fix']  ){
                //echo  $message;
                throw new \Exception('Field not exists');
            }
            echo  $message;
            echo 'Trying to fix that, try again after while...';

            $field_manager = \Drupal::service('entity_field.manager');
            $definition_update_manager = \Drupal::entityDefinitionUpdateManager();

            $entityTypeManager = \Drupal::entityTypeManager();
            $types = array_keys( $entityTypeManager->getDefinitions() );

            foreach( $types AS $entity_type_id ){
                if( strpos($message, '{'.$entity_type_id) == false ) continue;

                //$entityTypeManager->clearCachedDefinitions();
                $entity_type = \Drupal::service('entity_type.manager')->getDefinition($entity_type_id);
                \Drupal::service('entity.definition_update_manager')->updateEntityType($entity_type); //Param should be entity type interface, not string
                \Drupal::service('entity.definition_update_manager')->applyUpdates();

                $field_manager->useCaches(FALSE);
                try {
                    $storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type_id);
                } catch (\LogicException $e) {
                    //echo 'Caught exception: ',  $e->getMessage(), "\n";
                    continue;
                }
                $field_manager->useCaches(TRUE);

                $GLOBALS['skip_trans_fix'] = true; //Cause we do not need fall into endles loop ;)
                foreach( $storage_definitions AS $name=>$storage_definition ){
                    if( strpos( $name, '_translation_' ) ){
                        $definition_update_manager->installFieldStorageDefinition($storage_definition->getName(), $entity_type_id, 'content_translation', $storage_definition);
                    }
                }
                unset( $GLOBALS['skip_trans_fix'] );
            }//end foreach
        }//end translation_source missing fixer

Let's go RTBC

We need a re-roll here.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -169,6 +170,77 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +  // @see \Drupal\content_translation\ContentTranslationManager::isEnabled()
   

   @see's are best in the docblock

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -169,6 +170,77 @@ function content_translation_entity_type_alter(array &$entity_types) {
   + /**
   +  * Implements hook_ENTITY_TYPE_update().
   +  *
   +  * Installs Content Translation's field storage definitions for the target
   +  * entity type, if required.
   +  *
   +  * Also clears the bundle information cache so that the bundle's translatability
   +  * will be changed properly.
   +  *
   +  * @see content_translation_entity_bundle_info_alter()
   +  */
   +function content_translation_language_content_settings_update(ContentLanguageSettingsInterface $settings) {
   +  /** @var \Drupal\language\ContentLanguageSettingsInterface $original_settings */
   +  $original_settings = $settings->original;
   +  // @see \Drupal\content_translation\ContentTranslationManager::isEnabled()
   +  if ($settings->getThirdPartySetting('content_translation', 'enabled', FALSE)
   +    && !$original_settings->getThirdPartySetting('content_translation', 'enabled', FALSE)
   +  ) {
   +    _content_translation_install_field_storage_definitions($settings->getTargetEntityTypeId());
   +  }
   +
   +       \Drupal::service('entity_type.bundle.info')->clearCachedBundles();
   +     }
   

   The formatting here is a bit off.

   FILE: ....dev/core/modules/content_translation/content_translation.module
   ----------------------------------------------------------------------
   FOUND 6 ERRORS AFFECTING 5 LINES
   ----------------------------------------------------------------------
    191 | ERROR | [x] Expected 1 blank line after function; 0 found
    192 | ERROR | [x] Line indented incorrectly; expected 0 spaces,
        |       |     found 1
    213 | ERROR | [x] Line indented incorrectly; expected 2 spaces,
        |       |     found 7
    214 | ERROR | [x] Line indented incorrectly; expected 0 spaces,
        |       |     found 5
    214 | ERROR | [x] Closing brace indented incorrectly; expected 0
        |       |     spaces, found 5
    241 | ERROR | [x] Expected 1 blank line after function; 2 found
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -169,6 +170,77 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +}
   +
   +
    /**
   

   Two lines aren't necessary here.

4. +++ b/core/modules/language/migrations/d6_language_content_settings.yml
   @@ -40,8 +40,6 @@ process:
    destination:
      plugin: entity:language_content_settings
   -  content_translation_update_definitions:
   -    - node
   
   +++ b/core/modules/language/migrations/d7_language_content_settings.yml
   @@ -43,8 +43,6 @@ process:
    destination:
      plugin: entity:language_content_settings
   -  content_translation_update_definitions:
   -    - node
   

   We need to add this to the change record to say that this is no longer needed and is now ignored.

Also we need to base this on 8.7.x first.

See comment from @Taran2L at "Call to a member function getThirdPartySetting() on null in ContentTranslationManager"
https://www.drupal.org/project/drupal/issues/2868294#comment-12919604

Seems like the in order to fix the issue both patches are required. One from here and one from #2599228-105: Programmatically created translatable content type returns SQL error on content creation. The problem is that they cannot be applied together. I think those two issues should be combined into a single one.

Porting this patch to 8.7.x and solving some problems comment at #109

Here's an interdiff for the patch in #112

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -172,27 +173,72 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +  // @see \Drupal\content_translation\ContentTranslationManager::isEnabled()
   

   Seems like this one has not been addressed yet:

   @see's are best in the docblock

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -172,27 +173,72 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +    /** @var $storage_definition \Drupal\Core\Field\FieldStorageDefinitionInterface */
   

   Variable should go after type declaration

3. +++ b/core/modules/content_translation/src/ContentTranslationUpdatesManager.php
   @@ -2,19 +2,20 @@
   + * @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.0.
   

   Should be 8.7.x now

CR updated: https://www.drupal.org/node/2973222

I've addressed all issues from my previous comment and found one more migration that needed update. Please find updated patch

Arghh, another small update :/

I think #109 was a review of #105 and not #104, which confused me a bit. In any case the changes since then all look good to me and I did review the whole patch again to make sure that with all the back and forth between 8.6 and 8.7 everything is as it should be. The update to the change notice also looks good. Since I didn't work on this since it was last RTBC, I can safely RTBC this again.

OK, this conflicted with #2977107: Use more specific entity.manager services in module .services.yml files in a rather unfortunate way, which made the rebase a bit more involved. Thus, going back to needs review so that someone verifies that my thinking is correct on this.

Two points are noteworthy:

1. >Because this issue deprecates ContentTranslationUpdateManager entirely I removed the DeprecatedServicePropertyTrait and the deprecation messages in the constructor. I don't think it makes sense to warn people about the incorrect (deprecated) usage of something while at the same warning them that they shouldn't be using the thing in the first place. So I'm rather confident about this, but I'm still not 100% sure.
2. This one is a bit more tricky: The issue mentioned above added a third parameter to the constructor of ContentTranslationManager and made it optional with a deprecated BC layer if it is not provided. This issue, however, removes the second parameter of the constructor, because that second parameter is a now-deprecated (and unused) service. So I first thought to minimize the amount of change introduced here, to support explicitly passing NULL as the second parameter and throw a deprecation warning in case someone was still passing an actual instance of ContentTranslationUpdateManager. However, that would mean that we would make passing an explicit NULL as the second parameter the recommended way of using that service including in Drupal 9. Instead, the actually preferred way of instantiating the service should be new ContentTranslationManager($entity_type_manager, $entity_type_bundle_info) as those are the services that are actually needed. So I implemented it in that way, which - together with backwards-compatibility for all previous instantiation patterns - lead to a couple of things:
   1. Because we still need to support passing $updates_manager as the second parameter we cannot typehint EntityTypeBundleInfoInterface for $entity_type_bundle_info.
   2. If the second parameter is in fact an instance of ContentTranslationUpdateManager we should assume that either the third parameter is the entity type bundle info or there is no third parameter at all. This could have been solved with a third optional parameter which will be removed in Drupal 9, but I chose to implement this func_get_args() instead as - even though that might be regarded less elegant in terms of the code itself - I found it less confusing for the end user who is reading the documentation, especially together with the other change.

Ahh because of the "missing" typehint EntityTypeBundleInfoInterface is now unused in ContentTranslationManager, sorry. Will fix that once there is agreement regarding #121.

Hi @tstoeckler, thanks for the updated patch! The sequence of events is a bit unfortunate, I totally agree!

I've spent some time checking it. Here's my outcome:

1. Personally I like your point above removing errors in already deprecated code that should not be used in the first place
2. The mentioned patch has been committed to 8.7.x only, which does not have a release yet. So I think we should not support BC for something that is in dev mode at this point. As long as this issue is committed before 8.7.0 we should be good.

• ContentTranslationManager service should support only 2 instantiation patterns:
  • original one (present in 8.6.x) with ['@entity_type.manager', '@content_translation.updates_manager']
  • new one (introduced in 8.7.x) with ['@entity_type.manager', '@entity_type.bundle.info']
• The implementation should accept only 2 parameters (no need to support dev version with 3)
• Trigger deprecation warning if 2nd parameter is ContentTranslationUpdateManager and get correct service directly from the container, like other code does
• There still be an issue with type hinting though

@Taran2L that's fine with me, your suggestion sounds sensible. I would like confirmation from a release manager, though, that this is in line with the release process. Tagging accordingly.

Sorry :)

Yes, we definitely don't need to support the 8.7-8.7 change, so that's fine, no release manager feedback needed.

Officially we don't have to support BC for constructors, but I did spend a lot of time being extra nice and careful in my entity.manager issues to keep breaking contrib/custom due to such changes to an absolute minimum, it's pretty annoying, especially if you can't write code that works with *both* 8.6 and 8.7.

My suggestion would be to do something similar to what I tried for SystemManager: #3025152: Remove entityManager constructor argument from SystemManager:

Like #124 suggested, make the constructor: ['@entity_type.manager', '@entity_type.bundle.info'] but then don't add the type hint for the EntityTypeBundleInfo service and in the code, add if (!($entity_type_bundle_info instanceof ...)) { @trigger_error() + assign from \Drupal::service(). ).

You could even update the configuration for the DeprecatedServicePropertyTrait to not break a subclass that would still access the removed property.

OK, sounds good. Will implement unless someone beats me to it.

OK, here we go. Good idea in #126, I didn't think of that! 👍🏿

This does look good to me! Not sure, if I should set it to RTBC officially, but at least I can vote for it.

+1 to RTBC

patch #105 works for me against Core 8.6.9
I had to uninstall and reinstall content translation module
+ enable translation on a content type

+1 to RTBC

re-rolled the patch from #105 against 8.6.x. simply formatting.

+1 RTBC

also see the combined patch at #2868294-61: Call to a member function getThirdPartySetting() on null in ContentTranslationManager

The quick workaround for this bug has been to call $entity_definition_update_manager->applyUpdates(), that was for example done in multilingual_demo_install() and it was about to get into core with #3037111: Import multilingual content into Umami.

#2976035: Entity type CRUD operations must use the last installed entity type and field storage definitions broke that one-line workaround, now a workaround would need to explicitly install all the extra field storage definitions. As annoying as that might be right now, I think that is a good thing, it shows perfectly just how much applyUpdates() was misused to work around bugs and other weird things with possibly very problematic side effects.

I think it is important that this is fixed in 8.7.x, to unblock the umami issue so that it doesn't require those workarounds. Only getting it into 8.8 would also make the constructor changes that were done here much more complicated, they were changed in 8.7, we can just remove parts again, but we couldn't do that in 8.8.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -172,27 +173,72 @@ function content_translation_entity_type_alter(array &$entity_types) {
   + *
   + * @todo Generalize this code in https://www.drupal.org/node/2346013.
   + */
   +function _content_translation_install_field_storage_definitions($entity_type_id) {
   

   I was wondering whether this should be on a service instead of adding new functions, but it is called by hooks, so if those ever become hooks, it would be easy to move the helper function to the same event listener instead of making it a public API (it is basically the whole point of this issue to avoid having an API that needs to be called).

   I'm not sure if the @todo here is very helpful though. that issue hasn't been updated since 2015 and likely won't go anywhere anytime soon. work is happening in other issues, and if we do provide an improved API, we'll likely deprecate the old one, in which case we'd be forced to update this anyway.

   So I'd suggest to just remove that.

2. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
   @@ -33,28 +36,19 @@ class ContentTranslationManager implements ContentTranslationManagerInterface, B
   -    $this->updatesManager = $updates_manager;
   -    if (!$entity_type_bundle_info) {
   -      @trigger_error('The entity_type.bundle.info service must be passed to ContentTranslationManager::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   +
   +    if ($entity_type_bundle_info instanceof ContentTranslationUpdatesManager) {
   +      @trigger_error('The entity_type.bundle.info service must be passed to ContentTranslationManager::__construct() instead of the content_translation.updates_manager service. This will be required in Drupal 9.0.0. See https://www.drupal.org/node/2549139 and https://www.drupal.org/node/2973222.', E_USER_DEPRECATED);
          $entity_type_bundle_info = \Drupal::service('entity_type.bundle.info');
        }
   

   since we touch it, maybe we should add the info that this should be done since 8.7.0 like more recent constructor deprecation messages.

Other than that, I think this looks great.

Thanks for the review! Agree that it would be great to have this in 8.7.x. If I personally had a slot for marking an issue as critical without other people being allowed to challenge that decision, this issue would be it ;-)

Re 1.: @alexpott explicitly asked for "leaving" that todo in (i.e. moving it from its old location) in #101 so I am erring on the side of having it where we can always remove it rather that not having it.

Re 2.: I slightly changed the deprecation message, I hope that's what you meant.

Oops, forgot to upload the interdiff.

Thanks, lets get this in. No strong feelings on the @todo, just doesn't seem very useful to me but if others prefer it, fine.

Yes, not sure about critical or major. As I said, it is a good thing that the workaround is gone and we have to fix it now to unblock official initatives. The downside of official initiatives is that everything else becomes less important ;)

Nice simplification!

1. +++ b/core/modules/content_translation/content_translation.services.yml
   @@ -29,10 +29,9 @@ services:
   +    deprecated: The "%service_id%" service is deprecated. Definitions are updated automatically now so no replacement is needed. See https://www.drupal.org/node/2973222.
   
   +++ b/core/modules/content_translation/src/ContentTranslationManager.php
   @@ -33,28 +36,19 @@ class ContentTranslationManager implements ContentTranslationManagerInterface, B
   +    if ($entity_type_bundle_info instanceof ContentTranslationUpdatesManager) {
   +      @trigger_error('The entity_type.bundle.info service should be passed to ContentTranslationManager::__construct() instead of the content_translation.updates_manager service since 8.7.0. This will be required in Drupal 9.0.0. See https://www.drupal.org/node/2549139 and https://www.drupal.org/node/2973222.', E_USER_DEPRECATED);
   
   +++ b/core/modules/content_translation/src/ContentTranslationUpdatesManager.php
   @@ -2,28 +2,22 @@
   +@trigger_error('\Drupal\content_translation\ContentTranslationUpdatesManager is scheduled for removal in Drupal 9.0.0. Definitions are updated automatically now so no replacement is needed. See https://www.drupal.org/node/2973222.', E_USER_DEPRECATED);
   ...
     * Provides the logic needed to update field storage definitions when needed.
   + *
   + * @deprecated in Drupal 8.7.x, to be removed before Drupal 9.0.0.
   + *   Definitions are updated automatically now so no replacement is needed.
   + *
   + * @see https://www.drupal.org/node/2973222
   

   Please let's add a unit test ensuring deprecation errors are triggered whenever we 1) pass the update manager to the CTM constructor, 2) the update manager is instantiated, 3) the service is retrieved, per https://www.drupal.org/core/deprecation.

2. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
   @@ -33,28 +36,19 @@ class ContentTranslationManager implements ContentTranslationManagerInterface, B
        $this->entityTypeBundleInfo = $entity_type_bundle_info;
   

   Now we need an instanceof check to ensure that $entity_type_bundle_info is actually a bundle info object.

3. +++ b/core/modules/content_translation/tests/src/Kernel/ContentTranslationModuleInstallTest.php
   @@ -0,0 +1,88 @@
   +    $entity = EntityTestWithBundle::load($entity->id());
   

   Can we retrieve the unchanged entity to avoid static caching?

I briefly discussed our deprecation testing policies with @catch and we're not sure this warrants a unit test. Moving back to RTBC, however a new patch addressing the nitpicks in #138 would be great to have :)

1. Yeah, As mentioned on slack, I've never done unit tests for constructor deprecations, I think you were the only one who ever did that :) I think that's overkill unless it's really complex/ a very common base class, it's pretty unlikely that someone subclassed this.
2. What about inverting the instanceof check to trigger if it's not the new interface instead of triggering on the old one?
3. I think that's not necessary because this is a kernel test, Static cache is automatically reset then, that stuff is just necessary for web tests. And now that we have a entity static cache service, I think it would be nice if we could implement a reset like we have for other static caches on POST requests.

Thanks @Berdir! The interdiff looks good to me.

Tested #140 for #3040859: Undefined index: content_translation_source (Failures in Drupal\paragraphs_demo\Tests\ParagraphsDemoTest) and #3040863: Failures in \Drupal\Tests\lingotek\Functional\LingotekNodeWithParagraphsManageTranslationTabTest on D8.7.x and above and it works.

Crediting @psf_, @dxvargas, @nkoporec, @joey-santiago for patch testing
@alexpott, @plach for patch review
@andy@andyhawks.com for creating the issue

Committed and pushed 46424a89ef to 8.8.x and ad528c7375 to 8.7.x. Thanks!

Since this commit I'm getting 'LogicException: Getting the base fields is not supported for entity type Contact form.' when doing:
\Drupal::service('content_translation.manager')->setEnabled('contact_form', $this->contactForm->id(), TRUE);

This looks like the expected behavior as far as we cannot make contact forms translatable in the UI. We might be missing some checks somewhere else and that setEnabled did nothing, but didn't fail as it does after this commit. Just documenting it in case someone else gets this error now.

@penyaskito contact_form is the config entity for the contact_message, that line never made any sense, no idea how it didn't fail before.