With #1182058-34: Add Entity Translation (entity_translation) support for menu items using Menu Translation being commited, it is now possible to use the Menu Translation (i18n_menu) module from the Internationalization package for the translation of menu items associated with entity-translation based nodes. What is still missing at the moment, is a user interface for doing so from within the node edit form.

The patch, which I am going to upload shortly, integrates menu translation into the Entity Translation workflow:

  • Both types of menu items are supported:
    • If the menu item is language neutral, a string translation form is added (here is an early screenshot from the i18n issue)
    • If the menu item of the source node has a language, then a "normal menu form" is added (which allows to create a new separate menu item for the translation)
  • It ensures that the menu edit form matches the language of the node being edited (language neutral menu items would otherwise always show up in the site's default language)

I started work on the UI in the issue mentioned above and continued in this sandbox. The et-patch branch contains the required changes for Entity Translation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bforchhammer’s picture

Status: Active » Needs review
FileSize
13.94 KB

Please review...

plach’s picture

First of all, thanks for your precious efforts. You are doing a great work, but I am afraid it is somehow going in the wrong direction: as I mentioned in another issue, the translation UI is going to be completely overhauled in #1282018: Improve UX of language-aware entity forms. I think the functional side is almost ready to be committed, it just needs testing with other entities than nodes.

If you could adapt your work to the new UI (here is the feature branch http://drupalcode.org/project/entity_translation.git/tree/refs/heads/ml_...) it would be great. I don't think it makes sense to polish and commit this patch, only to totally revisit it shortly aftwerwards.

bforchhammer’s picture

Hm, I guess it depends on what you mean by "shortly aftwerwards." ;-) I'm really excited about the progress on the new UI, but from what I can tell it will still be a while before it is ready to be merged back into the master branch... for anyone using entity translation right now it would be nice to have this included.

From a quick look the changes to adapt it to the new branch shouldn't be too difficult: the patch already localizes the standard node form, so the entity translation specific functions (entity_translation_menu_form and entity_translation_menu_save) could probably just be dropped. Also, calls to $node->language may have to be replaced by entity_translation_language (?) respectively to determine the language of the form.

Kristen Pol’s picture

Title: UI for translation of menu items » UI for translation of menu items for entity-translated nodes
Countzero’s picture

It is unclear at this moment if there is a way to make menu translation work with ET, no matter the UI.

I understand the patch here doesn't make sense given the ongoing work on the general UI, so there is no need to test it and report. Right ?

plach’s picture

I am coming back on this in a few days.

plach’s picture

Component: Code » Base system
Assigned: Unassigned » plach
Status: Needs review » Needs work

Working on a port to the ml_edit_form branch.

plach’s picture

Component: Base system » Menu integration
Assigned: plach » Unassigned
Category: feature » task
Status: Needs work » Needs review
FileSize
16.82 KB

Here is a patch working with the new UI. I tried to stick close to the orginal code whenever possible. The main difference on the UI level, besides the fact that there is always a full menu widget, is a checkbox allowing to choose whether the menu item for the current language should be localized or part of a translation set.

@bforchhammer:

I created the "Menu integration" component for the issue queue. I'd like to create a mantainers team (like the core one) whose components will focus on single sub systems to help me scale myself. If you are interested and willing to adhere to the best practices (rules!) described in http://drupal.org/node/363367, I'll be glad to give you commit access and make you the maintainer of the "Menu integration" subsystem.

Please note the following excerpt from the link above ;)

If you've been granted limited commit access to maintain a certain part of the project only, then you will not commit any changes that affect other parts of the project.

Status: Needs review » Needs work

The last submitted patch, et-menu-1444866-8.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

This has been rolled against the ml_edit_form branch (see http://drupalcode.org/project/entity_translation.git/shortlog/refs/heads...).

bforchhammer’s picture

The checkbox makes sense.

I haven't tested the patch yet, but I remember going through quite a few different combinations of settings when I first wrote it. We should probably add some tests...

@plach: I am interested and willing to adhere to the rules :) As always, I can't promise I'll have a lot of time but I'll try to follow menu integration issues...

