Problem
Drupal understands the language of configuration in Drupal 8, and translation to other languages (as well as possible other contributed functionality) depends on the language of configuration. Configuration entities like menus, vocabularies, views, etc. have native language support. Views got a language selector in #1935022: Add a language selector on views and vocabularies got it even earlier. Menus are in the same boat, and could use a language selector on creation defaulted to the site default language.
Proposal
- Add a #type => language_select element on menu config entities.
- Add tests ensuring changing the language worked, saving back to the config entity storage.
UI changes
See before screenshots in #20
Related Issues
#2014617: Unsaved menu links have the wrong bundle (always 'tools') (pre-existing issue, noticed in #55)
Dependencies
This issue is postponed on:
Comment | File | Size | Author |
---|---|---|---|
#145 | interdiff-143-145.txt | 652 bytes | helenkim |
#145 | 1945226-145-menu-language.patch | 26.74 KB | helenkim |
#143 | 1945226-143-menu-language.patch | 26.67 KB | YesCT |
#143 | interdiff-136-143.txt | 3.08 KB | YesCT |
#136 | 1945226-136-menu-language.patch | 25.66 KB | tstoeckler |
Comments
Comment #1
Gábor HojtsyAlso putting on sprint.
Comment #2
YesCT CreditAttribution: YesCT commentedshould be good for someone new.
ready for an initial patch. get inspiration for how from the views issue. :)
Comment #3
babruix CreditAttribution: babruix commentedJust not sure if we need selector "MENU ITEMS LANGUAGE" - but I`ve added it, as vocabulary edit page has similar for terms.
Comment #4
babruix CreditAttribution: babruix commentedComment #5
Gábor HojtsyWoot, thanks for this great patch!
Does this actually make language inheritance work? The menu vs. menu link relation as in menu is a config entity vs. menu links are content entities are very similar to vocabularies vs. terms, which have the same relation. So taking inspiration from there makes a ton of sense. Also this would let people set certain menus to one specific language and then not needing to set each item separately. Sounds like a good approach. Does the inheritance feature actually work?
Comment #6
Gábor HojtsyDon't know how sprint was removed, it should be on.
Comment #7
YesCT CreditAttribution: YesCT commentedinitial patch added in #3 (so removing tag for that task)
needs manual testing now for general testing and inheritance. (contributor task doc for how to do manual testing: http://drupal.org/node/1489010)
shall we write a test to see if inheritance works? (contributor task doc for writing tests http://drupal.org/node/1468170)
Comment #8
Gábor HojtsyIndeed, both manual and automated testing is needed.
Comment #9
Gábor HojtsyAnybody up for doing either? :)
Comment #10
no_angel CreditAttribution: no_angel commentedI will try to do the manual testing on the patch.
Comment #11
no_angel CreditAttribution: no_angel commentedFailed to reproduce the same output results #4 using steps: http://pastebin.com/KCVaT7ZE
If someone could let me know what I did wrong, happy to try again.
attached are before and after screenshots (which r the same <--not good outcome)
Comment #12
no_angel CreditAttribution: no_angel commentedunassigned.
maybe the patch needs to be applied to the menu.module???
Comment #13
Gábor HojtsyThe language selector displays for me, so I think you have not actually checked the patched site?
Comment #14
no_angel CreditAttribution: no_angel commentedthanks @Gábor Hojtsy for confirming the patch. I'm new and still trying to sort my way thru the git stuff.
Comment #15
YesCT CreditAttribution: YesCT commentedcomment seems a little awkward. Lets say why the next bit is *in* the if module exists instead.
Wait, do we want to show the select when the language module is disabled? Or just make sure we are saving the site default language as the langcode for the menu?
in #1935022-42: Add a language selector on views the language select was added to views.
I checked it because the comment block here seemed.. not what we usually comment. So I was looking for what "usual" might be in the case of a language select.
I think the link to the change notice should be taken out and can be explained here in the issue, and someone can use git blame to find this issue if they want to know what issue (and change notice) is responsible for the element.
We usually link to d.o in the code ... if we have a @todo that is for a certain issue, but not otherwise.
The comments about why having the language element in the if that follows... maybe can be reworded.
deleting this blank line seems like an unrelated change.
======
This is probably both needs review and needs work. Changing to needs work to get the tests added.
Comment #16
YesCT CreditAttribution: YesCT commentedoh that
--
line in the patch is not removing a line, it's something about the git format of the patch.
@no_angel lets talk in irc about git to see if the format causes any concern. I think I got some message about whitespace when I applied the patch.
Comment #17
Gábor HojtsyThe language select element degrades to a #type value element if no language module, so no reason to put into an if. The variance in type/rendering is already encoded within the element as needed in a centralized way.
Comment #18
YesCT CreditAttribution: YesCT commentedhere is what I get when I apply this:
Anyway, I'll test this now.
Comment #19
YesCT CreditAttribution: YesCT commentedsteps:
git stash
git checkout 8.x
git pull --rebase
drush am 1945226 (or git checkout -b somebranch; git apply --index thepatch.patch)
git status
git add core*
git commit -m "whatever comment number the patch is from"
drush -y si --account-pass=admin --db-url="mysql://root:root@localhost/d8-git" --site-name=8.x
Added a menu and the langcode was saved in the active config as the site default: en
added a link to the menu.
checked the menu_links table in the database, and the item was saved with langcode: und
I think that should have been saved with the site default langcode.
Comment #20
YesCT CreditAttribution: YesCT commentedwith patch, but without language module enabled here are screenshots of forms:
0a admin menu
0b admin menu link item edit
0c menu link item add
0d custom menu add
0e custom menu edit
0f custom menu link item add
with patch, with language module enabled (but no other languages added, and no translation modules enabled), here are screenshots of forms:
1a admin menu
1ai admin menu scrolled all the way down
1aii admin menu select values
1d custom menu edit
1f custom menu link item add
Problem 1
the order of the fields in the menu form need adjusting. I think the language and default for menu items should be together and before the long list of menu items? or together and under it down by the save.
Problem 2
I'm not sure if the config for what a menu's link item default langcode gets saved in the menu yml config file... we should be able to compare to node articles and see what is done there.
I resaved the custom menu (which changing any fields)
I dont see a change to the yml... no info saved about what the default should be for menu link items.
Problem 3
After resave, new menu items still defaulting to not specified (und), and... hey the checkbox was unchecked for showing the language selector on menu items. so that should not have been showing either.
I'm going to take a stab at the code to see if I can fix any of that.
Comment #21
YesCT CreditAttribution: YesCT commentedI'm working on the patch for this.
Comment #22
YesCT CreditAttribution: YesCT commented@tim.plunkett pointed out to me that the out of order language select related to #1947814-44: New config entities often save as langcode: und incorrectly
I had to move the call to parent::form to the end.
Because this patch checked first if langcode was added, and if not it added it.
Then in the menu form function, it tried to add it in a different place, but it was already added.
--
I wrapped some comments to 80 chars.
--
Needs review so the bot can go on it. But it's really needs work.
Next: fix the default of the menu items language.
Comment #23
YesCT CreditAttribution: YesCT commentedI also have seen that the comment I thought was odd
Comes right from the VocabularyFormController.php
Comment #24
YesCT CreditAttribution: YesCT commentedis being added to
sites/default/files/config_zNb0Um-o1E8or0sWm5zUI-ZZuIxvfhw5KnDpLc9cAC8/active/language.settings.yml
But not to the settings page:
Comment #25
YesCT CreditAttribution: YesCT commented$ grep -R "Custom language settings" *
core/modules/language/language.admin.inc: '#title' => t('Custom language settings'),
and in there:
supported is an arg:
function language_content_settings_form(array $form, array $form_state, array $supported)
http://api.drupal.org/api/drupal/core%21modules%21language%21language.ad...
leads me to:
1 string reference to 'language_content_settings_form'
language_content_settings_page in core/modules/language/language.admin.inc
http://api.drupal.org/api/drupal/core%21modules%21language%21language.mo...
so menus should be supported if they are fieldable or translatable
we might want to add a section to http://drupal.org/node/1749954 explaining what kind of things can use that form[] pattern and how to check if they are supported. (if that is the cause of each menu item being und)
tim helped me again in irc. He's working on a key to be able to locate the entity definitions. but pointed me to
I cleared the cache, but didn't magically see it in the content language settings. I'll look at this some more in a bit.
Comment #26
YesCT CreditAttribution: YesCT commentedoh, if (!empty($info['fieldable']) && !empty($info['translatable'])) {
means it has to be fieldable and translatable, not just translatable.
Also, if a parallel to vocabularies and taxonomy terms, then I'd want to do it for menu items, not menus.
So I was curious to see if it would work.
*did* add menu items to the language settings config page. But that is not really the goal here, and it didn't help the menu item create and edit page pay attention to the settings. I'm done for now.
Next steps:
make the inheritance of the langcode work for the menu link items.
Comment #27
YesCT CreditAttribution: YesCT commentedTo clarify,
menu link items are always showing the language select, and always as "not specified" (und).
no matter what is set on the menu.
Comment #28
YesCT CreditAttribution: YesCT commentedthis adds menu links to the language settings page.
but since they are not fieldable, it changes the language_entity_supported function
interestingly, menu link items are the only other thing to show up listed there.
Comment #29
YesCT CreditAttribution: YesCT commentedin core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.php
And in core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
terms have bundle_keys =
menu link items have bundles =
---
@andypost points out #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus() might be relevant.
Comment #30
amateescu CreditAttribution: amateescu commentedWe have #1937384: Remove 'bundles' key from the MenuLink class annotation for removing 'bundles' from the definition of MenuLink, but no one looked at it so far..
Comment #31
Gábor Hojtsy@amateescu: well, for this issue, if we want to have per-menu language inheritance for language items, our current standard solution is to rely on bundles for that, and bundles provide the key to store the language setup per menu with. So long as there are no bundles for menus, we'll either need to get away with a global setting for *all menus* on the site, which does not satisfy many multilingual sites or make up yet another way to work around bundle-less entities to support some other variance for language settings, which would bloat that part of core. So what we are looking for is not so much to remove dead code but to make menus bundles :)
Comment #32
Gábor HojtsyYeah, so menu items are not from bundles of different types per menu currently, so this issue can only make the settings available on a global menu level like for user entities. We should IMHO introduce bundles for menus per menu in a different issue. I'd love to not postpone this issue on that, but per-menu language settings would only be possible then (or if we make up a non-bundle variance system for language settings and inheritance).
Comment #33
Gábor HojtsyOpened #1966298: Introduce menu link bundles per menus.
Comment #34
Gábor HojtsyLooking at the patch, since it assumes bundles exist for menus all around, it looks like best in fact to postpone on #1966298: Introduce menu link bundles per menus.
Comment #35
Gábor HojtsyOk, it is clear #1966298: Introduce menu link bundles per menus is going nowhere anytime soon, so let's reopen this and fix at least what we can on the short term.
1. There is no per-menu language settings yet because there is no agreement that menus should be bundles. So we cannot have the language inheritance setting on the menus.
2. We don't need to / should not set menu items to translatable entities for them to show up in the language configuration screen, the screen should support configuration for items which are not translatable.
This remaining condition should go away as well AFAIS. Even non-translatable entities would need their language configuration and show/hide checkbox, that is not dependent on translatability.
This may not be the right place to modify the condition for the UI only, if this helper has wider reaching consequences (eg. I assume the contact entities will show up again if we remove this condition, we need to figure out a sane condition to make them go away :)
Keep the $form['langcode'] but not the default menu items language because that would incorrectly make this a per-menu setting which is debated in #1966298: Introduce menu link bundles per menus. Yeah, the resulting settings will make it look like you set a language on the menu but you set menu item language defaults totally elsewhere. Yeah, that is the result of the debates there for now.
Also keep the parent::form() call at the end, that is a good change either way.
Don't need any of this for now :/
Comment #36
Gábor HojtsyRepostponing on #1966298-26: Introduce menu link bundles per menus.
Comment #37
andypostnot sure it's right place for that, I add this in #1966298-31: Introduce menu link bundles per menus
Comment #38
Gábor HojtsyI think we'll need to have some more critical look at how the configuration page is assembled, this configuration page allows to configure language setup even if you don't have content translation enabled at all. Menus should fully be able to participate in that even though they are not fieldable or translatable. I don't think we should fake menus to be translatable, the screen should support settings language configuration for bundles/entities that are not translatable, so the screen should be fixed not the menu code hacked.
Comment #39
YesCT CreditAttribution: YesCT commentedTo do #38 #1977784: Content language settings configuration page needs to determine what entities and bundles to include
Comment #39.0
YesCT CreditAttribution: YesCT commentedadded separate issue to address how the content language settings page is built
Comment #40
Gábor HojtsyI think #1977784: Content language settings configuration page needs to determine what entities and bundles to include would actually be a pre-requisite to this one. I thought we could solve it here, but its true it could be better on its own, so this would automatically work. The menu settings would only show up on that page once that is fixed, so I think this is postponed on that too then.
Comment #40.0
Gábor Hojtsyadded link to comment where before screenshots are.
Comment #41
Gábor HojtsyFYI this is currently postponed on #1966298: Introduce menu link bundles per menus and #1977784: Content language settings configuration page needs to determine what entities and bundles to include. Updated the issue summary as well.
Comment #42
YesCT CreditAttribution: YesCT commentedre-roll for #1913618: Convert EntityFormControllerInterface to extend FormInterface
and (when it lands) #1966298: Introduce menu link bundles per menus Apply that one first if it's not in yet.
The menu item create pages are not noticing the setting that tells them what the default language should be or wether or not to show the language selector.
Comment #43
YesCT CreditAttribution: YesCT commentedsince #1977784: Content language settings configuration page needs to determine what entities and bundles to include is in, the content language settings page looks like:
Comment #44
YesCT CreditAttribution: YesCT commentedThis still has the problem in #27.
I thought maybe something with actions parent. But I dont see how to move that down to the return. So must not be that.
Note to self: the default language for menu items (or vocabulary terms, etc) are stored in the language module settings:
sites/default/files/config_[....]/active/language.settings.yml
Here is a sample from my test site:
Comment #45
YesCT CreditAttribution: YesCT commentedmenu prefix looks like it's being added after the language config save.
http://api.drupal.org/api/drupal/core%21modules%21language%21language.mo...
in core/modules/menu/menu.admin.inc
Comment #46
andypostThe same happens in shortcut module with different prefix
Comment #47
Gábor Hojtsy#1966298: Introduce menu link bundles per menus landed! Reopening, retesting.
Comment #49
YesCT CreditAttribution: YesCT commented(not the test bots are not working, but I could use some advice)
What I did was start with just 8.x, and then add back things to get them working since #1966298: Introduce menu link bundles per menus went in.
I guessed that calling it menu links was the thing to do instead of menu items.
config files are saving ok
for example, I made a new menu and it's config file looks like:
and in the language settings config, the defaults for menu items/links is being saved, read ok into both the menu like admin/structure/menu/manage/menu-my-menu and the content language settings page (since menu links are content) admin/config/regional/content-language
things to think about in the future are: tests still needed. do we need to update default config?
check the interdiff to see what I didn't put in this patch, for example:
Immediate trouble I need some help on is:
the actual menu link add page ignores the show language selector setting and the language selector is always showing (there was this trouble before in this issue), also, the default language shown there is always "not specified".
Comment #50
YesCT CreditAttribution: YesCT commentedah, here is the issue that changed the constant #1620010: Move LANGUAGE constants to the Language class (already taken care of in the previous patch)
----
Q1.
I'm not clear on what this submit handler is for, or what effect it has.
----
and, module_exists is depreciated
https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
new patch uses
drupal_container()->get('module_handler')->moduleExists($hook)
Drupal::moduleHandler()->moduleExists('language')
so.. #2007684: Update deprecated doc for module_exists() and call method inside
Comment #51
Gábor Hojtsy@YesCT: the role of the submit handler is so the language settings are properly saved. It is not the responsibility of the containing form to save the translation related settings, so we make it a responsibility of the entity translation module for content entities.
Comment #52
Kristen PolThanks, Cathy!
I'm late in this game but here's a code review.
Still some module_exists references.
Still some module_exists references.
Still some module_exists references.
Nothing else caught my eye. I did not test it yet.
Comment #54
YesCT CreditAttribution: YesCT commentedjust addressing #52 for the moment.
Comment #55
YesCT CreditAttribution: YesCT commentedbundle name is always tools
and I can control if the language select is shown or hidden in the main menu, by setting the default show/hide in the tools menu.
!
Comment #56
YesCT CreditAttribution: YesCT commentedfirst interdiff is intermediate getting the default langcode to work [edit: oops, except I reversed it.]
second interdiff is replacing depreciate module_invoke with Drupal::moduleHandler()->invoke
Comment #57
YesCT CreditAttribution: YesCT commentedah, so this is what the submit handler is for.
so that not every menu item thinks it's in the tools bundle.
I didn't think we needed to know the bundle anywhere else, so instead of getting it at the beginning like is done for terms, I just did it when reading the default language content settings config.
Problem is, when editing a menu link item, the language it was saved in is overridden with the default from the config. I'll work on that next.
Comment #59
YesCT CreditAttribution: YesCT commentedwell, I was testing by editing a menu link... (thought I was reloading a add new page) which confused me. so the submit does not help with a new menu link item knowing what bundle it is in (to prevent it from thinking it's in tools.)
it does however make sure that a new menu (not menu item) gets it's machine name saved correctly in the language settings config. without the submit, new menus are saved with an empty machine name ''.
--
This one defaults to the correct language if editing a menu item (what it was already saved with),
or for a new menu link item, uses the content language settings default for menu links for a menu.
Fixes module_exists being depreciated.
Comment #60
YesCT CreditAttribution: YesCT commentedoops. added it twice.
Comment #61
YesCT CreditAttribution: YesCT commentedthis is probably ready for someone else to review.
known problem is that the first time creating a new menu, the machine name is not saved in the language settings with the menu_ appended to it, so when editing the menu, or adding a menu link item, it does not get the defaults the menu was created with.
[edit: see also #46]
Comment #62
YesCT CreditAttribution: YesCT commentedI checked git blame on the section adding menu_
http://drupalcode.org/project/drupal.git/blame/HEAD:/core/modules/menu/l...
shows it was part of #1814916-85: Convert menus into entities
which was new code... so I though of just taking it out.
But then, I showed this to @tstoeckler and they had the good idea to use a #field_prefix instead of the isNew.
This is much better because the old way, the UI hints what the machine name will be, but it lies, because after saving the machine name changes.
This way, its the same after save as it is while typing it.
Next... tests.
Comment #63
andypostJust use \Drupal in classes
$menu = $this->entity
Maybe better to leave this default?
Comment #65
YesCT CreditAttribution: YesCT commentedfix addresses the things in #63, except for
+ * translatable = TRUE,
that is staying. It doesn't actually make it translatable, but makes it multilingual and is the core of the issue here. We have another issue where we discuss the naming of the annotation setting...
then the other interdiff is adding a test. It doesn't do anything yet, it's just a sketch of what to test.
Comment #67
YesCT CreditAttribution: YesCT commentedthe test is showing the menu add form ok... only without my other changes to actually add the language select.
with the full patch to add the language select, the menu add verbose message in the tests shows a blank page.
Posting this to get some help. (there are some dpm's so need devel)
Comment #68
YesCT CreditAttribution: YesCT commentedah, it's the dpm on that page the test is trying to go to. :) taking out the dpm makes the verbose message in the test show the menu add form ok.
so I'm back on track and moving forward.
Comment #69
tstoecklerI didn't read the entire issue, so I might have missed something that was already discussed.
Was this taken out intenionally? If so, why?
Did you try out what happens if you remove the check for language.module? If the 'language_configuration' element were cool, it would degrade to a #value element just like 'language_select'.
If it currently doesn't that's definitely a follow-up but would be good to investigate anyhow.
Hmm... it would be cool if language.module could figure out on its own when to do that, but I don't know if that is feasible in any way.
I've never seen $form_state['language'], it looks fancy! :-)
I think all this logic should be in MenuLink::create() or somewhere. It doesn't seem specific to the form, right?
Comment #70
tstoecklerAhh, it's only moved to the end... Sorry, stupid me.
Comment #71.0
(not verified) CreditAttribution: commentedUpdate dependencies.
Comment #72
YesCT CreditAttribution: YesCT commentedNote, @tstoeckler opened #2014617: Unsaved menu links have the wrong bundle (always 'tools') and it might be blocking this issue.
Here is some improvements to the test.
Still more to go.
Note, can tell what to set in the $edit array by using an inspector like firebug and checking the form element name.
Comment #74
YesCT CreditAttribution: YesCT commentedI think there are too many drupalGet
Also, I dont know how to check the language on the menu item was actually saved correctly.
I think that is the next thing to go.
Comment #75
YesCT CreditAttribution: YesCT commentedforgot patch.
Comment #76
YesCT CreditAttribution: YesCT commentedI copied a helper function from MenuTest
works to check the langcode on a menu item.
I tried this by running the tests with the wrong langcode and it failed.
this is also a little clean up.
Next, look at the exceptions.
Comment #78
YesCT CreditAttribution: YesCT commentedOpened
#2014955: Deleted bundles do not have their language configuration deleted
This last bit I did here is mostly comment cleaning up.
taking off the needs tests.. since we have them (they might need improving though)
I'm going to be away from this for a bit. So if someone else wants to do anything, please go ahead. :)
Still to do: the exceptions.
Comment #80
kfritscheIf another language than english is chosen in the installation process, the menu language still is en.
Steps to reproduce:
Install D8 in another language with default profile (example german [de])
Check sites/default/files/config_*/active/menu.menu.tools.yml
Is:
Should be:
After just saving the menu again, the expected output will be produced. Everything else seems to work for me.
Will check now for the exceptions and mentioned error. Assigning to me.
Comment #81
kfritscheThis should fix the "Undefined variable"-Exception and therefor most of all the exceptions.
Problem was that $menu was used in menu_links, which we doesn't have there. I used a menu_load there now, but maybe it would be faster to just do a config load to get the langcode of the menu?
What also makes me wonder and probably produces the rest of the exception is:
This was replaced with:
+ '#field_prefix' => 'menu-',
I'm not sure, but does field_prefix really add it to the id and not just for theming the input field?
Because currently my custom menus ids are just "test" and not "menu-test".
The problem described in #80 seems to be a wider problem, as this is also happening for blocks, ... because this is whats in the default config is defined. Moving this to #2015301: Wrong langcode after D8 installation with other Language.
Comment #82
kfritscheTests passes now.
To clarify I removed
$menu_name = 'menu-' . $menu_name; // Drupal prepends the name with 'menu-'.
from the MenuTest, to correspond the change, which described above.Still not sure, if we maybe add this again.
Rest seems fine to me.
Removing me from assigned, further opinions welcome.
Comment #83
YesCT CreditAttribution: YesCT commentedTake out that commented code and add a link to the issue todo that.
https://drupal.org/node/2017127
#2017127: Add test for deleting menu bundles to verify their language configuration is deleted from language.settings.yml
This should do
This little part is novice.
Comment #84
IshaDakota CreditAttribution: IshaDakota commentedAdds link to the todo issue as described in #83
Comment #85
Gábor HojtsyLooks pretty good :) Some notes:
Is this copied from somewhere? I'm not sure we should repeat this doc all around :) At least I'd shorten it.
Can users change the bundle name? Or is this only for the automated bundle name change that is in the creation flow? (Is that even happening now that the form contains the prefix instead of it being added later).
How/when will the right one get created? Looking at the order of submit functions, the new one may be created before this? Also, does this affect entity translation settings too?
Create menu and menu item in non-English language.
// Check menu language and item language configuration.
// Test menu link language.
Also, note whitespace issue in the assignment.
Remove this section since this will be in a different issue / separate bug?
I think the other bug should be fixed first likely, putting todo code like this in does not seem like a good idea at this point :(
Is there comparable code in the taxonomy vocabulary/terms, etc. area? In other words is this code made up for menus or adapted from that pattern?
Is taxonomy, nodes, etc. using similar code? In other words is this code made up for menus or adapted from that pattern?
Comment #86
amateescu CreditAttribution: amateescu commentedTotally agreed.
About this part:
menu is an optional module, while menu_link is not. That code cannot be placed in the menu link form controller without at least a
module_exists()
around it..Comment #87
Gábor HojtsyOk, #2014617: Unsaved menu links have the wrong bundle (always 'tools') landed, the code can now use the bundle and the comment can be removed now :)
Comment #88
YesCT CreditAttribution: YesCT commentedI'll work on the new patch.
Comment #89
YesCT CreditAttribution: YesCT commented#85 @Gábor Hojtsy
Yeah, See #15 and #23.
I'll change it.
Comment #90
YesCT CreditAttribution: YesCT commented#85 @Gábor Hojtsy
How/when will the right one get created? Looking at the order of submit functions, the new one may be created before this? Also, does this affect entity translation settings too?
Ah, good point.
Built in default menu:
while creating a new menu, can edit machine name
but when editing that custom menu, no edit on machine name.
I thought there was an edit there, oh wait, that was for tags. yep.
Menus are different than vocabularies. Vocabularies can have their machine names changed on edit (after add/creation). Menu's cannot.
so:
---------
I dont know. I have not tried translating menu items.
Comment #91
YesCT CreditAttribution: YesCT commentedhere is the only change due to #2014617: Unsaved menu links have the wrong bundle (always 'tools')
Comment #92
YesCT CreditAttribution: YesCT commented#86 @amateescu
Hm.
Without menu (which handles customizing menus in the UI), we dont ever get here to add a menu link item.
So I could add the check, but it will only be checked when it is enabled.
I think I need some help understanding the implications here.
Could we get here if programmatically creating a menu link (not via the UI), maybe in a contrib module?
If we add the check for menu module installed, what should the default be when menu is not enabled? Maybe site default language?
-------
Also, note that with menu disabled,
shortcuts appears under /admin/config/regional/content-language
(actually shortcuts is always there, even though it is not under /admin/structure/menu)
And editing or adding a shortcut item does not give an option for language.
I think we should think about shortcuts and their interaction with menus and language in a different issue.
Comment #93
amateescu CreditAttribution: amateescu commentedThe menu_link form controller can be used by any other piece of code, even when menu.module is not enabled, for example in the node edit form, so yes, we definitely need to add that check. The fallback would be
\Drupal\Core\Language\Language::LANGCODE_NOT_SPECIFIED
.Comment #94
YesCT CreditAttribution: YesCT commentedhere is a patch with the changes from my comments above.
Left to do:
1.
figure out #92
2.
And discuss the new code @Gábor Hojtsy pointed out in the last two points of #85
3.
Also, new menus do not show on /admin/config/regional/content-language until another setting on a different menu is changed and saved, or if the cache is cleared. new content types do show when reloading the language settings, without a separate save or cache clear.
Maybe add a cache clear to the language settings page? But that seems kind of extreme.
Comment #95
Gábor HojtsyWhat does node types do? Do they have a cache similar to menus? Is this a cache only for the list of menus? We should clear the most specific cache needed only AFAIS and already within the menu module, not in the language extensions.
Comment #96
YesCT CreditAttribution: YesCT commentedthis addresses #92 and #93, puts the call to menu load in an if, if the menu module is installed.
---
I'll look into the caching.
---
#94 2. in TermFormController.php
it just does
'#default_value' => $term->langcode->value,
but for menus, it was always defaulting on a add menu form to be always not specified, see 1f in #20, #42 ... first interdiff in #56 (which is backwards, I had the diff orders wrong) and interdiff in #59
Comment #98
YesCT CreditAttribution: YesCT commentedI was looking at #1810386: Create workflow to setup multilingual for entity types, bundles and fields for ideas
and found
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/transla...
I'm not sure why that does the menu router rebuild.
This is not the best solution, since it's in language, but it does make a new menu show on the language settings admin page.
Oh, I was *just* looking at that custom code for if a menu is new...
I'll try it there. Wait, that was for new menu links.
This might be better, it's menu specific.
But I wonder about menus created other ways and not via the form.
Comment #99
YesCT CreditAttribution: YesCT commented#96: drupal-1945226-Add-language-selector-on-menus-96.patch queued for re-testing.
Comment #101
YesCT CreditAttribution: YesCT commentedthat didn't work
neither did this
so, I went with
entity_info_cache_clear();
in the language summit in the menu form controller.
Comment #102
andypostLet's do the same for shortcut
Awesome!
Let's fix it in #2018009: Move language element submit to process
Comment #103
YesCT CreditAttribution: YesCT commentedI'll be away for a few if someone wants to fix anything.
Comment #104
YesCT CreditAttribution: YesCT commentedthe prefix showed in the UI, but wasn't actually used to save the config.
Comment #105
kfritscheShould we use a menu prefix?
Because it was in before and we removed it in this patch.
If we want it, than we should revert this in the patch, like I mentioned in #81:
But I don't know if we need it.
Comment #106
YesCT CreditAttribution: YesCT commentedI hope we can decide we dont need it. And that that change is ok.. because also, it was causing a problem with the first time, the config in language settings was getting saved without the prefix, then the changes were not noticed and on second save, were saved with the prefix... something about the order of things happening, the menu- was being added too late after the config stuff was being saved.
Comment #107
YesCT CreditAttribution: YesCT commentedOh, @tim.plunkett pointed out to me that menu- was in D7.
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/me...
so we might have to make that still work.
And I was stuck and could not figure it out.
Maybe @tstoeckler can see why the
- '#field_prefix' => 'menu-',
did not *actually* make the machine name be prefixed in the language settings config file.
Comment #108
Gábor Hojtsy@YesCT: I'd hope we don't try to fire off a menu naming scheme discussion here. It would IMHO be important to keep the menu naming as-is and proceed without going into that trap :)
Comment #109
tstoecklerThis should fix that issue. I totally thought #field_prefix did that automatically but we have to add the prefix ourselves. Sorry @YesCT for misleading you!
Comment #110
tstoecklerNow actually reviewed the code in detail. Finally...
Overall it looks great. I think we can polish the tests a little bit and then this should be go IMO.
This should be $this->randomString();
The comment could be more clear, IMO. Maybe something like "Check that the settings were saved correctly and now show up as the default values of the form."
This is conceptually one step before the previous one. Also, if we're being that specific, we should also test that menu_load('menu', $menu_name)->langcode is also correct.
Again, should be randomString()
Seems that this function should be moved into a MenuWebTestBase
This is not actually a hook invocation, I think this is used only because a direct function call would fatal in case language.module does not exist. I think we should make that more explicit.
As the menu entity type is provided by system module, we can simply do entity_load('menu', $menu_link->menu_name); without the check for menu module.
$menu_link->langcode should be set up in this case already, so no need to call Language::LANGCODE_NOT_SPECIFIED manually.
I wonder if this works with the fallback if language.module is disabled. That should be tested.
I'll give this a shot.
Comment #111
Gábor Hojtsy@tstoeckler: great review, thanks!
Comment #112
tstoecklerHere we go. This would be good to go from my perspective. I'll comment on some of the changes below.
I didn't test this locally, because it just took to long. Hopefully it's green...
Comment #114
tstoecklerThis moves the menu_overview_form, i.e. the list of menu links to the bottom again. With the previous patch you would have title/description, then menu links, then language settings. I found that very confusing.
I had accidentally put that function below a submit handler, which made very little sense...
Ahh, I missed that: This should be broken into multiple lines IMO, as I did with the other ones.
Is this comment sufficiently clear?
Nooo!!! Dammit...
Although not strictly related to this issue, I do think injecting things where we can makes sense. In this issue we add the dependency on the module handler, so IMO injecting only that and leaving the rest legacy-style felt strange.
language_get_default_configuration() always returns a properly filled array, so I removed the checks.
Not relying on the existence of menu module, which we don't have to, allowed me to remove this condition.
I found using a separate variable more clear because with the new code flow $language_configuration is not always defined.
Comment #115
tstoecklerHow the hell did that 'g' get there?
Sorry for the noise...
Comment #116
YesCT CreditAttribution: YesCT commentedI wonder why no comma at the end of the last line here.
oops.
We can do those next time we re-do the patch.
here is the syntax error.
Oh I see you found that already. ;)
Comment #117
YesCT CreditAttribution: YesCT commentedI'm taking out the #prefix
It's an existing issue that during menu add, the machine name is shown without the menu- prefix, but gets saved with that prefix.
Not our problem here, just noticed here.
It can be fixed in:
#2021571: preview of menu machine name is inaccurate on while adding a menu (does not show it will be saved with menu- prefix)
Comment #118
YesCT CreditAttribution: YesCT commentedI think we are *really* close here.
A detailed review and one (?) more manual test, be sure to cat the language.settings.yml in config when testing too.
moving the helper function, so fixed the @params to add types on them. also fixes the nits from #116, and since we have the separate issue for the prefix, this hopefully is just like core without this patch now.
I dont think we do a "see above" pattern anywhere else in core. Maybe we can just explain it in one place. Maybe it's ok like it is.
Comment #120
YesCT CreditAttribution: YesCT commentedwe are not close. this prefixing is a pain.
I think maybe we need to only validate when the menu is new... but I dont know how to do that.
?
But how do I pass in the extra arg when submitting the form?
check this:
ps. I'm also getting ajax http 500 errors when the batch runs tests...
similar to:
#2019485: 500 Internal server error, during batch enable of translation from the content language settings page
Comment #121
YesCT CreditAttribution: YesCT commentedthis is just silly.
Comment #122
Gábor Hojtsy@tstoeckler, @kfritsche: can the two of you take over this issue? :)
Comment #123
tstoecklerHere we go. Still haven't managed to get the tests to run in a reasonable time, so there you go, bot.
Comment #125
kfritscheThis should fix the exceptions. While editing a menu link the language wasn't changeable anymore.
Comment #126
YesCT CreditAttribution: YesCT commented@kfritsche what was causing the exception?
Comment #128
kfritscheOkay somethings wrong.
The fails before my patch was caused because you couldn't change the language on a menu_item anymore, after it was created.
The exceptions happened because in the MenuTest we removed at some point the "menu-" prefix for custom menus. Now this exceptions doesn't happened anymore, but we have a couple of new failures. Which is somehow good, because I think the exceptions before caused the test suite to not test the menu stuff at all.
I don't think I can work on this before the sprint next week any further, but if this would be still open, this would be my first focus on Monday.
Comment #129
YesCT CreditAttribution: YesCT commented@tstoeckler @kfritsche I assigned this to myself. If you are working on it, ping me in irc.
I see the isNew() condition is reversed and working on another fail.
if it's new, it doesn't have a langcode yet. that's when it needs to get the default from the config.
Comment #130
tstoecklerNo biggie, but I don't see how #123 prevents you from changing the langcode on an existing menu item. I didn't verify it, i.e. I believe you that it does, but it's not clear to me from the code. I think conceptually the condition is clearer in #123 than in #125. Again, not at all married to my code... just sayin'
I hope to take another stab at this later tonight. @YesCT: Hopefully to RTBC the patch you will have uploaded :-)
Comment #131
YesCT CreditAttribution: YesCT commentedthis will pass more.
in addition to fixing the reversed ? mentioned in #129
this also goes to the menu link edit page before checking if the select is there (and has the right value).
Also adds commas to the last line in array() statements.
Still needs a full review.
Comment #133
YesCT CreditAttribution: YesCT commentedI think the fails on menu add language https://qa.drupal.org/pifr/test/552243
are from the new default language stuff in menu test
@tstoeckler added in #112
I wonder if that's testing if menu items are saved with the site default, even without content translation/language module enabled.
language for menu items is stored in the menu items table in the db if that helps.
(since the test results disappear, like when retesting,... I attached a screenshot of them.)
Drupal\menu\Tests\MenuTest 1913 10 0
Message Group Filename Line Function Status
Parameter langcode had expected value. Other MenuTest.php 222 Drupal\menu\Tests\MenuTest->doMenuTests()
Parameter langcode had expected value. Other MenuTest.php 230 Drupal\menu\Tests\MenuTest->doMenuTests()
etc.
We should manually test without enabling language or content translation and see what gets saved on menu item links. Also, we can compare to vocabular terms under similar conditions.
Comment #134
kfritsche@YesCT:
Wow. Nice catch. Sorry for such a hidden error ;)
@tstoeckler:
From your patch #123
Everything which could set $language_show to true and so display the language form is inside of "if isNew()". While editing a menu link it never gets inside this if block and thats the reason it doesn't show.
Comment #135
kfritscheBack to topic.
Installation with minimal profile and activated menu module. (but no language)
Added new menu link.
Checked database (
select menu_name, link_title, langcode from menu_links;
)Results in "en" for the new link and in "und" for all admin links created automatically before.
Problem is following line in MenuLinkFormController:199:
$default_langcode = ($menu_link->isNew()) ? entity_load('menu', $menu_link->menu_name)->langcode : $menu_link->langcode;
This sets the language code to the language of the menu. And this is "en" and not "site_default". Should this be site_default? Then we should change the menu/config/menu.menu.*.yml files and set langcode to site_default there. Otherwise I think the problem is very similar to #2015301: Wrong langcode after D8 installation with other Language. We have in some default config files the langcode: en, but this issn't always the site default or not even installed anymore.
Comment #136
tstoecklerHere we go, this is green on my machine.
This only fixes the test to asser 'en' as langcode insteaf of 'site_default'. I also added some comments. I now understand why my logic was flawed and that #125 and #131 are the correct fixes, but I've now documented the logic ind code.
Assuming this is (finally!!!) green, this shouldn't be far from RTBC IMO, but I won't have any time to work on it during the weekend.
Comment #138
tstoeckler#136: 1945226-136-menu-language.patch queued for re-testing.
EDIT: Please Mr. Bot, please tell me that was a random failure.
Comment #140
tstoeckler#136: 1945226-136-menu-language.patch queued for re-testing.
EDIT: This time it was line 31 of Drupal\views\Tests\Handler\ArgumentStringTest->testGlossary(), a different fail than before. (Sorry for not recording it last time, I forgot that the testbot is forgetful...)
Comment #141
Kristen PolGreen!!!
tstoeckler++
I scanned the code and don't see anything obviously wrong but I've only been a spectator on this issue and someone more involved should RTBC :)
Comment #142
kfritscheVery nice. Finally.
Changes seems fine to me and it works now. Will talk about #2015301: Wrong langcode after D8 installation with other Language with Gabor as follow up.
Good work all. Setting to RTBC.
Comment #143
YesCT CreditAttribution: YesCT commentedthis has had lots of manual testing and should be fine.
this is replacing \Drupal
and injects the module handler instead.
-----
[updated for clarity June 26 2013]
Notes
For those who might be new to injecting, and so I can remember what I did.
Since the MenuFormController extends EntityFormController,
look for other things that extend EntityFormController and also inject the module handler.
Looked at actions since it was recent and might have a good pattern to follow
also looked at #2004408: Allow modules to alter the result of EntityListController::getOperations since I had injected the module handler in that issue also
also looked at the ViewEditFormController. But the ViewEditFormController used $operator
in createInstance()/__construct() which was an older pattern, so that wasn't really needed.
Made the arguments in createInstance
match EntityControllerInterface.
Made the order of the arguments of the constructor
match EntityFormController (had operation), and added on the handler.
Made the objects in the static in createInstance
match the order of the arguments of the constructor.
It is a more common suggested pattern to put __construct() before the createInstance.
Comment #144
Gábor Hojtsy@alexpott suggested the injection of the module handler in his "over the air" review.
I think the proposed changes look good. The only thing I noticed:
Operation not documented.
Comment #145
helenkim CreditAttribution: helenkim commentedI copied the documentation on ViewEditFormController.
Comment #146
Gábor HojtsyLooks great assuming #143/#145 would pass. They looked to have great changes on code review.
Comment #147
alexpottCommitted 3a89eb5 and pushed to 8.x. Thanks!
Probably need to update an existing change notice.
Comment #148
andypostYay! Now we can adopt same kind of code for #111715: Convert node/content types into configuration
Comment #149
YesCT CreditAttribution: YesCT commentedA followup for this issue:
#2027335: If EntityFormController implements EntityControllerInterface it can ignore $operation in createInstance()/__construct()
Comment #150
YesCT CreditAttribution: YesCT commentedI'm writing the change notice for this.
Comment #151
tstoecklerI added https://drupal.org/node/2027579. Can someone fix the links there, plz? Drupal.org hates me again, today...
Comment #152
tstoecklerOops, that was a cross-post. Hmm..
Comment #153
Gábor HojtsyI reviewed that. Fixed the links and added one more cross-reference. Change notices cannot be linked in the form of [#...], they should be linked with their full URL. Only issues are referrable in the [#....] form. The change notice looks good.
Comment #154
YesCT CreditAttribution: YesCT commentedrelated #2029197: remove enforced menu- prefix to custom menu machine names
(also updating tags)
Comment #155.0
(not verified) CreditAttribution: commentedadded related issue for bundle always tools