Default menus have hardcoded titles which cannot be edited

Looks good to me.

Related: #1079010: The secondary links heading, "Secondary menu", is wrong

Not much can be salvaged from this patch unfortunately.

Still looks like the menu names are greyed out places like admin/structure/menu/manage/tools/edit

I don't think the title change applies anymore due to #1926736: Allow block titles to be automatically tied to the underlying object, so most of what would be left here would be the removal of the #access and test updates.

Tagging up.

Took the spirit of this patch and updated it to the current codebase.

1. Make the label editable.

2. Expose the edited label in the system menu block (template. Of course when the block is instantiated, in Drupal 8, the block title is initialised with this title, but then disconnected from the menu. So with the standard profile, where most if not all of the system blocks are already placed, the title change of the menu will not have any effect on the exposed block. That is not to be solved here though, it should be in #1926736: Allow block titles to be automatically tied to the underlying object.

3. Extended tests to make sure we can change a built-in menu's title and it would expose the block with the new title on the blocks admin page. From there the test needs to assume blocks work like blocks, and inherit the admin label by default. Tests for that in the block system.

4. I also found a tiny bug in the menu code where the menu form would throw notices if there are no items. The test saving an empty menu exposed that, so fixed that one line as well (is_array() in menu.admin.inc). Here is a patch without that fix too to prove it exposes that bug.

Tag removal was definitely not intended.

The visual change of the patch is as follows:

BEFORE:

AFTER:

Looks good to me.

Installation failure is most likely caused by loading menu entities too early during the installer.

I'm also a bit concerned that there might be larger consequences of making system menu labels editable. Did anyone double-check history to find out why exactly the labels are not editable currently?

The first and foremost reason I can think of would be that these labels were formerly translatable t() strings and did not live in configuration. In Drupal 8, the label should always come from the menu's configuration object though.

@sun: no I don't know why it is/was non-editable. Looks like setting the blocks up in the installer might be one reason :D Any ideas for a good fix for that?

Well, custom menu labels are not translatable either as-is. Also Drupal 8 does not seem to t() the menu labels in http://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/men... either.

whoooo! This would definitely solve a hack we are trying to do now, to avoid having uneditable fields.

Yeah I'd assume it would do t() on the built-in labels, until they are modified. I'm not sure I understand why we have these menus hardcoded in the first place. We can have them as default config (and translated via config) and we could save lots of specialty hassle.

With the patch, indeed in the installer we get "The plugin (system_menu_block:menu-footer) did not specify an instance class." errors, which turns out to be due to the naming mismatch that the comment alluded to in the code. I mistakenly modified it earlier so the menu derivative blocks had wrong ids. Now fixed by using more of the existing code.

I can install with this new one. Hopefully it will not fail for anything else but my intentional fail patch :)

re: #17: @Gábor Hojtsy:

I absolutely agree. I'd actually like to go one step further and remove the entire "system menu" drama. Instead:

KISS: You want menus? Enable Menu module.

If Menu module is not enabled, then the only reference to menu names would be in the {menu_links} table. That reference to menu names would inherently turn into names of "containers" that the links are bundled into (i.e., just an abstract relationship).

Without Menu module (or an equivalent contributed module), a site would simply not have and not output any menu blocks. Is that a valid use-case? Sure, why not? I've actually built quite a few Drupal sites that do not have Menu module enabled and which do not output any menus.

This direction occurred to me while working on #1869476: Convert global menus (primary links, secondary links) into blocks. It should be possible to do relatively easily.

Erm, I'd concentrate first on solving the UX WTF and the larger picture ideas can still be solved elsewhere/later. I'd love to keep scope!

Looking at the fails Fatal error: Call to a member function label() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php on line 50 at all the places indicates the config object is not really there all the time. We should gracefully degrade in this case to the system known menu title.

Hopefully now only the supposed fail patch will fail :)

Now that it works as intended, reviews? :)

I can't find any problems with #21. Except for maybe the change that makes the test pass:

+++ b/core/modules/menu/menu.admin.incundefined
@@ -221,7 +221,7 @@ function menu_overview_form_submit($complete_form, &$form_state) {
-  $order = array_flip(array_keys($input));
+  $order = is_array($input) ? array_flip(array_keys($input)) : array();

But then again, menu_overview_form_submit() is already full of pretty advanced things (I definitely don't understand what's happening there within 1 minute).

#20: This issue is about making hardcoded menu titles editable. You're proposing to disable menu module by default. That's quite a big difference in scope.

@Wim: yes, that code is failing if the menu has no items. There were no test before for that, but this test deal with a menu without items. So that is why it fails. Although strictly speaking unrelated to this issue, I could have picked a menu with items, I choose to fix this tiny bug while I was at it anyway.

Yes, #20 was out of scope. Please understand that it is important to have an idea of a larger vision and direction.

#17 was in direct relationship to the scope of this issue, questioning a smaller detail of the current architecture. #20 tried to clarify that, indeed, some much larger aspects of the current architecture are not exactly sensible, which in turn triggers questions like #17, and in turn duct-tape fixes like the proposed patch.

The proposed patch looks good to go for me. The code of the SystemMenuBlock vs. MenuBlock plugins becomes almost identical with this, which means that something is clearly wrong.

Huh. Interesting. For some reason I thought this was verboten. Nice to remove weirdness, and also expand the test coverage. :)

Unfortunately, this seems to no longer apply. ;( Feel free to mark back to RTBC after a re-roll.

Er. :)

@webchick: Can you elaborate on why you thought it was verboten? It's possible that we're missing unintended consequences here.

Nothing's really ringing any specific bells, no. I did a quick run through Git blame which points to various work with machine names in D7. Might've been an introduced during an interim state of those patches.

Sorry, yeah, that question was just a safety-net. Let's re-roll this and move on :)

But apparently, no idea what's going on here... Patch applies perfectly fine for me. Here's a safety re-roll.

It did not apply due to the subject => admin_label change that @webchick likely did not push yet at that point. Direct reroll.

@VVVi: you did not apply the subject => admin_label change. That should be correct in my patch in #36.

I tested the patch in #36 with SimplyTest.me and agreed that it looks good to go.

Committed and pushed to 8.x. Thanks!

Landed.

People, I'm not getting why all patches for drupal 8 while the issue is related to drupal 7 as reported?