plach’s picture

@bforchhammer:

Contgratulations! You just got commit access to the Entity translation repository. I added you as the first component maintainer in the new MAINTAINERS.txt file. Since this will be committed only after the new UI is in, I didn't add the file to the master branch.

PS: Yes, tests would be welcome. Not sure I'll be have the time to write any but I'll try to.

plach’s picture

Status: Needs review » Postponed
plach’s picture

FileSize
16.88 KB

Fixed problems when creating a translation.

muschpusch’s picture

I just tried the patch but it results in an error:

1.) translate a menu item
2.) go to edit the same node again
3.) Fatal error: Call to a member function get_translations() on a non-object in /patched/entity_translation/entity_translation.menu.inc on line 147
4.) I tried to debug and it seems like this part if failing


      if (!empty($default)) {
        $handler = entity_translation_entity_form_get_handler($form, $form_state);
        $langcode = $handler->getFormLanguage();
//$source_menu['i18n_tsid'] == 0 :(
        $translation_set = i18n_menu_translation_load($source_menu['i18n_tsid']);
        $translations = $translation_set->get_translations();
        if (count($translations) > 1 || !isset($translations[$langcode])) {
          $form['menu']['link']['tset']['#disabled'] = TRUE;
        }
      }

plach’s picture

Issue tags: +Needs tests

@bforchhammer:

Any idea of what could be going wrong?

bforchhammer’s picture

I'm not sure; the menu_form function looks a bit different than in my initial patch... This may just be missing a !empty($node->menu['i18n_tsid']) check.

I'll try to have a closer look later this week.

plach’s picture

FileSize
17.05 KB

I had to fix #15 for a project of mine. Here is a new patch with some additional fix.

plach’s picture

Forgot the interdiff.

muschpusch’s picture

#18 works! Thanks plach! I'm not following the development of entity_translation closely but i think this patch in combination with the new branch is awesome. Why is the status of the patch postponed??? It's way too cool to be postponed :)

plach’s picture

plach’s picture

muschpusch’s picture

hm... this isn't working completely well... Assigning different parent items doesn't work and the title get's overwritten randomly :(

[edit]i use: http://drupal.org/project/title [/edit]

peximo’s picture

Don't show if menu tab is not present.

plach’s picture

Status: Postponed » Needs review

Let's test this.

plach’s picture

FileSize
17.16 KB

Reuploading since the bot does not seem to like #24.

bforchhammer’s picture

Status: Needs review » Needs work
FileSize
13.49 KB
7.91 KB

I started work on some basic tests...

Tests currently fail, because when creating a translation and ticking "Menu link enabled only for the ... language", I would expect the menu link for the "source node" to still be there after saving the translation. Currently it's removed/replaced by the menu item for the translated page.

Currently only "new" pages and "new" translations are covered; we may also want to add tests for "editing" existing pages and their translations... especially for the translation-set use case.

plach’s picture

Status: Needs work » Needs review

Let's see failures

Status: Needs review » Needs work

The last submitted patch, et-menu-1444866-27.patch, failed testing.

bforchhammer’s picture

Status: Needs work » Needs review

Uah, that's a few more fails than I expected... The test requires the core patch (D7.15); could that be the reason why it fails so horribly?

Edit: Hm, we need i18n (i18n_menu) for these tests... that's probably what's missing; how can we get the test bot to checkout i18n?

plach’s picture

Well, tests should not fail just because i18n_menu is not there: all the menu integration is optional. However enabling i18n and i18n_menu in the setUp method should make the testbot download the dependency.

Anyway #26 and #27 are largely different in size, maybe that's the reason for the number of failures?

bforchhammer’s picture

I didn't have i18n in setUp(), and I also missed the .menu.inc file in the first patch.

