Add the option to add custom menus to a content type on menu creating/editing

Don't we just have checkboxes? I could see that work here too.

I tested the patch locally, and it functions as expected:

• There are checkboxes for content types on the menu edit form that correspond to the checkboxes for menus on the content type edit form.
• Checking the box on the menu for a content type enables it for the menu on the content type form, and vice versa.
• Either method makes the menu available on the node edit form.

+++ b/modules/menu/menu.admin.incundefined
@@ -465,6 +465,23 @@
+    '#description' => t('some description'),

Obviously, some user instructions would go here. :)

+++ b/modules/menu/menu.admin.incundefined
@@ -593,6 +610,21 @@
+    $content_types = $menu['menu_content_types'];
+    foreach (node_type_get_names() as $content_type => $name) {
+      $menu_options = variable_get('menu_options_' . $content_type, array('main-menu'));

Instead of node_type_get_names(), can we just use the form definition here? Edit: Just during submission, I mean.

+++ b/modules/menu/menu.admin.incundefined
@@ -593,6 +610,21 @@
+        // remove this menu from the content type

Needs capitalization and a period.

+++ b/modules/menu/menu.admin.incundefined
@@ -593,6 +610,21 @@
+        // add this menu to the content type

Ditto.

+++ b/modules/menu/menu.admin.incundefined
@@ -593,6 +610,21 @@
+      // save the new menu options

I'd say either remove this comment, or make it more descriptive.

The other way around: Node has to integrate with Menu.

Small edits

+++ b/modules/menu/menu.admin.inc
@@ -465,6 +465,22 @@
+  foreach (node_type_get_names() as $content_type => $name) {

1) No need to check this on menu creation.

2) Could use an inline comment that explains what is being done, as the operation is a bit mind-boggling.

