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:

CommentFileSizeAuthor
#145 interdiff-143-145.txt652 byteshelenkim
#145 1945226-145-menu-language.patch26.74 KBhelenkim
#143 1945226-143-menu-language.patch26.67 KBYesCT
#143 interdiff-136-143.txt3.08 KBYesCT
#136 1945226-136-menu-language.patch25.66 KBtstoeckler
#136 interdiff-131-136.txt5.58 KBtstoeckler
#133 tests-langcode.png233.57 KBYesCT
#131 drupal-1945226-Add-language-selector-on-menus-130.patch25.7 KBYesCT
#131 interdiff-125-130.txt2.6 KBYesCT
#125 drupal-1945226-Add-language-selector-on-menus-125.patch25.44 KBkfritsche
#125 interdiff-123-125.txt1.91 KBkfritsche
#123 interdiff-118-123.txt2.63 KBtstoeckler
#123 1945226-123-menu-language.patch25.87 KBtstoeckler
#121 interdiff-118-121.txt3.8 KBYesCT
#120 menu-menu-.png263.16 KBYesCT
#118 1945226-118-menu-language.patch25.23 KBYesCT
#118 interdiff-115-118.txt2.24 KBYesCT
#115 1945226-115-menu-language.patch25.69 KBtstoeckler
#115 interdiff-112-115.txt699 byteststoeckler
#112 1945226-111-menu-language.patch25.69 KBtstoeckler
#112 interdiff-109-111.txt24.94 KBtstoeckler
#109 1945226-108-menu-language.patch15.58 KBtstoeckler
#109 interdiff-104-108.txt1.25 KBtstoeckler
#104 drupal-1945226-Add-language-selector-on-menus-104.patch14.85 KBYesCT
#104 interdiff-101-104.txt749 bytesYesCT
#101 drupal-1945226-Add-language-selector-on-menus-101.patch15.3 KBYesCT
#101 interdiff-96-101.txt755 bytesYesCT
#96 drupal-1945226-Add-language-selector-on-menus-96.patch15.18 KBYesCT
#96 interdiff-93-96.txt1.47 KBYesCT
#94 drupal-1945226-Add-language-selector-on-menus-93.patch14.96 KBYesCT
#94 interdiff-84-93.txt5.39 KBYesCT
#90 nochange.png127.02 KBYesCT
#90 mynewmenuedit.png122.71 KBYesCT
#90 nochangeonedit.png148.49 KBYesCT
#90 vocabedit.png137.46 KBYesCT
#84 drupal-1945226-Add-language-selector-on-menus-84.patch15.96 KBIshaDakota
#84 interdiff.txt930 bytesIshaDakota
#81 drupal-1945226-Add-language-selector-on-menus-81.patch15.94 KBkfritsche
#81 interdiff-78-81.txt5.09 KBkfritsche
#78 drupal-1945226-Add-language-selector-on-menus-78.patch13.66 KBYesCT
#78 interdiff-76-78.txt3.6 KBYesCT
#76 drupal-1945226-Add-language-selector-on-menus-76.patch13.82 KBYesCT
#76 interdiff-74-76.txt7.53 KBYesCT
#75 drupal-1945226-Add-language-selector-on-menus-74.patch13.37 KBYesCT
#75 interdiff-72-74.txt2.22 KBYesCT
#72 whattotest.png819.86 KBYesCT
#72 drupal-1945226-Add-language-selector-on-menus-72.patch12.84 KBYesCT
#72 interdiff-67-72.txt7.23 KBYesCT
#67 drupal-1945226-Add-language-selector-on-menus-67.patch11.78 KBYesCT
#67 interdiff-65-67.txt5.82 KBYesCT
#65 interdiff-62-fix.txt3.56 KBYesCT
#65 interdiff-addtests.txt4.53 KBYesCT
#65 drupal-1945226-Add-language-selector-on-menus-65.patch11.66 KBYesCT
#62 field_prefix_menu.png209.9 KBYesCT
#62 interdiff-61-62.txt1.08 KBYesCT
#62 drupal-1945226-Add-language-selector-on-menus-62.patch6.9 KBYesCT
#61 drupal-1945226-Add-language-selector-on-menus-61.patch6.55 KBYesCT
#61 interdiff-59-61.txt3.2 KBYesCT
#59 drupal-1945226-Add-language-selector-on-menus-59.patch8.32 KBYesCT
#59 interdiff-57-59.txt3.08 KBYesCT
#57 drupal-1945226-Add-language-selector-on-menus-57.patch7.74 KBYesCT
#57 interdiff-56-57.txt3.85 KBYesCT
#56 drupal-1945226-Add-language-selector-on-menus-56.patch6.46 KBYesCT
#56 interdiff-55-defaultlangcode.txt2 KBYesCT
#56 interdiff-defaultlangcode-56.txt1.11 KBYesCT
#55 drupal-1945226-Add-language-selector-on-menus-55.patch5.85 KBYesCT
#55 interdiff-53-55.txt2.65 KBYesCT
#54 drupal-1945226-Add-language-selector-on-menus-53.patch3.55 KBYesCT
#54 interdiff-50-53.txt1.62 KBYesCT
#50 drupal-1945226-Add-language-selector-on-menus-50.patch3.51 KBYesCT
#50 interdiff-49-50.txt827 bytesYesCT
#49 drupal-1945226-Add-language-selector-on-menus-49.patch3.47 KBYesCT
#49 interdiff-42-49.txt3.08 KBYesCT
#43 content_language_settings.png147.93 KBYesCT
#42 drupal-1945226-Add-language-selector-on-menus-42.patch4.28 KBYesCT
#28 translatable.png226.48 KBYesCT
#28 drupal-1945226-Add-language-selector-on-menus-28.patch4.87 KBYesCT
#28 interdiff-22-28.txt1.11 KBYesCT
#27 alwaysund.png216.3 KBYesCT
#27 menulinkitemsdefault.png186.35 KBYesCT
#24 nomenu.png187.98 KBYesCT
#22 drupal-1945226-Add-language-selector-on-menus-22.patch3.76 KBYesCT
#22 interdiff-3-22.txt2.12 KBYesCT
#20 s00a-adminmenu.png181.79 KBYesCT
#20 s00b-editadministrationitem.png224.51 KBYesCT
#20 s00c-addlink.png230.66 KBYesCT
#20 s00d-addcustommenu.png155.12 KBYesCT
#20 s00e-custommenuedit.png163.66 KBYesCT
#20 s00f-custommenulinkadd.png225.74 KBYesCT
#20 s01a-adminmenu.png177.38 KBYesCT
#20 s01ai-scrolleddown.png62.57 KBYesCT
#20 s01aii-selectvalues.png68.05 KBYesCT
#20 s01d-custommenuedit.png238.95 KBYesCT
#20 s01f-custommenulinkadd.png248.5 KBYesCT
#13 Screenshot_4_3_13_6_18_PM.png76.63 KBGábor Hojtsy
#11 patch.pdf43.13 KBno_angel
#11 no-patch.pdf41.79 KBno_angel
#4 language-selector-on-menu.png109.91 KBbabruix
#3 drupal-1945226-Add-language-selector-on-menus-3.patch3.71 KBbabruix
#3 Screen Shot 2013-03-19 at 1.07.32 PM.png109.91 KBbabruix
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +sprint