Let's try this again...

Edit: this patch also contains tests for "editing nodes".

Status: Needs review » Needs work

The last submitted patch, et-menu-1444866-32-tests-only.patch, failed testing.

bforchhammer’s picture

Well, tests should not fail just because i18n_menu is not there: all the menu integration is optional. However enabling i18n and i18n_menu in the setUp method should make the testbot download the dependency.

Well these are integration tests, so I'd say that they should only pass if i18n is available; the module itself should of course also work without i18n.

Simply adding 'i18n' to 'setUp' does not seem to work. We may need a sub-module (or hidden test module) with an extra dependency in the .info file... We could actually move the whole patch to a sub-module; this would also make it easier for users to figure out how to enable menu translation. What do you think, plach?

plach’s picture

Well these are integration tests, so I'd say that they should only pass if i18n is available; the module itself should of course also work without i18n.

Yes, my point was that there should be an explicit assertion to make them fail but the code in itself should not fail just because i18n_menu is not available.

Simply adding 'i18n' to 'setUp' does not seem to work. We may need a sub-module (or hidden test module) with an extra dependency in the .info file... We could actually move the whole patch to a sub-module; this would also make it easier for users to figure out how to enable menu translation. What do you think, plach?

On one hand I like the current behavior: just enable the proper translation method on the menu and things just work. OTOH having the ability to disable ET native i18n_menu integration might make sense for particular edge cases so, yes, a submodule would work for me.

bforchhammer’s picture

Status: Needs work » Needs review
FileSize
27.12 KB

Attached patch adds menu integration as a sub-module entity_translation_i18nmenu. (I tried entity_translation_menu first, but it caused problems with menu hooks).

Let's see what the test-bot says; I'm expecting 4 fails...

The last submitted patch, et-menu-separate-module-1444866-36.patch, failed testing.

bforchhammer’s picture

Status: Needs work » Needs review

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

The last submitted patch, et-menu-separate-module-1444866-36.patch, failed testing.

bforchhammer’s picture

Status: Needs work » Needs review

Hm, the testbot is still not downloading the additional "i18n" dependency... According to #698932-39: "test_dependencies" for dependencies / integration tests this should be working.

bforchhammer’s picture

New patch, which makes the test-bot ignore menu tests if the i18n_menu module is not available; according to #1670936: Testbot does not detect/download additional dependencies dependencies are only calculated from named releases, i.e. this has to be committed and a new release has to be packaged before the testbot will be able to test this...

Status: Needs review » Needs work

The last submitted patch, et-menu-separate-module-1444866-41.patch, failed testing.

plach’s picture

Ok, let's fix that remaining exception, I'll review/test the new patch soon.

bforchhammer’s picture

Status: Needs work » Needs review
FileSize
27.62 KB

The previous exception came from adding the "edit original content" permission to the "translator user"...

plach’s picture

I've been testing this for a while now and it seems to perform well. I'll have a code review ASAP to confirm this can be committed.

bforchhammer’s picture

Glad to hear it :)

