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.

Files: 
CommentFileSizeAuthor
#70 1952062-translation-module-70.interdiff.txt1.98 KBkfritsche
#70 1952062-translation-module-70.patch75.19 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 58,746 pass(es).
[ View ]
#67 interdiff.txt1 KBpenyaskito
#67 1952062-translation-module-67.patch75.87 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,566 pass(es).
[ View ]
#63 interdiff-58-62.txt5.41 KBvasi1186
#62 1952062-translation-module-62.patch74.86 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 58,740 pass(es).
[ View ]
#60 interdiff.txt948 bytespfrenssen
#58 1952062-translation-module-58.patch69.45 KBwillyk
PASSED: [[SimpleTest]]: [MySQL] 58,371 pass(es).
[ View ]
#56 1952062-translation-module-56.patch69.45 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,184 pass(es).
[ View ]
#56 interdiff.txt1.92 KBpfrenssen
#54 1952062-translation-module-54.patch69.45 KBwillyk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1952062-translation-module-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#51 1952062-translation-module-51.patch69.45 KBwillyk
PASSED: [[SimpleTest]]: [MySQL] 58,267 pass(es).
[ View ]
#48 1952062-translation-module-48.patch67.39 KBwillyk
FAILED: [[SimpleTest]]: [MySQL] 57,812 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#47 1952062-translation-module-47.patch67.39 KBwillyk
FAILED: [[SimpleTest]]: [MySQL] 57,926 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#43 1952062-translation-module-43.patch66.89 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,686 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#40 1952062-translation-module-40.patch61.95 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#40 1952062-diff-38-40.txt765 bytesvijaycs85
#38 1952062-translation-module-38.patch61.95 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,594 pass(es), 19 fail(s), and 4 exception(s).
[ View ]
#36 1952062-translation-module-36.patch63.52 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1952062-translation-module-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 1952062-diff-32-36.txt6.18 KBvijaycs85
#32 1952062-translation-module-32.patch58.5 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,808 pass(es), 12 fail(s), and 1 exception(s).
[ View ]
#32 1952062-diff-20-32.txt3.04 KBvijaycs85
#27 drupal8.translation-module.1952062-27.patch320.12 KBmanningpete
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.translation-module.1952062-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 drupal8.translation-module.1952062-20.patch54.87 KBCMS Dude
FAILED: [[SimpleTest]]: [MySQL] 57,166 pass(es), 15 fail(s), and 1 exception(s).
[ View ]

Comments

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.

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,
+      ),

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.

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.

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.

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.

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.

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.

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

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.

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.

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.

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

@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.

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.

Assigned:Unassigned» CMS Dude

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

StatusFileSize
new54.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,166 pass(es), 15 fail(s), and 1 exception(s).
[ View ]

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.

Status:Active» Needs review

Status:Needs review» Needs work

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

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

...this usually helps ;)

Status:Needs review» Needs work

...xpost :/

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.

Issue tags:+Needs reroll

Issue tags:-Needs reroll
StatusFileSize
new320.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.translation-module.1952062-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

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.

Issue summary:View changes

Add new dependency.

Issue tags:+beta blocker

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
new58.5 KB
FAILED: [[SimpleTest]]: [MySQL] 57,808 pass(es), 12 fail(s), and 1 exception(s).
[ View ]

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

Issue tags:+LONDON_2013_AUGUST

Adding tag...

Status:Needs review» Needs work

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

Still fails, but less than above :)

Status:Needs work» Needs review
StatusFileSize
new6.18 KB
new63.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1952062-translation-module-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new61.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,594 pass(es), 19 fail(s), and 4 exception(s).
[ View ]

Re-rolling with current code base.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new765 bytes
new61.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

Fixing drupalPost() issue.

Status:Needs review» Needs work

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

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new66.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,686 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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?

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.

StatusFileSize
new67.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,926 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new67.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,812 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

@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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

StatusFileSize
new69.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,267 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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 '='.

StatusFileSize
new69.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1952062-translation-module-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new69.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,184 pass(es).
[ View ]

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.

Status:Needs review» Needs work
StatusFileSize
new69.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,371 pass(es).
[ View ]

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

Status:Needs work» Needs review

I bet the bot will test!

Status:Needs work» Needs review
StatusFileSize
new948 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.

#56: 1952062-translation-module-56.patch queued for re-testing.

StatusFileSize
new74.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,740 pass(es).
[ View ]

Removed some more code.

and the interdiff

Woot, great diffstat!

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

:)

StatusFileSize
new75.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,566 pass(es).
[ View ]
new1 KB

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?

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

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

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new75.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,746 pass(es).
[ View ]
new1.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)

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

since we are not doing upgrade path

Title:Remove legacy translation module in favor of content translationChange 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.

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

Woot! Amazing work guys!

Title:Change notice: Remove legacy translation module in favor of content translationRemove 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.

Issue summary:View changes

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