Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
139.55 KB

Initial patch...

Status: Needs review » Needs work

The last submitted patch, 2024867-rename-translation_entity-1.patch, failed testing.

vijaycs85’s picture

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
146.24 KB

Renaming files.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content
plach’s picture

Thanks for working on this but I think we should avoid the entity_translation namespace, since it would prevent us from being able to provide a proper upgrade path from D7 ET to D8 in contrib. What about content_translation instead? It would be consistent with config_translation.

vijaycs85’s picture

yep, content_translation makes more sense...

vijaycs85’s picture

Title: Rename translation_entity to entity_translation » Rename translation_entity to content_translation

Status: Needs review » Needs work
Issue tags: -D8MI, -language-content

The last submitted patch, 2024867-rename-translation_entity-7.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +language-content
vijaycs85’s picture

Updating labels and seems missed to remove few files...

vijaycs85’s picture

BTW, tests are green locally!

rename-content-translation-test.png

plach’s picture

Status: Needs review » Needs work
Issue tags: +sprint

Awesome! Just some minor stuff and we should be good to go :)

+++ b/core/modules/content_translation/content_translation.admin.inc
diff --git a/core/modules/translation_entity/translation_entity.admin.js b/core/modules/content_translation/content_translation.admin.js
rename from core/modules/translation_entity/translation_entity.admin.js
rename to core/modules/content_translation/content_translation.admin.js

We need to update the behavior namespace here.

+++ b/core/modules/content_translation/content_translation.install
@@ -2,7 +2,7 @@
+ * Installation functions for content translation module.

I'd say it was missing an article but I'd keep the uppercase letters:

Installation functions for the Content Translation module.
+++ b/core/modules/content_translation/content_translation.module
@@ -14,12 +14,12 @@
+      $output .= '<p>' . t('The Content Translation module allows you to create and manage translations for your Drupal site content. You can specify which elements need to be translated at the content-type level for content items and comments, at the vocabulary level for taxonomy terms, and at the site level for user accounts. Other modules may provide additional elements that can be translated. For more information, see the online handbook entry for <a href="!url">Content Translation</a>.', array('!url' => 'http://drupal.org/documentation/modules/entity_translation')) . '</p>';

The correct url currently is https://drupal.org/documentation/modules/translation_entity. It will change again probably.

+++ b/core/modules/content_translation/content_translation.module
@@ -515,15 +515,15 @@ function translation_entity_controller($entity_type) {
+ * Checks whether an content translation is accessible.

a content translation

plach’s picture

Component: entity system » translation_entity.module

We'll need to rename the component too. Gabor, will you? ;)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
171.11 KB

Thanks for the review @plach. Here is my update:

For #13:

  1. Behaviour change in js - Fixed
  2. Installation comment - Fixed
  3. Link update - Updated and created #2026089: Change the documentation page link in content_translation module for future update
  4. a content translation - Fixed

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-content

The last submitted patch, 2024867-rename-translation_entity-15.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2024867-rename-translation_entity-15.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2024867-rename-translation_entity-15.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-content
vijaycs85’s picture

FYI - Seems locally all these failing test cases are fine :)

plach’s picture

Status: Needs review » Reviewed & tested by the community

QED :)

jibran’s picture

Category: bug » task

This is not a bug at all.

plach’s picture

The main reason for renaming ET is #1985488: Consider renaming translation_entity module, which is actually a bug.
However I agree this is a task :)

Gábor Hojtsy’s picture

Issue tags: +Avoid commit conflicts

If we want to have this land first, then it should have Avoid commit conflicts.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@alexpott points out there are unrelated changes in the patch that are not involved with the rename. Eg. phpdoc type documentation changes. Those should not be coupled in, making it easier to review and get committed.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
16.64 KB
172.58 KB

Removing changes that are not related to this issue.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in then! Thanks for rolling it this way :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

From MenuFormController::actions()

    // We cannot leverage the regular submit handler definition because we have
    // button-specific ones here. Hence we need to explicitly set it for the
    // submit action, otherwise it would be ignored.
    if ($this->moduleHandler->moduleExists('translation_entity')) {
      array_unshift($actions['submit']['#submit'], 'translation_entity_language_configuration_element_submit');
    }

Need to update...

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
980 bytes
173.54 KB

Not sure how did I miss it....

alexpott’s picture

Status: Needs review » Needs work
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
167.61 KB

Re-rolling...

Gábor Hojtsy’s picture

How did this become 5k smaller vs. the prior patch?

Status: Needs review » Needs work

The last submitted patch, 2024867-rename-translation_entity-33.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
173.94 KB

somehow, can't re-roll after that patch.. had to re-do all again. hope this is the closer one, but needs full review again :(

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I spot-checked and it looked good. Should be up for @alexpott again if/when green :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed de3558c and pushed to 8.x. Thanks!

alexpott’s picture

Issue tags: -Avoid commit conflicts

Removing tag

Gábor Hojtsy’s picture

Component: translation_entity.module » ajax system

This was also added to the "original" change notice for the introduction of the module (introduction happened in November 2012, I just wrote the change notice). See https://drupal.org/node/2028009

Gábor Hojtsy’s picture

Component: ajax system » content_translation.module

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Remove from sprint!