Also putting on sprint.

YesCT’s picture

Issue tags: +medium, +needs initial patch

should be good for someone new.
ready for an initial patch. get inspiration for how from the views issue. :)

babruix’s picture

Just not sure if we need selector "MENU ITEMS LANGUAGE" - but I`ve added it, as vocabulary edit page has similar for terms.

babruix’s picture

FileSize
109.91 KB

language for menu

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, 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?

Gábor Hojtsy’s picture

Issue tags: +sprint

Don't know how sprint was removed, it should be on.

YesCT’s picture

Issue tags: -needs initial patch +Needs tests, +Needs manual testing

initial 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)

Gábor Hojtsy’s picture

Indeed, both manual and automated testing is needed.

Gábor Hojtsy’s picture

Anybody up for doing either? :)

no_angel’s picture

Assigned: Unassigned » no_angel

I will try to do the manual testing on the patch.

no_angel’s picture

FileSize
41.79 KB
43.13 KB

Failed 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)

no_angel’s picture

Assigned: no_angel » Unassigned

unassigned.
maybe the patch needs to be applied to the menu.module???

Gábor Hojtsy’s picture

The language selector displays for me, so I think you have not actually checked the patched site?

Screenshot_4_3_13_6_18_PM.png

no_angel’s picture

thanks @Gábor Hojtsy for confirming the patch. I'm new and still trying to sort my way thru the git stuff.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,6 +63,31 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
+    // $form['langcode'] is not wrapped in an if (module_exists('language'))
+    // check because the language_select form element works also without the
+    // language module being installed.
+    // http://drupal.org/node/1749954 documents the new element.

comment 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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,10 +101,38 @@ protected function actions(array $form, array &$form_state) {
    */
   public function save(array $form, array &$form_state) {
-- 

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.

YesCT’s picture

oh 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.

Gábor Hojtsy’s picture

The 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.

YesCT’s picture

here is what I get when I apply this:

[attr]drupaltext    text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
 not allowed: /Users/ctheys/foo/d8-git/.gitattributes:13
[attr]drupalbinary  -text diff
 not allowed: /Users/ctheys/foo/d8-git/.gitattributes:18

Anyway, I'll test this now.

YesCT’s picture

steps:

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

id: menu-test-menu-machinename
uuid: a7ab0123-83fb-4042-9858-8826409619fe
label: 'test menu title'
description: 'test menu administrative summary'
status: '1'
langcode: 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.

YesCT’s picture

with patch, but without language module enabled here are screenshots of forms:

0a admin menu
s00a-adminmenu.png

0b admin menu link item edit
s00b-editadministrationitem.png

0c menu link item add
s00c-addlink.png

0d custom menu add
s00d-addcustommenu.png

0e custom menu edit
s00e-custommenuedit.png

0f custom menu link item add
s00f-custommenulinkadd.png

with patch, with language module enabled (but no other languages added, and no translation modules enabled), here are screenshots of forms:

1a admin menu
s01a-adminmenu.png

1ai admin menu scrolled all the way down
s01ai-scrolleddown.png

1aii admin menu select values
s01aii-selectvalues.png

1d custom menu edit
s01d-custommenuedit.png

1f custom menu link item add
s01f-custommenulinkadd.png

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.

YesCT’s picture

Assigned: Unassigned » YesCT

I'm working on the patch for this.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
3.76 KB

@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.

YesCT’s picture

Status: Needs review » Needs work

I also have seen that the comment I thought was odd

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,6 +63,31 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
+    // $form['langcode'] is not wrapped in an if (module_exists('language'))
+    // check because the language_select form element works also without the
+    // language module being installed.
+    // http://drupal.org/node/1749954 documents the new element.

Comes right from the VocabularyFormController.php

YesCT’s picture

FileSize
187.98 KB
menu_item:
  menu_test_menu_machinename:
    language:
      default_configuration:
        langcode: site_default
        language_show: '0'

is being added to
sites/default/files/config_zNb0Um-o1E8or0sWm5zUI-ZZuIxvfhw5KnDpLc9cAC8/active/language.settings.yml

But not to the settings page:
nomenu.png

YesCT’s picture

$ grep -R "Custom language settings" *
core/modules/language/language.admin.inc: '#title' => t('Custom language settings'),

and in there:

  foreach ($supported as $entity_type) {
    $info = $entity_info[$entity_type];

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

function language_content_settings_page() {
  return drupal_get_form('language_content_settings_form', language_entity_supported());

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

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Entity/Menu.php
@@ -20,6 +20,7 @@
  *   module = "system",
  *   controller_class = "Drupal\Core\Config\Entity\ConfigStorageController",
  *   config_prefix = "menu.menu",
+ *   translatable = TRUE,
  *   entity_keys = {
  *     "id" = "id",
  *     "label" = "label",

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.

YesCT’s picture

Assigned: YesCT » Unassigned

oh, 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.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
@@ -29,6 +29,8 @@
  *   static_cache = FALSE,
  *   base_table = "menu_links",
  *   uri_callback = "menu_link_uri",
+ *   fieldable = TRUE,
+ *   translatable = TRUE,
  *   entity_keys = {
  *     "id" = "mlid",
  *     "label" = "link_title",

*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.

YesCT’s picture

FileSize
186.35 KB
216.3 KB

To clarify,

menu link items are always showing the language select, and always as "not specified" (und).

alwaysund.png

no matter what is set on the menu.

menulinkitemsdefault.png

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
4.87 KB
226.48 KB

this 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.

translatable.png

YesCT’s picture

in core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.php

/**
 * Defines the taxonomy term entity.
 *
 * @Plugin(
 *   id = "taxonomy_term",
 *   label = @Translation("Taxonomy term"),
 *   bundle_label = @Translation("Vocabulary"),
 *   module = "taxonomy",
 *   controller_class = "Drupal\taxonomy\TermStorageController",
 *   render_controller_class = "Drupal\taxonomy\TermRenderController",
 *   access_controller_class = "Drupal\taxonomy\TermAccessController",
 *   form_controller_class = {
 *     "default" = "Drupal\taxonomy\TermFormController"
 *   },
 *   translation_controller_class = "Drupal\taxonomy\TermTranslationController",
 *   base_table = "taxonomy_term_data",
 *   uri_callback = "taxonomy_term_uri",
 *   fieldable = TRUE,
 *   translatable = TRUE,
 *   entity_keys = {
 *     "id" = "tid",
 *     "bundle" = "vid",
 *     "label" = "name",
 *     "uuid" = "uuid"
 *   },
 *   bundle_keys = {
 *     "bundle" = "vid"
 *   },
 *   menu_base_path = "taxonomy/term/%taxonomy_term",
 *   permission_granularity = "bundle"
 * )
 */
class Term extends Entity implements ContentEntityInterface {

And in core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php

/**
 * Defines the menu link entity class.
 *
 * @Plugin(
 *   id = "menu_link",
 *   label = @Translation("Menu link"),
 *   bundle_label = @Translation("Menu"),
 *   module = "menu_link",
 *   controller_class = "Drupal\menu_link\MenuLinkStorageController",
 *   render_controller_class = "Drupal\Core\Entity\EntityRenderController",
 *   form_controller_class = {
 *     "default" = "Drupal\menu_link\MenuLinkFormController"
 *   },
 *   static_cache = FALSE,
 *   base_table = "menu_links",
 *   uri_callback = "menu_link_uri",
 *   translatable = TRUE, // line added by me while playing with this.
 *   entity_keys = {
 *     "id" = "mlid",
 *     "label" = "link_title",
 *     "uuid" = "uuid"
 *   },
 *   bundles = {
 *     "menu_link" = {
 *       "label" = "Menu link",
 *     }
 *   }
 * )
 */
class MenuLink extends Entity implements \ArrayAccess, ContentEntityInterface {
 

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.

amateescu’s picture

We 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..

Gábor Hojtsy’s picture

@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 :)

Gábor Hojtsy’s picture

Yeah, 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).

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs review » Postponed

Looking 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.

Gábor Hojtsy’s picture

Status: Postponed » Needs work

Ok, 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.

+++ b/core/modules/language/language.moduleundefined
@@ -201,7 +201,7 @@ function language_theme() {
-    if (!empty($info['fieldable']) && !empty($info['translatable'])) {
+    if (!empty($info['translatable'])) {
       $supported[$entity_type] = $entity_type;
     }

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -29,6 +29,7 @@
  *   uri_callback = "menu_link_uri",
+ *   translatable = TRUE,
  *   entity_keys = {

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 :)

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,7 +62,32 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
+    $form['langcode'] = array(
+      '#type' => 'language_select',
+      '#title' => t('Menu language'),
+      '#languages' => LANGUAGE_ALL,
+      '#default_value' => $menu->langcode,
+    );
+    if (module_exists('language')) {
+      $form['default_menu_items_language'] = array(
+        '#type' => 'details',
+        '#title' => t('Menu items language'),
+      );
+      $form['default_menu_items_language']['default_language'] = array(
+        '#type' => 'language_configuration',
+        '#entity_information' => array(
+          'entity_type' => 'menu_item',
+          'bundle' => $menu->id(),
+        ),
+        '#default_value' => language_get_default_configuration('menu_item', $menu->id()),
+      );
+    }
+
+    return parent::form($form, $form_state, $menu);

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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,10 +100,38 @@ protected function actions(array $form, array &$form_state) {
 
+    // Add the language configuration submit handler. This is needed because the
+    // submit button has custom submit handlers.
+    if (module_exists('language')) {
+      array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
+      array_unshift($actions['submit']['#submit'], array($this, 'languageConfigurationSubmit'));
+    }
+    // We cannot leverage the regular submit handler definition because we have
+    // button-specific ones here. Hence we need to explicitly set it for the
+    // submit action, otherwise it would be ignored.
+    if (module_exists('translation_entity')) {
+      array_unshift($actions['submit']['#submit'], 'translation_entity_language_configuration_element_submit');
+    }
     return $actions;
   }
 
   /**
+   * Submit handler to update the bundle for the default language configuration.
+   */
+  public function languageConfigurationSubmit(array &$form, array &$form_state) {
+    $menu = $this->getEntity($form_state);
+    // Delete the old language settings for the menu, if the machine name
+    // is changed.
+    if ($menu && $menu->id() && $menu->id() != $form_state['values']['id']) {
+      language_clear_default_configuration('menu_item', $menu->id());
+    }
+    // Since the machine name is not known yet, and it can be changed anytime,
+    // we have to also update the bundle property for the default language
+    // configuration in order to have the correct bundle value.
+    $form_state['language']['default_language']['bundle'] = $form_state['values']['id'];
+  }
+

Don't need any of this for now :/

Gábor Hojtsy’s picture

Status: Needs work » Postponed
andypost’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -29,6 +29,7 @@
+ *   translatable = TRUE,

not sure it's right place for that, I add this in #1966298-31: Introduce menu link bundles per menus

+    if (Drupal::moduleHandler()->moduleExists('language')) {
+      $entity_info['menu_link']['translatable'] = TRUE;
+    }
Gábor Hojtsy’s picture

I 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.

YesCT’s picture

Issue summary: View changes

added separate issue to address how the content language settings page is built

Gábor Hojtsy’s picture

I 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.

Gábor Hojtsy’s picture

Issue summary: View changes

added link to comment where before screenshots are.

Gábor Hojtsy’s picture

YesCT’s picture

re-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.

YesCT’s picture

YesCT’s picture

This 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:

menu_item:
  new_menu:
    language:
      default_configuration:
        langcode: en
        language_show: '0'
  menu_new_menu:
    language:
      default_configuration:
        langcode: af
        language_show: '0'
taxonomy_term:
  tags:
    language:
      default_configuration:
        langcode: af
        language_show: '0'
YesCT’s picture

menu 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


/**
 * Returns whether a menu name already exists.
 *
 * @see menu_edit_menu()
 * @see form_validate_machine_name()
 */
function menu_edit_menu_name_exists($value) {
  $custom_exists = entity_load('menu', $value);
  // 'menu-' is added to the menu name to avoid name-space conflicts.
  $value = 'menu-' . $value;
  $link_exists = Drupal::entityQuery('menu_link')->condition('menu_name', $value)->range(0,1)->count()->execute();

  return $custom_exists || $link_exists;
}
andypost’s picture

The same happens in shortcut module with different prefix

Gábor Hojtsy’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-42.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
3.47 KB

(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:

$ cat menu.menu.menu-my-menu.yml
id: menu-my-menu
uuid: 5c472a9b-8b0f-4627-9b00-a0d446f9e41e
label: 'My menu'
description: 'my menu admin summary'
status: '1'
langcode: sq

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

$ cat language.settings.yml 
comment:
  comment_node_article:
    language:
      default_configuration:
<snip>
custom_block:
  basic:
    language:
      default_configuration:
        langcode: site_default
        language_show: '0'
menu_link:
  account:
    language:
      default_configuration:
<snip>
  main:
    language:
      default_configuration:
        langcode: site_default
        language_show: '0'
  menu_my_menu:
    language:
      default_configuration:
        langcode: af
        language_show: '1'
  tools:

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:

-      array_unshift($actions['submit']['#submit'], array($this, 'languageConfigurationSubmit'));

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".

YesCT’s picture

ah, 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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,6 +101,18 @@ protected function actions(array $form, array &$form_state) {
+    // Add the language configuration submit handler. This is needed because the
+    // submit button has custom submit handlers.
+    if (module_exists('language')) {
+      array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
+    }
+    // We cannot leverage the regular submit handler definition because we have
+    // button-specific ones here. Hence we need to explicitly set it for the
+    // submit action, otherwise it would be ignored.
+    if (module_exists('translation_entity')) {
+      array_unshift($actions['submit']['#submit'], 'translation_entity_language_configuration_element_submit');
+    }
+

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

Gábor Hojtsy’s picture

@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.

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks, Cathy!

I'm late in this game but here's a code review.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,7 +64,32 @@ public function form(array $form, array &$form_state) {
+    // $form['langcode'] is not wrapped in an if (module_exists('language'))
+    // check because the language_select form element works also without the

Still some module_exists references.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,6 +102,18 @@ protected function actions(array $form, array &$form_state) {
+    if (module_exists('language')) {

Still some module_exists references.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,6 +102,18 @@ protected function actions(array $form, array &$form_state) {
+    if (module_exists('translation_entity')) {
+      array_unshift($actions['submit']['#submit'], 'translation_entity_language_configuration_element_submit');

Still some module_exists references.

Nothing else caught my eye. I did not test it yet.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
3.55 KB

just addressing #52 for the moment.

YesCT’s picture

bundle 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.

!

YesCT’s picture

first 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

YesCT’s picture

ah, 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-57.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
8.32 KB

well, 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.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,10 +102,50 @@ protected function actions(array $form, array &$form_state) {
+    // Add the language configuration submit handler. This is needed because the
+    // submit button has custom submit handlers.
+    if (Drupal::moduleHandler()->moduleExists('language')) {
+      array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
+    }
...
+    // Add the language configuration submit handler. This is needed because the
+    // submit button has custom submit handlers.
+    if (Drupal::moduleHandler()->moduleExists('language')) {
+      array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
+      array_unshift($actions['submit']['#submit'], array($this, 'languageConfigurationSubmit'));
+    }

oops. added it twice.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
3.2 KB
6.55 KB

this 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]

YesCT’s picture

I 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.

field_prefix_menu.png

Next... tests.

andypost’s picture

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -7,7 +7,9 @@
+use Drupal;

@@ -63,7 +64,32 @@ public function form(array $form, array &$form_state) {
+    if (Drupal::moduleHandler()->moduleExists('language')) {

@@ -76,10 +102,38 @@ protected function actions(array $form, array &$form_state) {
+    if (Drupal::moduleHandler()->moduleExists('language')) {
...
+    if (Drupal::moduleHandler()->moduleExists('translation_entity')) {

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -7,6 +7,7 @@
+use Drupal;

@@ -156,11 +157,27 @@ public function form(array $form, array &$form_state) {
+    $language_configuration = Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('menu_link', $menu_link->menu_name));

Just use \Drupal in classes

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -18,7 +20,6 @@ class MenuFormController extends EntityFormController {
     $menu = $this->entity;

@@ -76,10 +102,38 @@ protected function actions(array $form, array &$form_state) {
+    $menu = $this->getEntity($form_state);

$menu = $this->entity

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -32,6 +32,7 @@
+ *   translatable = TRUE,

Maybe better to leave this default?

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-62.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
11.66 KB
4.53 KB
3.56 KB

fix 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-65.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
5.82 KB
11.78 KB

the 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)

YesCT’s picture

ah, 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.

tstoeckler’s picture

I didn't read the entire issue, so I might have missed something that was already discussed.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -18,7 +19,6 @@ class MenuFormController extends EntityFormController {
-    $form = parent::form($form, $form_state);

Was this taken out intenionally? If so, why?

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -63,7 +64,33 @@ public function form(array $form, array &$form_state) {
+    if (\Drupal::moduleHandler()->moduleExists('language')) {
+      $form['default_menu_links_language'] = array(
+        '#type' => 'details',
+        '#title' => t('Menu links language'),
+      );
+      $form['default_menu_links_language']['default_language'] = array(
+        '#type' => 'language_configuration',
+        '#entity_information' => array(
+          'entity_type' => 'menu_link',
+          'bundle' => $menu->id(),
+        ),
+        '#default_value' => language_get_default_configuration('menu_link', $menu->id()),
+      );

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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -76,10 +103,38 @@ protected function actions(array $form, array &$form_state) {
+    if ($menu && $menu->id() && $menu->id() != $form_state['values']['id']) {
+      language_clear_default_configuration('menu_item', $menu->id());
+    }

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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -76,10 +103,38 @@ protected function actions(array $form, array &$form_state) {
+    $form_state['language']['default_language']['bundle'] = $form_state['values']['id'];

I've never seen $form_state['language'], it looks fancy! :-)

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
--- a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php

I think all this logic should be in MenuLink::create() or somewhere. It doesn't seem specific to the form, right?

tstoeckler’s picture

Was this taken out intenionally? If so, why?

Ahh, it's only moved to the end... Sorry, stupid me.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-67.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Update dependencies.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
12.84 KB
819.86 KB

Note, @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.
whattotest.png

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-72.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

I think there are too many drupalGet

Also, I dont know how to check the language on the menu item was actually saved correctly.

    // Add a menu link.
    $this->drupalGet('admin/structure/menu/manage/' . $menu_name . '/add');
    $title = $this->randomName(16);
    $edit = array(
      'link_title' => $title,
      'link_path' => $sample_link_path,
    );
    $this->drupalPost('admin/structure/menu/manage/' . $menu_name . '/add', $edit, t('Save'));
    // Check the link is added.
    // Check the link has the correct language.
    // admin/structure/menu/item/329/edit
    // How to get the item number. Might need to:
    // $item1 = $this->addMenuLink(0, 'node/' . $node1->nid, $menu_name);
    // $this->assertMenuLink($menu_link->id(), array('menu_name' => $menu_name, 'link_path' => $link, 'has_children' => 0, 'plid' => $plid));

I think that is the next thing to go.

YesCT’s picture

YesCT’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-76.patch, failed testing.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.6 KB
13.66 KB

Opened
#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.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-78.patch, failed testing.

kfritsche’s picture

Assigned: Unassigned » kfritsche

If 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:

id: tools
uuid: ...
label: Tools
description: 'Contains links for site visitors. Some modules add their links here.'
status: '1'
langcode: en

Should be:

id: tools
uuid: ...
label: Tools
description: 'Contains links for site visitors. Some modules add their links here.'
status: '1'
langcode: de

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.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
5.09 KB
15.94 KB

This 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:

-    if ($menu->isNew()) {
-      // Add 'menu-' to the menu name to help avoid name-space conflicts.
-      $menu->set('id', 'menu-' . $menu->id());
-    }

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.

kfritsche’s picture

Assigned: kfritsche » Unassigned

Tests 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.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Novice
+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,165 @@
+    // Delete custom menu.
+    // @todo Make an issue to add tests for deleting, and verifying config is
+    //   also removed.
+    // Check that the language settings are not still there.
+    // $language_settings = language_get_default_configuration('taxonomy_term', $edit['vid']);
+    // $this->assertEqual($language_settings['langcode'], 'bb');

Take 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

     // @todo Test deleting a custom menu also deletes the language settings config.
     //   https://drupal.org/node/2017127

This little part is novice.

IshaDakota’s picture

Status: Needs work » Needs review
FileSize
930 bytes
15.96 KB

Adds link to the todo issue as described in #83

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks pretty good :) Some notes:

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,7 +64,32 @@ public function form(array $form, array &$form_state) {
+    // $form['langcode'] is not wrapped in a check if language module exists
+    // check because the language_select form element works also without the
+    // language module being installed.
+    // http://drupal.org/node/1749954 documents the new element.

Is this copied from somewhere? I'm not sure we should repeat this doc all around :) At least I'd shorten it.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,10 +102,38 @@ protected function actions(array $form, array &$form_state) {
+    // Delete the old language settings for the menu, if the machine name
+    // is changed.
+    if ($menu && $menu->id() && $menu->id() != $form_state['values']['id']) {
+      language_clear_default_configuration('menu_item', $menu->id());
+    }

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?

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,165 @@
+      'name' => 'Menu language',
+      'description' => 'Create menu in non-English language and menu link item.',

Create menu and menu item in non-English language.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,165 @@
+    // Assert the new menu is in that language and the menu link defaults.

// Check menu language and item language configuration.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,165 @@
+    // Start testing menu link things.
+    // Just link to front page.
+    $sample_link_path='<front>';

// Test menu link language.

Also, note whitespace issue in the assignment.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,165 @@
+    // Delete custom menu.
+    // @todo Test deleting a custom menu also deletes the language settings config.
+    //   https://drupal.org/node/2017127
+    // Check that the language settings are not still there.
+    // $language_settings = language_get_default_configuration('taxonomy_term', $edit['vid']);
+    // $this->assertEqual($language_settings['langcode'], 'bb');

Remove this section since this will be in a different issue / separate bug?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -156,11 +156,34 @@ public function form(array $form, array &$form_state) {
+    // Get the default langcode and show language select value.
+    // @todo Make this work with bundle instead of menu_name. Uses menu_name
+    //   right now to get around ->bundle being 'tools' for all new menu items.
+    //   http://drupal.org/node/2014617

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 :(

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -156,11 +156,34 @@ public function form(array $form, array &$form_state) {
+    if ($menu_link->isNew()) {
+      // Adding a new menu link, use the default from language settings config.
+      // If no default for menu links, default to the language of the menu.
+      if (!is_null($language_configuration['langcode'])) {
+        $default_langcode = $language_configuration['langcode'];
+      }
+      else {
+        $menu = menu_load($menu_link->menu_name);
+        $default_langcode = $menu->langcode;
+      }
+    }

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?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -156,11 +156,34 @@ public function form(array $form, array &$form_state) {
-      '#default_value' => $menu_link->langcode,
+      '#default_value' => $default_langcode,
+      '#access' => !is_null($language_configuration['language_show']) && $language_configuration['language_show'],

Is taxonomy, nodes, etc. using similar code? In other words is this code made up for menus or adapted from that pattern?

amateescu’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -156,11 +156,34 @@ public function form(array $form, array &$form_state) {
+    // Get the default langcode and show language select value.
+    // @todo Make this work with bundle instead of menu_name. Uses menu_name
+    //   right now to get around ->bundle being 'tools' for all new menu items.
+    //   http://drupal.org/node/2014617

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 :(

Totally agreed.

About this part:

+      else {
+        $menu = menu_load($menu_link->menu_name);
+        $default_langcode = $menu->langcode;
+      }

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..

Gábor Hojtsy’s picture

Ok, #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 :)

YesCT’s picture

Assigned: Unassigned » YesCT

I'll work on the new patch.

YesCT’s picture

#85 @Gábor Hojtsy

Is this copied from somewhere? I'm not sure we should repeat this doc all around :) At least I'd shorten it.

Yeah, See #15 and #23.
I'll change it.

--- a/core/modules/menu/lib/Drupal/menu/MenuFormController.php
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -64,16 +64,14 @@ public function form(array $form, array &$form_state) {
       $form['links'] = menu_overview_form($form['links'], $form_state);
     }
 
-    // $form['langcode'] is not wrapped in a check if language module exists
-    // check because the language_select form element works also without the
-    // language module being installed.
-    // http://drupal.org/node/1749954 documents the new element.
     $form['langcode'] = array(
       '#type' => 'language_select',
       '#title' => t('Menu language'),
       '#languages' => Language::STATE_ALL,
       '#default_value' => $menu->langcode,
     );
+    // Unlike the menu langcode, the default language configuration for menu
+    // links only works with language module installed.
     if (\Drupal::moduleHandler()->moduleExists('language')) {
       $form['default_menu_links_language'] = array(
         '#type' => 'details',
YesCT’s picture

Issue tags: -Novice
FileSize
137.46 KB
148.49 KB
122.71 KB
127.02 KB

#85 @Gábor Hojtsy

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?

Ah, good point.

Built in default menu:
nochange.png

while creating a new menu, can edit machine name

mynewmenuedit.png

but when editing that custom menu, no edit on machine name.

nochangeonedit.png

I thought there was an edit there, oh wait, that was for tags. yep.

vocabedit.png

Menus are different than vocabularies. Vocabularies can have their machine names changed on edit (after add/creation). Menu's cannot.

so:

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -119,12 +119,6 @@ protected function actions(array $form, array &$form_state) {
    * Submit handler to update the bundle for the default language configuration.
    */
   public function languageConfigurationSubmit(array &$form, array &$form_state) {
-    $menu = $this->entity;
-    // Delete the old language settings for the menu, if the machine name
-    // is changed.
-    if ($menu && $menu->id() && $menu->id() != $form_state['values']['id']) {
-      language_clear_default_configuration('menu_item', $menu->id());
-    }
     // Since the machine name is not known yet, and it can be changed anytime,

---------

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?

I dont know. I have not tried translating menu items.

YesCT’s picture

here is the only change due to #2014617: Unsaved menu links have the wrong bundle (always 'tools')

diff --git a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
index 9946e79..c86a00e 100644
--- a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -157,10 +157,7 @@ public function form(array $form, array &$form_state) {
     );
 
     // Get the default langcode and show language select value.
-    // @todo Make this work with bundle instead of menu_name. Uses menu_name
-    //   right now to get around ->bundle being 'tools' for all new menu items.
-    //   http://drupal.org/node/2014617
-    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('menu_link', $menu_link->menu_name));
+    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('menu_link', $menu_link->bundle()));
 
     if ($menu_link->isNew()) {
       // Adding a new menu link, use the default from language settings config.
YesCT’s picture

#86 @amateescu

+      else {
+        $menu = menu_load($menu_link->menu_name);
+        $default_langcode = $menu->langcode;
+      }
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..

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.

amateescu’s picture

The 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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
5.39 KB
14.96 KB

here 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.

Gábor Hojtsy’s picture

What 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.

YesCT’s picture

this 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

    $language_configuration = module_invoke('language', 'get_default_configuration', 'taxonomy_term', $vocabulary->id());
    $form['langcode'] = array(
      '#type' => 'language_select',
      '#title' => t('Language'),
      '#languages' => Language::STATE_ALL,
      '#default_value' => $term->langcode->value,
      '#access' => !is_null($language_configuration['language_show']) && $language_configuration['language_show'],
    );

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

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-96.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

I 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.

+++ b/core/modules/language/language.module
@@ -352,6 +352,7 @@ function language_configuration_element_submit(&$form, &$form_state) {
  */
 function language_save_default_configuration($entity_type, $bundle, $values = array()) {
   config('language.settings')->set(language_get_default_configuration_settings_key($entity_type, $bundle), arra
+  entity_info_cache_clear();
 }
 
 /**

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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -123,6 +123,7 @@ public function languageConfigurationSubmit(array &$form, array &$form_state) {
     // we have to also update the bundle property for the default language
     // configuration in order to have the correct bundle value.
     $form_state['language']['default_language']['bundle'] = $form_state['values']['id'];
+    entity_info_cache_clear();
   }
 
   /**

But I wonder about menus created other ways and not via the form.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +D8MI, +sprint, +language-config, +medium

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-96.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
755 bytes
15.3 KB
502 $ git diff
diff --git a/core/modules/menu/lib/Drupal/menu/MenuFormController.php b/core/mod
index 1651079..bd77ca7 100644
--- a/core/modules/menu/lib/Drupal/menu/MenuFormController.php
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -123,6 +123,9 @@ public function languageConfigurationSubmit(array &$form, ar
     // we have to also update the bundle property for the default language
     // configuration in order to have the correct bundle value.
     $form_state['language']['default_language']['bundle'] = $form_state['values
+    // Clear cache so new menus show on the language settings admin page.
+    $menu = $this->entity;
+    menu_cache_clear($menu->id());
   }
 
   /**

that didn't work

diff --git a/core/modules/menu/lib/Drupal/menu/MenuFormController.php b/core/mod
index 1651079..53b2a55 100644
--- a/core/modules/menu/lib/Drupal/menu/MenuFormController.php
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -123,6 +123,9 @@ public function languageConfigurationSubmit(array &$form, ar
     // we have to also update the bundle property for the default language
     // configuration in order to have the correct bundle value.
     $form_state['language']['default_language']['bundle'] = $form_state['values
+    // Clear cache so new menus show on the language settings admin page.
+    $menu = $this->entity;
+    menu_cache_clear_all();
   }
 
   /**

neither did this

so, I went with
entity_info_cache_clear();
in the language summit in the menu form controller.

andypost’s picture

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -35,6 +35,7 @@ public function form(array $form, array &$form_state) {
+      '#field_prefix' => 'menu-',

Let's do the same for shortcut

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -63,7 +64,30 @@ public function form(array $form, array &$form_state) {
+      $form['default_menu_links_language'] = array(
+        '#type' => 'details',
+        '#title' => t('Menu links language'),
+      );
+      $form['default_menu_links_language']['default_language'] = array(
+        '#type' => 'language_configuration',
+        '#entity_information' => array(
+          'entity_type' => 'menu_link',
+          'bundle' => $menu->id(),
+        ),
+        '#default_value' => language_get_default_configuration('menu_link', $menu->id()),

Awesome!

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -76,10 +100,32 @@ protected function actions(array $form, array &$form_state) {
+    // Add the language configuration submit handler. This is needed because the
+    // submit button has custom submit handlers.
+    if (\Drupal::moduleHandler()->moduleExists('language')) {
+      array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
+      array_unshift($actions['submit']['#submit'], array($this, 'languageConfigurationSubmit'));
+    }
+    // We cannot leverage the regular submit handler definition because we have
+    // button-specific ones here. Hence we need to explicitly set it for the
+    // submit action, otherwise it would be ignored.
+    if (\Drupal::moduleHandler()->moduleExists('translation_entity')) {
+      array_unshift($actions['submit']['#submit'], 'translation_entity_language_configuration_element_submit');
...
+   * Submit handler to update the bundle for the default language configuration.
+   */
+  public function languageConfigurationSubmit(array &$form, array &$form_state) {
+    // Since the machine name is not known yet, and it can be changed anytime,
+    // we have to also update the bundle property for the default language
+    // configuration in order to have the correct bundle value.
+    $form_state['language']['default_language']['bundle'] = $form_state['values']['id'];
+  }

Let's fix it in #2018009: Move language element submit to process

YesCT’s picture

Assigned: YesCT » Unassigned

I'll be away for a few if someone wants to fix anything.

YesCT’s picture

the prefix showed in the UI, but wasn't actually used to save the config.

kfritsche’s picture

Should 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:

-    if ($menu->isNew()) {
-      // Add 'menu-' to the menu name to help avoid name-space conflicts.
-      $menu->set('id', 'menu-' . $menu->id());
-    }

But I don't know if we need it.

YesCT’s picture

I 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.

YesCT’s picture

Oh, @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.

Gábor Hojtsy’s picture

@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 :)

tstoeckler’s picture

This 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!

tstoeckler’s picture

Status: Needs review » Needs work

Now 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.

+++ a/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -0,0 +1,157 @@
+    $label = $this->randomName(16);

This should be $this->randomString();

+++ a/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -0,0 +1,157 @@
+    // Check menu language and item language configuration.

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."

+++ a/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -0,0 +1,157 @@
+    // Check that the language settings were saved.

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.

+++ a/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -0,0 +1,157 @@
+    $title = $this->randomName(16);

Again, should be randomString()

+++ a/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -0,0 +1,157 @@
+   * @see \Drupal\menu\Tests\MenuTest::assertMenuLink()

Seems that this function should be moved into a MenuWebTestBase

+++ a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,11 +156,36 @@ public function form(array $form, array &$form_state) {
+    // Get the default langcode and show language select value.
+    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('menu_link', $menu_link->bundle()));

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.

+++ a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,11 +156,36 @@ public function form(array $form, array &$form_state) {
+      elseif (\Drupal::moduleHandler()->moduleExists('menu')) {
+        // If no default for menu links, default to the language of the menu.
+        $menu = menu_load($menu_link->menu_name);

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.

+++ a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,11 +156,36 @@ public function form(array $form, array &$form_state) {
+        // We do not know the language of the menu.
+        $default_langcode = Language::LANGCODE_NOT_SPECIFIED;

$menu_link->langcode should be set up in this case already, so no need to call Language::LANGCODE_NOT_SPECIFIED manually.

+++ a/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,11 +156,36 @@ public function form(array $form, array &$form_state) {
+      '#access' => !is_null($language_configuration['language_show']) && $language_configuration['language_show'],

I wonder if this works with the fallback if language.module is disabled. That should be tested.

I'll give this a shot.

Gábor Hojtsy’s picture

@tstoeckler: great review, thanks!

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
24.94 KB
25.69 KB

Here 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...

Status: Needs review » Needs work

The last submitted patch, 1945226-111-menu-language.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -52,18 +52,6 @@ public function form(array $form, array &$form_state) {
-    // Add menu links administration form for existing menus.
-    if (!$menu->isNew() || isset($system_menus[$menu->id()])) {
-      // Form API supports constructing and validating self-contained sections
-      // within forms, but does not allow to handle the form section's submission
-      // equally separated yet. Therefore, we use a $form_state key to point to
-      // the parents of the form section.
-      // @see menu_overview_form_submit()
-      $form_state['menu_overview_form_parents'] = array('links');
-      $form['links'] = array();
-      $form['links'] = menu_overview_form($form['links'], $form_state);
-    }

@@ -87,6 +75,18 @@ public function form(array $form, array &$form_state) {
+    // Add menu links administration form for existing menus.
+    if (!$menu->isNew() || isset($system_menus[$menu->id()])) {
+      // Form API supports constructing and validating self-contained sections
+      // within forms, but does not allow to handle the form section's submission
+      // equally separated yet. Therefore, we use a $form_state key to point to
+      // the parents of the form section.
+      // @see menu_overview_form_submit()
+      $form_state['menu_overview_form_parents'] = array('links');
+      $form['links'] = array();
+      $form['links'] = menu_overview_form($form['links'], $form_state);
+    }

This 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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -116,6 +116,15 @@ protected function actions(array $form, array &$form_state) {
   /**
+   * {@inheritdoc}
+   */
+  public function validate(array $form, array &$form_state) {
+    // The machine name is validated automatically, we only need to add the
+    // 'menu-' prefix here.
+    $form_state['values']['id'] = 'menu-' . $form_state['values']['id'];

@@ -129,15 +138,6 @@ public function languageConfigurationSubmit(array &$form, array &$form_state) {
   /**
-   * {@inheritdoc}
-   */
-  public function validate(array $form, array &$form_state) {
-    // The machine name is validated automatically, we only need to add the
-    // 'menu-' prefix here.
-    $form_state['values']['id'] = 'menu-' . $form_state['values']['id'];

I had accidentally put that function below a submit handler, which made very little sense...

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.php
@@ -64,59 +66,76 @@ function testMenuLanguage() {
+    $this->assertMenuLink($menu_link->id(), array('menu_name' => $menu_name, 'link_path' => $link_path, 'langcode' => 'bb'));

Ahh, I missed that: This should be broken into multiple lines IMO, as I did with the other ones.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
@@ -209,9 +210,33 @@ function doMenuTests($menu_name) {
+      // We assert the language code here to make sure that the language
+      // selection element degrades gracefully without Language module.
+      'langcode' => 'site_default',

Is this comment sufficiently clear?

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuWebTestBase.php
@@ -0,0 +1,48 @@
+ ¶

Nooo!!! Dammit...

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -45,7 +71,10 @@ public function __construct($operation, AliasManagerInterface $path_alias_manage
     return new static(
       $operation,
-      $container->get('path.alias_manager.cached')
+      $container->get('plugin.manager.entity')->getStorageController('menu_link'),
+      $container->get('path.alias_manager.cached'),
+      $container->get('url_generator'),
+      $container->get('module_handler')
     );

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.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,36 +184,30 @@ public function form(array $form, array &$form_state) {
+        $language_configuration = language_get_default_configuration('menu_link', $menu_link->bundle());
         $default_langcode = $language_configuration['langcode'];
+        $language_show = $language_configuration['language_show'];

language_get_default_configuration() always returns a properly filled array, so I removed the checks.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,36 +184,30 @@ public function form(array $form, array &$form_state) {
       else {
-        // We do not know the language of the menu.
-        $default_langcode = Language::LANGCODE_NOT_SPECIFIED;

Not relying on the existence of menu module, which we don't have to, allowed me to remove this condition.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,36 +184,30 @@ public function form(array $form, array &$form_state) {
-      '#access' => !is_null($language_configuration['language_show']) && $language_configuration['language_show'],
+      '#access' => $language_show,

I found using a separate variable more clear because with the new code flow $language_configuration is not always defined.

tstoeckler’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
@@ -156,11 +184,30 @@ public function form(array $form, array &$form_state) {
+    $language_show = FALSE;g

How the hell did that 'g' get there?

Sorry for the noise...

YesCT’s picture

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuLanguageTest.phpundefined
@@ -0,0 +1,151 @@
+  public static function getInfo() {
+    return array(
+      'name' => 'Menu language',
+      'description' => 'Create menu and menu links in non-English language, and edit language settings.',
+      'group' => 'Menu'
+    );

I wonder why no comma at the end of the last line here.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuWebTestBase.phpundefined
@@ -0,0 +1,48 @@
+ ¶

oops.

We can do those next time we re-do the patch.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -156,11 +184,30 @@ public function form(array $form, array &$form_state) {
+    $language_show = FALSE;g

here is the syntax error.
Oh I see you found that already. ;)

YesCT’s picture

Assigned: Unassigned » YesCT
Status: Needs review » Needs work

I'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)

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
2.24 KB
25.23 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 1945226-118-menu-language.patch, failed testing.

YesCT’s picture

FileSize
263.16 KB

we 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.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -117,10 +117,12 @@ protected function actions(array $form, array &$form_state) {
   /**
    * {@inheritdoc}
    */
-  public function validate(array $form, array &$form_state) {
+  public function validate(array $form, array &$form_state, $is_new = FALSE) {
     // The machine name is validated automatically, we only need to add the
-    // 'menu-' prefix here.
-    $form_state['values']['id'] = 'menu-' . $form_state['values']['id'];
+    // 'menu-' prefix here if menu was new.
+    if ($is_new) {
+      $form_state['values']['id'] = 'menu-' . $form_state['values']['id'];
+    }
   }

?

But how do I pass in the extra arg when submitting the form?

check this:

menu-menu-.png

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

YesCT’s picture

FileSize
3.8 KB

this is just silly.

    $this->drupalPost('admin/structure/menu/add', $edit, t('Save'));
+    // Menu machine names get prefixed with 'menu-'.
+    $menu_name = 'menu-' . $menu_name;
 
     // Check that the language settings were saved.
-    $this->assertEqual(entity_load('menu', $menu_name)->langcode, $edit['langcode']);
+    $menu = menu_load($menu_name);
+    $this->assertEqual($menu->langcode, $edit['langcode']);
     $language_settings = language_get_default_configuration('menu_link', $menu_name);
     $this->assertEqual($language_settings['langcode'], 'bb');
     $this->assertEqual($language_settings['language_show'], TRUE);
@@ -81,6 +84,10 @@ function testMenuLanguage() {
     // Test menu link language.
     $link_path = '<front>';
 
+    // Previous post ends with url using wrong menu-menu- pattern. Get the
+    // correct page.
+    $this->drupalGet("admin/structure/menu/manage/$menu_name");
+
     // Add a menu link.
     $link_title = $this->randomString();
Gábor Hojtsy’s picture

@tstoeckler, @kfritsche: can the two of you take over this issue? :)

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Status: Needs work » Needs review
FileSize
25.87 KB
2.63 KB

Here we go. Still haven't managed to get the tests to run in a reasonable time, so there you go, bot.

Status: Needs review » Needs work

The last submitted patch, 1945226-123-menu-language.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
25.44 KB

This should fix the exceptions. While editing a menu link the language wasn't changeable anymore.

YesCT’s picture

@kfritsche what was causing the exception?

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-125.patch, failed testing.

kfritsche’s picture

Okay 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.

YesCT’s picture

Assigned: tstoeckler » YesCT

@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.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -185,21 +185,18 @@ public function form(array $form, array &$form_state) {
+      $default_langcode = ($menu_link->isNew()) ? $menu_link->langcode : $language_configuration['langcode'];

if it's new, it doesn't have a langcode yet. that's when it needs to get the default from the config.

tstoeckler’s picture

No 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 :-)

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
2.6 KB
25.7 KB

this 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1945226-Add-language-selector-on-menus-130.patch, failed testing.

YesCT’s picture

FileSize
233.57 KB

I 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.

kfritsche’s picture

@YesCT:
Wow. Nice catch. Sorry for such a hidden error ;)

@tstoeckler:
From your patch #123

@@ -156,11 +184,30 @@ public function form(array $form, array &$form_state) {
       '#description' => t('Optional. In the menu, the heavier links will sink and the lighter links will be positioned nearer the top.'),
     );
 
+    // Set up defaults for menu links that already exist.
+    $default_langcode = $menu_link->langcode;
+    $language_show = FALSE;
+
+    if ($menu_link->isNew()) {

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.

kfritsche’s picture

Back 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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
25.66 KB

Here 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.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -D8MI, -sprint, -language-config, -medium

The last submitted patch, 1945226-136-menu-language.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

#136: 1945226-136-menu-language.patch queued for re-testing.

EDIT: Please Mr. Bot, please tell me that was a random failure.

Status: Needs review » Needs work

The last submitted patch, 1945226-136-menu-language.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +D8MI, +sprint, +language-config, +medium

#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...)

Kristen Pol’s picture

Green!!!

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 :)

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community

Very 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.

YesCT’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs manual testing
FileSize
3.08 KB
26.67 KB

this 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@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:

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -7,18 +7,50 @@
+  /**
+   * Constructs a new EntityListController object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler to invoke hooks on.
+   */
+  public function __construct($operation, ModuleHandlerInterface $module_handler) {

Operation not documented.

helenkim’s picture

Status: Needs work » Needs review
FileSize
26.74 KB
652 bytes

I copied the documentation on ViewEditFormController.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great assuming #143/#145 would pass. They looked to have great changes on code review.

alexpott’s picture

Title: Add language selector on menus » Change notice: Add language selector on menus
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 3a89eb5 and pushed to 8.x. Thanks!

Probably need to update an existing change notice.

andypost’s picture

Yay! Now we can adopt same kind of code for #111715: Convert node/content types into configuration

YesCT’s picture

YesCT’s picture

Assigned: Unassigned » YesCT

I'm writing the change notice for this.

tstoeckler’s picture

Assigned: YesCT » Unassigned
Status: Active » Needs review

I added https://drupal.org/node/2027579. Can someone fix the links there, plz? Drupal.org hates me again, today...

tstoeckler’s picture

Oops, that was a cross-post. Hmm..

Gábor Hojtsy’s picture

Title: Change notice: Add language selector on menus » Add language selector on menus
Priority: Critical » Normal
Status: Needs review » Fixed

I 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.

YesCT’s picture

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

Anonymous’s picture

Issue summary: View changes

added related issue for bundle always tools