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

CommentFileSizeAuthor
#16 remove-content-translation-menu-alter-hook-2210077-7.patch3.15 KBpp
#12 remove-content-translation-menu-alter-hook-2210077-6.patch3.35 KBpp
#5 remove-content-translation-menu-alter-hook-2210077-5.patch2.75 KBandrei.dincu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Needs tests

Also needs tests then.

andrei.dincu’s picture

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

andrei.dincu CreditAttribution: andrei.dincu commented
FileSize
remove-content-translation-menu-alter-hook-2210077-5.patch2.75 KB

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

andrei.dincu’s picture

andrei.dincu CreditAttribution: andrei.dincu commented
Status: Active » Needs review
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ 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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

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

dawehner
Primary language German
CreditAttribution: dawehner commented

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

pp CreditAttribution: pp commented
Status: Needs review » Needs work
FileSize
remove-content-translation-menu-alter-hook-2210077-6.patch3.35 KB

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

pp CreditAttribution: pp commented
Status: Needs work » Postponed

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
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

pp CreditAttribution: pp commented
FileSize
remove-content-translation-menu-alter-hook-2210077-7.patch3.15 KB

I reroll the patch

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This looks nice!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
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.