If you can, please also review the tests! (I'm still new to writing tests for drupal). Also, menu-tests still fail due to the issue mentioned in #27. Not sure whether the test assumption is the correct one...

Tests currently fail, because when creating a translation and ticking "Menu link enabled only for the ... language", I would expect the menu link for the "source node" to still be there after saving the translation. Currently it's removed/replaced by the menu item for the translated page.

plach’s picture

@bforchammer:

Are you willing to fix that bug or should we open a new issue after committing this one?

bforchhammer’s picture

Willing yes, but I have exams at the moment, so not sure when I'll find time.

It's arguably only a non-critical edge-case, so iIf you don't mind having failing tests in the repository, I'd prefer a new issue...

plach’s picture

Status: Needs review » Needs work

Thanks, I'll try to work on the bug fix so we don't have broken tests in the repository. Meanwhile:

+++ b/entity_translation_i18nmenu/entity_translation_i18nmenu.install
@@ -0,0 +1,14 @@
+  // Change weight to run after menu module. Required so that the 'menu' key is
+  // populated when our implementation of hook_node_prepare() gets called.

Can we use hook_module_implements_alter() instead of this?

+++ b/entity_translation_i18nmenu/entity_translation_i18nmenu.test
@@ -0,0 +1,217 @@
+  function testMenuLocalization2() {

Would it be possible to have a more meaningful name? E.g. testMenuLocalizationNonDefault?

Last thing, would you please change the package of the the various (sub)modules to "Multilingual - Entity Translation"?

plach’s picture

Another thing, for consistency with i18n_menu we should pick the entity_translation_i18n_menu prefix.

bforchhammer’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
28.32 KB

Can we use hook_module_implements_alter() instead of this?

Done.

Would it be possible to have a more meaningful name? E.g. testMenuLocalizationNonDefault?

Renamed it to testMenuLocalizationCustomSourceLanguage.

Last thing, would you please change the package of the the various (sub)modules to "Multilingual - Entity Translation"?

Are you sure? That would be a new package; the main module itself is currently in the "Multilingual" package (same for the "upgrade" sub-module).

Another thing, for consistency with i18n_menu we should pick the entity_translation_i18n_menu prefix.

Done.

I'll try to work on the bug fix so we don't have broken tests in the repository

I have found some time to work on it, and attached patch should now pass all tests... (See failing-tests-fix-interdiff.txt for respective changes)

Back to needs review :-)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Great work!

Are you sure? That would be a new package; the main module itself is currently in the "Multilingual" package (same for the "upgrade" sub-module).

Sorry, I meant all the modules in the ET project, main module included. Let's skip this for now.

Feel free to commit #51 :)
(remember the changelog entry)

plach’s picture

Issue tags: -Needs tests
bforchhammer’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed :)

bforchhammer’s picture

plach’s picture

Status: Fixed » Needs review
FileSize
1.72 KB

I had some problems with the hook invocation order on node presave/menu link alter, don't know why tests did not catch them beacuse they totally broke menu translation in that particular installation.

THe atached patch fixes them plus a notice when editing non-node items.

plach’s picture

Status: Needs review » Fixed

Went ahead and committed the patch since tests passed locally. Review welcome, anyway.

bforchhammer’s picture

Status: Fixed » Needs work

Hm, I thought that tests load modules the same way as a normal site, so not sure why this wasn't caught...

+++ b/entity_translation_i18n_menu/entity_translation_i18n_menu.module
@@ -27,15 +27,19 @@ function entity_translation_i18n_menu_node_prepare($node) {
+    case 'i18n_menu_menu_link_alter':

Shouldn't this just be 'menu_link_alter'?

plach’s picture

Status: Needs work » Fixed

Removed that one. Probably it wasn't needed after all.

plach’s picture

FileSize
747 bytes

The committed patch.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

minor cleanup and formatting and adding module names and project URLs

  • Commit 23182e8 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions by bforchhammer:
    Issue #1444866 by bforchhammer, plach, peximo: Added UI for translation...
  • Commit 7c63f33 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions by plach:
    Issue #1444866 by plach: Fixed the invocation order on presave and menu...
  • Commit f396c23 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions by plach:
    Issue #1444866 by plach: Removed bogus hook menu link alteration (follow...

  • Commit 23182e8 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench by bforchhammer:
    Issue #1444866 by bforchhammer, plach, peximo: Added UI for translation...
  • Commit 7c63f33 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench by plach:
    Issue #1444866 by plach: Fixed the invocation order on presave and menu...
  • Commit f396c23 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench by plach:
    Issue #1444866 by plach: Removed bogus hook menu link alteration (follow...
stefan.r’s picture

Issue summary: View changes

Bumping this with a link to #2858495: Allow basic translation of menu items on entity forms in case anyone needs this, we have this running on production for a few months now