This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.

Problem/Motivation

content_translation.module contains content_translation_menu_alter() - hook_menu_alter no longer exists.

Proposed resolution

Remove content_translation_menu_alter()

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#16 remove-content-translation-menu-alter-hook-2210077-7.patch3.15 KBpp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,690 pass(es). View
#5 remove-content-translation-menu-alter-hook-2210077-5.patch2.75 KBandrei.dincu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,877 pass(es). View

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +langauge-content

Well, there is a whole lot of code in https://api.drupal.org/api/drupal/core%21modules%21content_translation%2..., did that end up being added elsewhere? Just removing this without maintaining the functionality is probably not a good idea.

Gábor Hojtsy’s picture

Looks like #2177041: Remove all implementations of hook_menu removed a few lines of this, but left it intact. I think this needs to be renamed to content_translation_menu_link_defaults_alter()?

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Also needs tests then.

andrei.dincu’s picture

Assigned: Unassigned » andrei.dincu
andrei.dincu’s picture

FileSize
2.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,877 pass(es). View

I have removed the function that implements hook_menu_alter().
Waiting for your feedback.

andrei.dincu’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

As discussed above, the functionality is still needed. See #1-#3.

alexpott’s picture

+++ b/core/modules/content_translation/content_translation.module
@@ -182,55 +182,6 @@ function content_translation_entity_operation_alter(array &$operations, \Drupal\
-          while (!empty($entity_form_item['type']) && $entity_form_item['type'] == MENU_DEFAULT_LOCAL_TASK);

Also once the function is renamed... this line needs changing since MENU_DEFAULT_LOCAL_TASK no longer exists

Berdir’s picture

Status: Needs work » Needs review

Gabor, any idea what is broken? That code has nothing to do with menu links, it's mostly about local tasks from what I see.

Content Translation has a Route Subscriber and there's a Derivate for local tasks, which should be replacing this, based on entity link templates.

The only thing that might still be relevant is the thing that makes sure that translate is shown after edit.

I did a manual check using nodes and custom blocks and all works fine. The only thing I noticed is that the translate operation in the custom block overview does display a "Translate" operation but it's not a link... but the local task is working fine.

So unless we actually know something that is broken, this looks fine to me :)

Gábor Hojtsy’s picture

I don't know of specific things that do not work but I did not check the code. I have not been able to verify that equivalent functionality to this code is present elsewhere and I did not check if it was tested or not.

dawehner’s picture

As far as I understand the code in there, it fulfills two parts:

  • Don't provide a translate tab, if we don't have something like an edit route. I wonder whether we actually need this. It would be kinda helpful to also be able to translate a API only entity
  • Add the 'translate' tab right next to the edit tab. This seems to be a total valid feature. Maybe we indeed need something similar as views does in views_local_tasks_alter at the moment.
pp’s picture

Status: Needs review » Needs work
FileSize
3.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-content-translation-menu-alter-hook-2210077-6.patch. Unable to apply patch. See the log in the details link for more information. View

I am at DrupalDevDays Szeged, and I am working with this issue a little bit. (I try to talk with adrei.dincu, but I can't)

I reroll the patch because it isn't work.

@dawehner #11
If you see my patch, you can found a new thing:

- $items['admin/config/regional/content-language']['title'] = 'Content language and translation';
- $items['admin/config/regional/content-language']['description'] = 'Configure language and translation support for content.';

I talked about this with @Gábor Hojtsy and He said me there was a test which test this functionality (add "... and translation" text to this menu title and description when content translation is turned on). I cant found the test (I searching for the texts)

We needs tests, more tests.

pp’s picture

Status: Needs work » Postponed

I think this issue waiting for this:
https://drupal.org/node/1987882

andypost’s picture

Status: Postponed » Needs review
Issue tags: +Needs manual testing

No need to postpone, all menu was converted so this code is not executed.
The task here is to find what's missing and remove dead code

Status: Needs review » Needs work

The last submitted patch, 12: remove-content-translation-menu-alter-hook-2210077-6.patch, failed testing.

pp’s picture

FileSize
3.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,690 pass(es). View

I reroll the patch

andypost’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks nice!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit e843c11 on 8.x by catch:
    Issue #2210077 by pp, andrei.dincu: Remove...

Status: Fixed » Closed (fixed)

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