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.
Comment | File | Size | Author |
---|---|---|---|
#60 | et-menu-1444866-60.patch | 747 bytes | plach |
#56 | et-menu-1444866-56.patch | 1.72 KB | plach |
#51 | et-menu-separate-module-1444866-51.patch | 28.32 KB | bforchhammer |
#51 | interdiff.txt | 2.47 KB | bforchhammer |
#44 | et-menu-separate-module-1444866-44.patch | 27.62 KB | bforchhammer |
Comments
Comment #1
bforchhammer CreditAttribution: bforchhammer commentedPlease review...
Comment #2
plachFirst 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.
Comment #3
bforchhammer CreditAttribution: bforchhammer commentedHm, 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
andentity_translation_menu_save
) could probably just be dropped. Also, calls to$node->language
may have to be replaced byentity_translation_language
(?) respectively to determine the language of the form.Comment #4
Kristen PolComment #5
Countzero CreditAttribution: Countzero commentedIt 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 ?
Comment #6
plachI am coming back on this in a few days.
Comment #7
plachWorking on a port to the ml_edit_form branch.
Comment #8
plachHere 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 ;)
Comment #10
plachThis has been rolled against the ml_edit_form branch (see http://drupalcode.org/project/entity_translation.git/shortlog/refs/heads...).
Comment #11
bforchhammer CreditAttribution: bforchhammer commentedThe 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...
Comment #12
plach@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.
Comment #13
plachStill waiting for #1495648: Introduce entity language support.
Comment #14
plachFixed problems when creating a translation.
Comment #15
muschpusch CreditAttribution: muschpusch commentedI 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
Comment #16
plach@bforchhammer:
Any idea of what could be going wrong?
Comment #17
bforchhammer CreditAttribution: bforchhammer commentedI'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.
Comment #18
plachI had to fix #15 for a project of mine. Here is a new patch with some additional fix.
Comment #19
plachForgot the interdiff.
Comment #20
muschpusch CreditAttribution: muschpusch commented#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 :)
Comment #21
plachSee #1624830: Plan for Entity Translation 7.x-1.0 release.
Comment #22
plachAnother small fix.
Comment #23
muschpusch CreditAttribution: muschpusch commentedhm... 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]
Comment #24
peximo CreditAttribution: peximo commentedDon't show if menu tab is not present.
Comment #25
plachLet's test this.
Comment #26
plachReuploading since the bot does not seem to like #24.
Comment #27
bforchhammer CreditAttribution: bforchhammer commentedI 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.
Comment #28
plachLet's see failures
Comment #30
bforchhammer CreditAttribution: bforchhammer commentedUah, 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?
Comment #31
plachWell, tests should not fail just because
i18n_menu
is not there: all the menu integration is optional. However enablingi18n
andi18n_menu
in thesetUp
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?
Comment #32
bforchhammer CreditAttribution: bforchhammer commentedI didn't have
i18n
insetUp()
, 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".
Comment #34
bforchhammer CreditAttribution: bforchhammer commentedWell 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?
Comment #35
plachYes, 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.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.Comment #36
bforchhammer CreditAttribution: bforchhammer commentedAttached patch adds menu integration as a sub-module
entity_translation_i18nmenu
. (I triedentity_translation_menu
first, but it caused problems with menu hooks).Let's see what the test-bot says; I'm expecting 4 fails...
Comment #38
bforchhammer CreditAttribution: bforchhammer commented#36: et-menu-separate-module-1444866-36.patch queued for re-testing.
Comment #40
bforchhammer CreditAttribution: bforchhammer commentedHm, the testbot is still not downloading the additional "i18n" dependency... According to #698932-39: "test_dependencies" for dependencies / integration tests this should be working.
Comment #41
bforchhammer CreditAttribution: bforchhammer commentedNew 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...
Comment #43
plachOk, let's fix that remaining exception, I'll review/test the new patch soon.
Comment #44
bforchhammer CreditAttribution: bforchhammer commentedThe previous exception came from adding the "edit original content" permission to the "translator user"...
Comment #45
plachI'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.
Comment #46
bforchhammer CreditAttribution: bforchhammer commentedGlad 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...
Comment #47
plach@bforchammer:
Are you willing to fix that bug or should we open a new issue after committing this one?
Comment #48
bforchhammer CreditAttribution: bforchhammer commentedWilling 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...
Comment #49
plachThanks, I'll try to work on the bug fix so we don't have broken tests in the repository. Meanwhile:
Can we use
hook_module_implements_alter()
instead of this?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"?
Comment #50
plachAnother thing, for consistency with
i18n_menu
we should pick theentity_translation_i18n_menu
prefix.Comment #51
bforchhammer CreditAttribution: bforchhammer commentedDone.
Renamed it to
testMenuLocalizationCustomSourceLanguage
.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).
Done.
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 :-)
Comment #52
plachGreat work!
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)
Comment #53
plachComment #54
bforchhammer CreditAttribution: bforchhammer commentedCommitted and pushed :)
Comment #55
bforchhammer CreditAttribution: bforchhammer commentedFollow-up issues:
Oh, and we can probably also remove the menu-integration branch, right?
Comment #56
plachI 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.
Comment #57
plachWent ahead and committed the patch since tests passed locally. Review welcome, anyway.
Comment #58
bforchhammer CreditAttribution: bforchhammer commentedHm, I thought that tests load modules the same way as a normal site, so not sure why this wasn't caught...
Shouldn't this just be 'menu_link_alter'?
Comment #59
plachRemoved that one. Probably it wasn't needed after all.
Comment #60
plachThe committed patch.
Comment #61.0
(not verified) CreditAttribution: commentedminor cleanup and formatting and adding module names and project URLs
Comment #64
stefan.r CreditAttribution: stefan.r commentedBumping 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