+++ b/modules/menu/menu.admin.inc
@@ -465,6 +465,22 @@
+    if(in_array($menu['menu_name'], $menu_options)) {

Missing space after if.

+++ b/modules/menu/menu.admin.inc
@@ -465,6 +465,22 @@
+  $form['menu_content_types'] = array(

Let's name this 'menu_node_types'.

+++ b/modules/menu/menu.admin.inc
@@ -465,6 +465,22 @@
+    '#title' => t('Available content types'),
...
+    '#description' => t('The content types available that can use this menu.'),

Replace #title with #description and drop the #description.

+++ b/modules/menu/menu.admin.inc
@@ -465,6 +465,22 @@
+    '#options' => node_type_get_names(),

No need to call this twice.

+++ b/modules/menu/menu.admin.inc
@@ -594,6 +610,21 @@
+  $content_types = $menu['menu_content_types'];
+  foreach ($content_types as $content_type => $value) {

Like above, this operation is equally not "obvious", so could use an inline comment explaining what is being done.

+++ b/modules/menu/menu.admin.inc
@@ -594,6 +610,21 @@
+    if (empty($value) && in_array($menu['menu_name'], $menu_options)) {

This in_array() looks superfluous to me.

Lastly, we need tests for this feature.

25 days to next Drupal core point release.

The patch looks good to me, and this seems like a useful parity to have in the UI.

+++ b/core/modules/menu/menu.admin.incundefined
@@ -466,6 +466,26 @@
+    '#title' => t('The content types available that can use this menu.'),

The UI text here is a little weird. Not sure what to change it to, at least not before my coffee. ;)

+++ b/core/modules/menu/menu.testundefined
@@ -157,12 +159,23 @@
+    // Verify article has this menu enabled
...
+    // Verify we can remove the menu again from the node type

I'd suggest changing this to "Verify this menu is listed on the article administration form." and "Disable the menu from the article administration page." (With periods.)

Untagging because the feature has tests.

+++ b/core/modules/menu/menu.admin.inc
@@ -466,6 +466,26 @@
+  $node_types = node_type_get_names();

This entire code needs to be wrapped in a module_exists('node').

Especially due to that, I do not think this feature belongs into Menu module, but instead, Node module should integrate with Menu module.

Both modules are optional. But the actual configuration is primarily used on the node form.

I understand that menu.module already contains integration code in the other direction, but that looks equally bogus to me, now that Node module is optional. At the very least, I'd like to see a follow-up issue to move all of the integration code from Menu to Node module.

+++ b/core/modules/menu/menu.admin.inc
@@ -466,6 +466,26 @@
+  if ($type == 'edit') {
+    // When editing a menu we need to collect all the node types that have this
+    // menu enabled.

We need to explain _why_ the add form case is ignored.

+++ b/core/modules/menu/menu.admin.inc
@@ -466,6 +466,26 @@
+    '#title' => t('Make this menu available to the following content types:'),

1) No semicolon at the end.

2) Could be shortened.

+++ b/core/modules/menu/menu.admin.inc
@@ -466,6 +466,26 @@
@@ -595,6 +615,22 @@

Please double-check your git configuration to create patches with -p to provide more context in diff hunks. (There should be a menu_yadayada_form_submit() context reference on this line.)

+++ b/core/modules/menu/menu.admin.inc
@@ -595,6 +615,22 @@
+  // Check if we need to add or remove this menu from each available node type.

Drop the first part "Check if we need to" without replacement.

Suggestion for the label: "Make this menu available for these content types"

Or even "Available for these content types" you are already in the activity, of "creating this menu".

The general concept is sound, I remember this being quite awkward when we designed it - as it was a one-way solution.

I'll take this one.

+++ b/core/modules/menu/menu.admin.incundefined
@@ -476,6 +476,29 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
+  if module_exists('node') {

This needs parens around the condition.

+++ b/core/modules/menu/menu.testundefined
@@ -133,10 +133,12 @@ class MenuTestCase extends DrupalWebTestCase {
+      "menu_node_types[$type]" => TRUE,

@@ -159,12 +161,23 @@ class MenuTestCase extends DrupalWebTestCase {
+    $edit['menu_node_types[' . $type . ']'] = FALSE;

Really nitpicky, but can we be consistent between these two?

+++ b/core/modules/menu/menu.testundefined
@@ -159,12 +161,23 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertFieldChecked('edit-menu-node-types-' . $type, t('Custom menu added to article node type'));
...
+    $this->assertNoFieldChecked('edit-menu-node-types-' . $type , t('Custom menu removed from article node type'));

Let's strip the t() from these assertion messages. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

patch updated and resubmitted for test

Thanks @trrroy! Setting to "needs review" so the patch can be tested by the testbot and reviewed by reviewers.

So, unrelated. Neither aspilicious nor I can remember why we decided with sun that menu should integrate with node rather than vice versa. aspilicious thinks it might have had to do with not being able to get the correct $menu array in node. (We really should have documented it...) As far as aspilicious recalls, the three of us came to the conclusion that it had to be this way, and sun mentioned a followup issue to decouple menu from node. aspilicious didn't know if that issue actually exists.

Tagging for a summary update to straighten this out, and also for research to find a followup issue for further decoupling node and menu.

+++ b/core/modules/menu/menu.test
@@ -133,10 +133,12 @@ class MenuTestCase extends DrupalWebTestCase {
+      'menu_node_types[$type]' => TRUE,

@@ -159,12 +161,23 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertFieldChecked('edit-menu-node-types-' . $type, 'Custom menu added to article node type');
...
+    $edit['menu_node_types[$type]'] = FALSE;
...
+    $this->assertNoFieldChecked('edit-menu-node-types-' . $type , 'Custom menu removed from article node type');

The key needs to be in double-quotes, as it contains a $variable you want to be expanded.

The variable expansion is the reason why double-quotes are slightly slower, and thus, why we generally use single quotes everywhere, unless double quotes are needed.

That said, the surrounding code in this test uses single quotes with string concatenation of $type. Beautiful code is consistent, so please decide on one way :) (I think I'd personally go with double quotes here)

No, really.

Changing to double quotes.

I'm fine with this patch. Only code comments could use some love - the added code in the form has no explanation at all for what is being done there - and why.

Likewise, the code in the form submit handler accesses a lot of variables without checking whether they actually exist. That said, the test code should be moved into an own test case, and ideally, the test case verify the correct behavior with both Node module being disabled and enabled.

Lastly, there's a very minor coding style error in the test (missing space in array declaration).

I added the comments to form additions and fixed the missing space in array declaration. Still need to check for variables in the submit handler and split out the test case (from comment in 31). I'm just getting my feet wet with test cases, please bear with me.

ok, variables are checked and a new test case is broken out with the node module disabled. I'm getting some warnings from simpletest about "call_user_func_array() expects parameter 1 to be a valid callback, function '_node_add_access' not found or invalid function name" when trying to access 'admin/structure/menu/manage/$menu_name/add'. I'm not sure how to get rid of those.

Thanks @trrroy! I'm working with irunflower during office hours this morning, and she's going to add some code style cleanups to your patch as follows:

1. +++ b/core/includes/menu.incundefined
   @@ -508,7 +508,13 @@ function menu_execute_active_handler($path = NULL, $deliver = TRUE) {
   +        } ¶
   
   +++ b/core/modules/menu/menu.admin.incundefined
   @@ -476,6 +476,32 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
   +  // Add the option to add custom menus to a content type on menu ¶
   
   +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +584,127 @@ class MenuTestCase extends WebTestBase {
   +    module_disable(array('node'));
   +    ¶
   +    // Login the user.
   ...
   +    $this->items = array();
   +    ¶
   ...
   +    $this->items[] = $item2;
   +  }  ¶
   

   Trailing whitespace on all these lines should be removed.

2. +++ b/core/includes/menu.incundefined
   @@ -627,7 +633,8 @@ function _menu_check_access(&$item, $map) {
   +      // if callback function does not exist, do not set $item['access'] to true
   

   TRUE should be all in caps, the sentence should begin with a capital and end with a period, and there should be articles (e.g. "if the callback function does not exist"). Also, be sure to wrap this comment to 80 chars when it's updated.

3. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -476,6 +476,32 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
   +  // creating/editing
   

   I'd say "when creating or updating a menu." Also, the comment needs to end in a period.

4. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -476,6 +476,32 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
   +    // add checkboxes to the form for each content type
   

   Needs capitalization and a period.

5. +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +584,127 @@ class MenuTestCase extends WebTestBase {
   +   * Login users, add menus and menu links, and test menu functionality through the admin and user interfaces.
   

   This needs to be 80 characters or less and begin with a third-person verb. Also, can we clarify a little what the test is testing?

6. +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +584,127 @@ class MenuTestCase extends WebTestBase {
   +    // No standard user tests because there are no accessible menu links to $std_user.
   

   This comment is over 80 characters long and should be wrapped earlier.

7. +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +584,127 @@ class MenuTestCase extends WebTestBase {
   +  /**
   +   * Test standard menu functionality using navigation menu.
   +   *
   +   */
   ...
   +   * Test custom menu functionality using navigation menu.
   +   *
   +   */
   ...
   +  /**
   +   * Test menu functionality using navigation menu.
   +   *
   +   */
   

   These docblocks all have superfluous blank lines after the summary, and should begin with a third-person verb (e.g. "Tests"). Also, the docs should be a little more specific (describing what the tests are doing).

8. +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +584,127 @@ class MenuTestCase extends WebTestBase {
   +    // Note in the UI the 'mlid:x[hidden]' form element maps to enabled, or
   +    // NOT hidden.
   

   Can we clarify this comment more?

When submitting a new patch, please include an interdiff so @trrroy can easily see the changes we're making.

Updated.

In #36, comment #1, the last part of trailing white space - couldn't find trailing white space in:

+    module_disable(array('node'));
+    ¶
+    // Login the user.
...
+    $this->items = array();
+    ¶

In #6, comments #7 and #8 still have to be addressed.

Interdiff made. First time making an interdiff, but I checked the comparisons between the patches, and I think I made this correctly...

Thanks @irunflower! I'm new to interdiffs too so I'm not certain but I don't think that #38 had all your changes in it. I pulled from 37, checked whitespaces and completed 6/7/8 from #36. I'm pretty sure its all in here so far.

oops, I found more whitespace. try this again.

This is looking a lot better! Thanks @irunflower and @trrroy.

I've reviewed the latest patch and noticed the following:

1. +++ b/core/includes/menu.incundefined
   @@ -508,7 +508,13 @@ function menu_execute_active_handler($path = NULL, $deliver = TRUE) {
   +          debug($router_item['page_callback'] . ' function does not exist.[1]');
   

   Uh. Stray debug.

2. +++ b/core/includes/menu.incundefined
   @@ -627,7 +633,9 @@ function _menu_check_access(&$item, $map) {
   +      // If the callback function does not exist, do not set $item['access'] to
   +      // TRUE.
   

   An explanation of "why" would be good in this comment.

3. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -476,6 +476,32 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
   +  // Add the option to add custom menus to a content type when creating or
   +  // updating a menu.
   ...
   +      // When editing a menu we need to collect all the node types that have
   +      // this menu enabled.
   
   +++ b/core/modules/menu/menu.testundefined
   @@ -580,6 +583,129 @@ class MenuTestCase extends WebTestBase {
   +    // No standard user tests because there are no accessible menu links to
   +    // $std_user.
   

   I think some of comments are wrapping too early? They should go as close to 80 char as possible without going over. Can you double-check?

4. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -476,6 +476,32 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
   +  if (module_exists('node')) {
   

   I may have said this earlier--while this is gross, decoupling menu from node is a significant task and beyond the scope of this issue, so this is acceptable in that sense.

5. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -580,6 +606,11 @@ function menu_edit_menu_name_exists($value) {
   +  // Abort if menu values do not exist
   

   Missing period. Also, there should be articles (e.g., "Abort if the menu values do not exist."

6. +++ b/core/modules/menu/menu.admin.incundefined
   @@ -580,6 +606,11 @@ function menu_edit_menu_name_exists($value) {
   +    drupal_set_message(t('There was a problem. Your configuration was not saved.'));
   

   This is a fairly useless error message...

7. +++ b/core/modules/menu/menu.testundefined
   @@ -31,7 +31,8 @@ class MenuTestCase extends WebTestBase {
   -   * Login users, add menus and menu links, and test menu functionality through the admin and user interfaces.
   +   * Logs in users, adds menus and menu links, and tests menu functionality
   +   * through the admin and user interfaces.
   
   @@ -580,6 +583,129 @@ class MenuTestCase extends WebTestBase {
   +   * With node module disabled, this test logs in users, adds menus and menu
   +   * links, and tests menu functionality through the admin and user interfaces.
   

   These docblocks should begin with a one-line summary that is 80 characters or less and begins with a third-person verb. Reference: http://drupal.org/node/1354#functions

8. +++ b/core/modules/menu/menu.testundefined
   @@ -49,7 +50,8 @@ class MenuTestCase extends WebTestBase {
   -      $node = node_load(substr($item['link_path'], 5)); // Paths were set as 'node/$nid'.
   +      // Paths were set as 'node/$nid'.
   +      $node = node_load(substr($item['link_path'], 5));
          $this->verifyMenuLink($item, $node);
   
   @@ -80,7 +82,6 @@ class MenuTestCase extends WebTestBase {
       * Test standard menu functionality using navigation menu.
   -   *
   
   @@ -89,7 +90,6 @@ class MenuTestCase extends WebTestBase {
      /**
       * Test custom menu functionality using navigation menu.
   -   *
       */
   
   @@ -142,7 +140,8 @@ class MenuTestCase extends WebTestBase {
   -    // Verify that using a menu_name that is too long results in a validation message.
   +    // Verify that using a menu_name that is too long results in a validation
   +    // message.
   
   @@ -197,7 +196,6 @@ class MenuTestCase extends WebTestBase {
      /**
       * Test menu functionality using navigation menu.
   -   *
       */
   
   @@ -521,7 +523,8 @@ class MenuTestCase extends WebTestBase {
   +    // Retrieve menu link id of the Log out menu link, which will always be on
   +    // the front page.
   
   @@ -679,7 +805,8 @@ class MenuNodeTestCase extends WebTestBase {
   -    // Assert that disabled Management menu is not shown on the node/$nid/edit page.
   +    // Assert that disabled Management menu is not shown on the node/$nid/edit
   +    // page.
   
   @@ -701,7 +828,8 @@ class MenuNodeTestCase extends WebTestBase {
   -    // Assert that it is not possible to set the parent of the first node to itself or the second node.
   +    // Assert that it is not possible to set the parent of the first node to
   +    // itself or the second node.
   

   These changes, while (in most cases) correct by our coding standards, are out of scope for the issue. When code style errors exist elsewhere in core, we should fix them in followup issues (e.g. #1326672: Clean up API docs for menu module). In this patch, we should only make sure that new or changed lines comply with the standards, and not change other lines. See: http://xjm.drupalgardens.com/blog/core-mentoring-and-xjms-guide-patch-re...

9. +++ b/core/modules/menu/menu.testundefined
   @@ -246,8 +244,8 @@ class MenuTestCase extends WebTestBase {
   +    // Test a hidden menu link. Note in the UI the 'mlid:x[hidden]' form element
   +    // maps to enabled, or NOT hidden.
   
   @@ -580,6 +583,129 @@ class MenuTestCase extends WebTestBase {
   +    // Test a hidden menu link. Note in the UI the 'mlid:x[hidden]' form element
   +    // maps to enabled, or NOT hidden.
   

   This comment could be clarified. "Maps to"?

10. +++ b/core/modules/menu/menu.testundefined
    @@ -340,9 +338,9 @@ class MenuTestCase extends WebTestBase {
    -  function verifyMenuLink($item, $item_node, $parent = NULL, $parent_node = NULL) {
    +  function verifyMenuLink($item, $item_node = NULL, $parent = NULL, $parent_node = NULL) {
    

    This API change also needs to include an update to the function's docblock to indicate that $item_node is optional.

11. +++ b/core/modules/menu/menu.testundefined
    @@ -352,9 +350,11 @@ class MenuTestCase extends WebTestBase {
    +        $this->assertTitle(t("@title | Drupal", array('@title' => $title)), t('Parent menu link link target was correct'));
    
    @@ -362,9 +362,11 @@ class MenuTestCase extends WebTestBase {
    +      $this->assertTitle(t("@title | Drupal", array('@title' => $title)), t('Menu link link target was correct'));
    

    We can omit t() from the assertion message here (the final argument). Reference: http://drupal.org/simpletest-tutorial-drupal7#t

    Also, let's add a period to it.

12. +++ b/core/modules/menu/menu.testundefined
    @@ -580,6 +583,129 @@ class MenuTestCase extends WebTestBase {
    +  /**
    +   * Test standard menu functionality using navigation menu.
    ...
    +   */
    ...
    +  /**
    +   * Test custom menu functionality using navigation menu.
    +   *
    +   */
    ...
    +  /**
    +   * Test menu functionality using navigation menu.
    +   *
    +   */
    

    These should begin with "Tests" rather than "Test". Also, these are the extra blank lines that should be removed, because they are lines we are adding in this patch.

When creating a patch, please read the patch file before uploading to make sure it includes the intended changes. Thanks!

All comments from 43 addressed. On #9, I'm not sure if my change answers the question (also, I'm not sure if I understand the code/comment). On #10, should that docblock be re-formatted with descriptions below the parameters?

(arg! there was whitespace in 44! sorry! I thought I checked that.) Please check my questions in comment #44.

I'm getting some warnings from simpletest about "call_user_func_array() expects parameter 1 to be a valid callback, function '_node_add_access' not found or invalid function name" when trying to access 'admin/structure/menu/manage/$menu_name/add'. I'm not sure how to get rid of those.

heh. You got rid of those by hacking menu.inc? LOL! But hey, at least I like your creativity :)

All patches since #35 are carrying on this hack, which obviously needs to be removed.

Also, the additional test methods in MenuTestCase have to be moved into an own, new test case class (in case there is no test case for testing the node/menu associations already; I suspect there's probably one in node.test already).

Ok, I removed my hacks and looked for an existing test for node/menu associations but didn't see one. I did move the new MenuNodeDisabledTest into its own test case but I'm back to the same error that I had before that I'm not sure how to get rid of "call_user_func_array() expects parameter 1 to be a valid callback, function '_node_add_access' not found or invalid function name." Any suggestions?

Fixed mistake

(Merging "node system" and "node.module" components for 8.x; disregard.)

A lot of work was put into this one, but its seems like such a long time. Adding tag Needs reroll.

Rerolled the patch from Comment #49

Please don't remove tags.

Working on this issue as a part of Drupal Con Asia 2016 code sprint.

I tried applying patch provided in #55 on latest 8.0.x version. The patch gets applied. However, it doesn't work as expected. No content type links provided while creating or editing menu.

I applied patch #55 in my local environment but it does not make any UI changes !

I'm at DrupalCon Nashville sprints and I'm going to start looking into this issue!

Tagging this as Needs Reroll again because the reroll in #55 was not sufficient. It replicates the menu.admin.inc file from Drupal 7.x, but the callbacks in that file are no longer present in Drupal 8.x. That's why applying the patch has no effect. Also, since the Menu core module does not exist in Drupal 8.x, #55 would require that module to be installed, which is not possible because there is no .info.yml file.

The patch needs to be rerolled in such a way that it fits into the way menus are managed in Drupal 8.x Core.

I've redone the main part of the patch #49 meaning out of tests according to way menus are managed in Drupal 8.x Core
That's why I didn't make interdiff. I'm not good at creating unit tests, so would be great if someone more experienced will take care of it .

The last patch was rerolled successfully.
Solved 2 conflicts in
core/modules/menu_ui/src/MenuForm.php b/core/modules/menu_ui/src/MenuForm.php

The patch still applies

A re-rolled patch against 9.3.x. Please review.

Hi @Suresh Prabhu Parkala,

Above test cases, #83 is getting failed.
Can you please check it once again and attached some before and after screenshots.

Needs to be re-rolled for 9.3.x
Can be a move to Needs work

Fixed custom command failure errors in #83, thanks!

I have applied patch #85 in durpal-9.3.x-dev after patch we can configure menu for specific content type for ref sharing screenshot

Patch No. #85 tested and applied successfully on drupal 9.4.x version.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The changes here are for menu_ui so updating the component.

So this issue more or less went dormant in 2016 then kinda just got rerolled but is this a feature that's really needed or could it be a contrib module?

Since it's been 3 months and no follow up don't think this is a feature that needs to be in core. Looking at it again and seems like a big edge case vs 80% edge case.

Spoke to the other sub-maintainer darvanen and we both agreed.

We did start up https://www.drupal.org/project/menu_ui_extras to hold these features we are seeing for menu_ui that don't perfectly fit core.