Problem

Drupal 7 had two translation systems, node copy based content translation with a core UI and field based translation with a core API and a contrib UI. Drupal 8 attempts to standardize on field translation (which applies to entities of all kinds not just nodes). However, Drupal 8 still has both modules in the codebase. The legacy module is hidden from sight for new sites. We are not providing upgrade path in 8.0.

Proposal

Remove the module from the codebase, and tests that test it.

Dependencies

none.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

plach’s picture

Linked this in #1836086: [meta] Entity Translation UI improvements.

We tried to bring on the comment stuff during the sprint weekend, but we were stopped: #1938096-4: Convert the node comments list to a view. It seems we should ping @amateescu to understand if we can rely on him to fix it.

chrishks’s picture

From line 2044 in comment 303 on #1498674-303: Refactor node properties to multilingual - remove this code

+      // @todo Remove the following columns when removing the legacy Content
+      //   Translation module. See http://drupal.org/node/1952062.
+      'tnid' => array(
+        'description' => 'The translation set ID for this node, which equals the node ID of the source post in each set.',
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+        'default' => 0,
+      ),
+      'translate' => array(
+        'description' => 'A boolean indicating whether this translation page needs to be updated.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),
+    ),
+    'indexes' => array(
+      'node_type' => array(array('type', 4)),
+      'tnid' => array('tnid'),
+      'translate' => array('translate'),
+    ),
+    'unique keys' => array(
+      'vid' => array('vid'),
+      'uuid' => array('uuid'),
+    ),
+    'foreign keys' => array(
+      'node_revision' => array(
+        'table' => 'node_field_revision',
+        'columns' => array('vid' => 'vid'),
+      ),
+    ),
+    'primary key' => array('nid'),
+  );
+  // Node field storage.
+  $schema['node_field_data'] = array(
+    'description' => 'Base table for node properties.',
+    'fields' => array(
+      'nid' => array(
+        'description' => 'The primary identifier for a node.',
+        'type' => 'serial',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+      ),
+      'vid' => array(
+        'description' => 'The current {node_field_revision}.vid version identifier.',
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+      ),
+      'type' => array(
+        'description' => 'The {node_type}.type of this node.',
+        'type' => 'varchar',
+        'length' => 32,
+        'not null' => TRUE,
+        'default' => '',
+      ),
+      'langcode' => array(
+        'description' => 'The {language}.langcode of these node property values.',
+        'type' => 'varchar',
+        'length' => 12,
+        'not null' => TRUE,
+        'default' => '',
+      ),
+      'default_langcode' => array(
+        'description' => 'Boolean indicating whether the property values are in the {language}.langcode of this node.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 1,
+      ),
yched’s picture

catch’s picture

Status: Postponed » Active

Could we move this to contrib for now? Even if this isn't completely replaceable yet it's very close, and shipping with it seems a lot worse than shipping without.

Gábor Hojtsy’s picture

Do you mean the upgrade path would be in contrib too? Or that we should work on that later? We did not have the opportunity to work on that since we cannot reproduce the functionality yet.

catch’s picture

I'm not sure on the upgrade path bit of this. Is there anything actually blocking an upgrade path in core from the point of view of the data itself or is it just that some features can't be replicated yet? We only guarantee people's data, not features, so that shouldn't block it IMO.

The main concern I have with this is that we end up with another profile module situation - it was marked hidden at the final hour of Drupal 7 because it made no sense to present it to new Drupal users, but we still have a critical issue open for an upgrade path even now.

So if we keep translation module around, then end up marking it hidden just before release, we could be back here at the end of 9.x with it still not completely nuked yet.

Gábor Hojtsy’s picture

I think ATM the situation is more like the set of people who could write this were busy with building features when that was allowed, now busy with API changes while that is still allowed and then if the upgrade path is possible we could focus on that after API changes / criticals are done.

plach’s picture

What Gabor said.

The main critical issue with the upgrade path is that we will be merging nodes, which sounds like a lot of potential pain. In contrib D7 the upgrade path is a standalone module that provides a UI, hooks, and a functionality to redirect from the old urls to the new ones:

it/node/1
en/node/2  -- redirect --> en/node/1

I am not sure whether it makes sense to have such code in core. It sounds much more like a D7 CCK thing, even if we don't provide a UI for it.

catch’s picture

Assigned: Unassigned » Dries

Assigning to Dries. It makes sense to me to have the upgrade path for this in contrib given it's not at all straightforward.

If the upgrade path is in contrib, then I think we could remove translation module now - I don't see any value to shipping Drupal 8 with both methods.

plach’s picture

As a side note @Jose Reyero is planning to revive the node translation module at https://drupal.org/project/translation.

Gábor Hojtsy’s picture

Priority: Normal » Critical
Issue tags: +D8 upgrade path

Marking as critical on the basis of it being a module removal and add upgrade path tag since this will set up an external upgrade path dependency for the eventual Drupal 8 release (much like it is with Views as far as I understand). This should be evaluated and executed or dismissed before the beta release.

Not sure how we track external upgrade path dependencies.

Dries’s picture

Assigned: Dries » Unassigned

I think it makes sense to remove the legacy approach from core. I'd be ok with fixing the upgrade path in contrib. If that proves to be problematic, we can always decide to move the ugprade path back to core.

Gábor Hojtsy’s picture

I've been thinking maybe git rm would be enough to do here (no patch needed), but there may be core hooks implemented that would need code removed from other parts of core. This may need a little bit of exploration.

plach’s picture

Shouldn't we wait to be able to translate titles with the Content Translation module?

Gábor Hojtsy’s picture

@plach: all the required issues for that are marked critical (if not, they should be), so we already committed to not shipping Drupal 8 without solving that. Also the legacy content translation module is already hidden on new installs, so practically users don't have any way to translate node titles at the moment. Removing the module would not make a difference for regular users on new installs at this point.

Gábor Hojtsy’s picture

Issue tags: +Upgrade path, +sprint

Tag for upgrade path and moved onto sprint. The hooks need to be found that are implemented for this feature (especially in node module but possibly in some field modules). It is likely not just a git rm.

CMS Dude’s picture

Assigned: Unassigned » CMS Dude

Assigning this to myself ; working at Midwest Drupal Summit -- first issue.

CMS Dude’s picture

Removing Translation Module

Double-checked to see if any other modules/code is calling on any of the old functions from the Translations module before deleting.

In #18 @Gábor Hojtsy

The hooks need to be found that are implemented for this feature (especially in node module but possibly in some field modules). It is likely not just a git rm.

In IDE, did a search for any function calls that match the string "translation_," as well as any hooks that match string "_translation" Found no instances of any hooks or other functions defined in old translation module.

CMS Dude’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal8.translation-module.1952062-20.patch, failed testing.

klonos’s picture

Assigned: CMS Dude » Unassigned
Status: Needs work » Needs review

...this usually helps ;)

klonos’s picture

Status: Needs review » Needs work

...xpost :/

YesCT’s picture

a bunch of fails are tests, upgrade path tests too, which are trying to enable the module.
we should edit those tests and not enable the module.
let us look at the tests and see if tests in content_translation are covering similar situations.

jair’s picture

Issue tags: +Needs reroll
manningpete’s picture

Rerolled.

manningpete’s picture

Status: Needs work » Needs review

Setting it for needs review for testbot to run. Still needs work.

Status: Needs review » Needs work

The last submitted patch, drupal8.translation-module.1952062-27.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Add new dependency.

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

Gábor Hojtsy’s picture

Looking at other modules, there are at least a few other things that should be removed:

- tnid from node table
- tnid based handling from views

Maybe there are other things that I did not yet find, this is just a quick look. The tnid schema actually references this issue in the code. Obviously we cannot remove the tnid data, we need it for migration in the contrib migration module. We should move it aside in the upgrade to an upgrade-only table, so the contrib upgrade module can still see it, but it is not in the live node table data.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
58.5 KB

removing translation module and permission related to it from few other test cases.

vijaycs85’s picture

Issue tags: +LONDON_2013_AUGUST

Adding tag...

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-32.patch, failed testing.

Gábor Hojtsy’s picture

Still fails, but less than above :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.18 KB
63.52 KB

It looks like test assumes that translation is separate node. So had to hardcode language code few places. Issuing patch to make sure in right direction.

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-36.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
61.95 KB

Re-rolling with current code base.

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-38.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
765 bytes
61.95 KB

Fixing drupalPost() issue.

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-40.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php
@@ -71,11 +86,12 @@ function testAliasTranslation() {
+    $edit['body[fr][0][value]'] = $this->randomName();

I think this is now outdated, see #2083811: Remove langcode nesting for fields in entity $form structures, which should explain the fail :)

Other fails where eg. the upgrade path tests for renaming of permission form translate content to translate all content are just outdated. Since the update function is removed, there is no point in testing for that. That is an orphaned permission.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
66.89 KB

Thanks for the update @Gábor Hojtsy. Issuing new patch with below changes:

1. Removed upgrade path test - As it is trying to check different translation nid which is not anymore case in D8.
2. Updated LanguagePath test to get the latest code from #2083811: Remove langcode nesting for fields in entity $form structures

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-43.patch, failed testing.

willyk’s picture

I reviewed and tested the last submitted patch and it appears that including translation in ViewTestBase still causes the errors in HandlerAllTest due to some of the likely deeper views hander issue. Wondering if we should remove it there just to see if it prevents the failure?

willyk’s picture

Actually correction to the above #45, it appears that modifying/updating HandlerAllTest.php so that it doesn't enable 'translation' should resolve what's causing 1952062-translation-module-43.patch to fail testing.

willyk’s picture

As mentioned above, updating HandlerAllTest.php to enable 'translation_content' rather than 'translation' in order to try and resolve what's causing 1952062-translation-module-43.patch to fail testing. Hopefully this passes now.

willyk’s picture

@Gábor Hojtsy, just in case you want to remove from testing entirely, here's another patch that removes the old translation, as opposed to updating what's enabled as in the above 1952062-translation-module-47.patch.

I'm guessing the patch in 47 is preferred approach; but figured I add this one as well, since I wasn't certain the preferred approach given the status/timing we discussed yesterday. If either patch passes, perhaps you could choose whichever serves the purpose.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-48.patch, failed testing.

willyk’s picture

Apologies for the error in 1952062-translation-module-48.patch. The issue there should be corrected and the other two translation instances mentioned by @vijaycs85 should also be addressed in this revised patch.

vijaycs85’s picture

Status: Needs work » Needs review
pfrenssen’s picture

Status: Needs review » Needs work
+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -30,8 +30,20 @@ public static function getInfo() {
+      'edit any page content',
+      'create page content',
+      'administer url aliases',
+      'create url aliases',
+      'administer languages',
+      'access administration pages',
+      'administer content types',
+      'create content translations',
+      'administer content translation',
+      'translate any entity'

Order them alphabetically now that we have a nice list?

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -43,6 +55,11 @@ function setUp() {
+
+    // Enable translation for page node.
+    $edit = array('entity_types[node]' => 1, 'settings[node][page][translatable]' => 1, 'settings[node][page][fields][body]' => 1, 'settings[node][page][settings][language][language_show]' => 1);
+    $this->drupalPostForm('admin/config/regional/content-language', $edit, t('Save'));

Move this to the top of testAliasTranslation. See the comment there.

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -89,7 +106,8 @@ function testAliasTranslation() {
-    $french_node = $this->drupalGetNodeByTitle($edit["title"]);
+    $french_node =  $english_node->getTranslation('fr');
+

An unneeded empty line is added, as well as a double space after the '='.

willyk’s picture

Can you please clarify what you mean by "Move this to the top of testAliasTranslation. See the comment there." I don't see the comment you're referring to and am not following what you mean by the move. Ping me @willyk in #drupal-i18n if it's easier to discuss there. An updated patch is attached that includes the other two changes.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

The patch in #54 does not apply:

$ git apply 1952062-translation-module-54.patch
fatal: corrupt patch at line 83

I will roll a valid patch.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
1.92 KB
69.45 KB

Ignore my comment in #53 about moving the code block that enables translations for Page nodes. Looking closer the rest of the related configuration is also done in setUp(), so it is more consistent to keep it there.

Status: Needs review » Needs work

The last submitted patch, 1952062-translation-module-56.patch, failed testing.

willyk’s picture

Status: Needs review » Needs work
FileSize
69.45 KB

Attached in another patch that that addressed the problem that caused #54 to fail. It should now apply if someone wants to review :).

tim.plunkett’s picture

Status: Needs work » Needs review

I bet the bot will test!

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
948 bytes

Here's the interdiff between #56 and #58. Only some whitespace changed. The line that was added was not needed, and the one that was removed is usually left in place to indicate the end of a class definition. Whitespace-wise, #56 is a better patch :)

The failure in #56 is probably not related to the patch.

pfrenssen’s picture

vasi1186’s picture

Removed some more code.

vasi1186’s picture

and the interdiff

vasi1186’s picture

Gábor Hojtsy’s picture

Woot, great diffstat!

25 files changed, 31 insertions(+), 1464 deletions(-)

:)

Gábor Hojtsy’s picture

penyaskito’s picture

For reviewing this, I searched for "module_exists('translation')". I found another reference to translation module in hook_system_info_alter implementation in node and I removed it.
I rgrep for 'tnid' and the only reference is ./core/modules/system/tests/upgrade/drupal-7.field.database.php, and that's OK.

So the attached patch only removed this hook_system_info_alter implementation in node.module.

There are still references to the translations paths in node_admin_paths, maybe this should be removed too or moved to content_translation module?

Gábor Hojtsy’s picture

What are those admin paths? Sounds like they may not apply to the other module anymore (or it has them already)?

penyaskito’s picture

As we saw, they are the node/{nid}/translations, and they belong to node module.
So everything is OK from my POV. Reviews required.

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
75.19 KB
1.98 KB
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.php
@@ -132,7 +132,7 @@ public function submitForm(array &$form, array &$form_state) {
-          $this->field['entity_type'],
+          $this->fieldInfo->entity_type,

Completely unrelated change, so I'm removed it.

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php
@@ -43,6 +55,11 @@ function setUp() {
+    $edit = array('entity_types[node]' => 1, 'settings[node][page][translatable]' => 1, 'settings[node][page][fields][body]' => 1, 'settings[node][page][settings][language][language_show]' => 1);

This is just a minor code style issues. Fixed it.

Otherwise it seems good.

Manually tested installation process and one translation of a node, to be sure that nothing interfered.
RTBC if testbot not disagrees (only minor code styling change)

kfritsche’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

since we are not doing upgrade path

catch’s picture

Title: Remove legacy translation module in favor of content translation » Change notice: Remove legacy translation module in favor of content translation
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: -sprint +Needs change record

This is great. We already marked this module hidden for new installs, and with a migrate-based upgrade path the route from different nodes with tnids to translatable nodes will be a new source + some tweaking rather than a very complicated hook_update_N(). This needs to go in before we offer 8-8 updates due to the schema changes in node module itself.

Committed/pushed to 8.x, thanks!

I think we already have a change notice relating to this, but will either need to update it or create a new one if it's not covered properly yet.

vijaycs85’s picture

Thanks @catch. I guess, we need to replace/update documentation on http://drupal.org/documentation/modules/translation_entity page. (including URL change and redirect ). Created new follow up to update this link in content_translation_help at #2102041: Update the URL of content_translation document in content_transation hook_help

plach’s picture

Woot! Amazing work guys!

Gábor Hojtsy’s picture

Title: Change notice: Remove legacy translation module in favor of content translation » Remove legacy translation module in favor of content translation
Priority: Major » Critical
Status: Active » Fixed
Issue tags: -Needs change record

Wrote specific change notice for this at https://drupal.org/node/2103189
Updated existing change notice at https://drupal.org/node/2028009

Reopened #1952044: Migration path for legacy content translation module data for renewed core consideration.

Status: Fixed » Closed (fixed)
Issue tags: -Upgrade path, -beta blocker, -D8 upgrade path, -D8MI, -language-content, -LONDON_2013_AUGUST

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

nope, that issue being wont fix is not about no upgrade path. so just saying it.

klonos’s picture

plach’s picture