conflict with Menu Settings Per Content Type module

Also get the same problem with the module Menuless Node Type

Just hit this one myself after installing ctm too.

Questions for the developer to consider in the comments below:

<?php

/**
 * Implementation of hook_form_alter().
 */
function submenutree_form_alter(&$form, $form_state, $form_id) {
  // Changed to a more standard node form check.
  if (isset($form['#node']) && $form['#node']->type .'_node_form' == $form_id
        // Do you want to enforce the permissions? Probably overkill.
        && user_access('administer menu')
        // Do you want to continue if other modules have removed $form['menu']
        && isset($form['menu'])) {
    $node = $form['#node'];
    // Inject some sane defaults
    if (empty($node->submenutree_enable)) {
      $node->submenutree_enable = 0;
      $node->submenutree_display = SUBMENUTREE_DISPLAY_MENU;
      $node->submenutree_weight = 1;
    }
    if (empty($node->siblingmenutree_enable)) {
      $node->siblingmenutree_enable = 0;
      $node->siblingmenutree_display = SUBMENUTREE_DISPLAY_MENU;
      $node->siblingmenutree_weight = 1;
    }
    // Other modules may disable the menu, so clone the menu modules' fieldset.
    // See the questions above to see if this is needed.
    if (!isset($form['menu'])) {
      $form['menu'] = array(
        '#type' => 'fieldset',
        '#title' => t('Menu settings'),
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
        '#weight' => -2,
        '#attributes' => array('class' => 'menu-item-form'),
      );
    }

    // ....
  }
}
?>

The attached patch is for the negative - no fieldset then ignore.

Cheers

Alan

Hi,

Thanks for your information. Not having used the Menu Settings Per Content Type module myself, I didn't really look into this issue.

I've generated a prospective patch. Compared with your code snippet ...

I think user_access('administer menu') is redundant. That is already tested for by menu_form_alter(), which is the code that injects $form['menu'].

Later in your code snippet ... why do you have that code stanza that recreates $form['menu']? Is there something I should know about?

My first patch had a similar thing. The code chunk pasted in was a combo of different options, sorry if it was confusing.

The big difference is the check on the form being processed. The check on the node type in the first patch is consistent with the core menu hook_form_alter(). The question is why core does a more complete check?

eg:

<?php

/**
 * Implementation of hook_form_alter(). Adds menu item fields to the node form.
 */
function menu_form_alter(&$form, $form_state, $form_id) {
  if (isset($form['#node']) && $form['#node']->type .'_node_form' == $form_id) {
    // Note - doing this to make sure the delete checkbox stays in the form.
    $form['#cache'] = TRUE;
    ....
  }
}
?>

Personally I do not know the answer, but trusting the #id property will fail if another form registers itself with the id 'node-type' first.

Eg: http://drupal.org/files/issues/submenutree-349143-4-no-menu-fieldset-che...

-  if ($form['#id'] == 'node-form') {
+  if (isset($form['#node']) && $form['#node']->type .'_node_form' == $form_id
+        // Check that the menu fieldset still exists.
+        && isset($form['menu'])) {

compared with http://drupal.org/files/issues/submenutree-349143-patch-1.txt

-  if ($form['#id'] == 'node-form') {
+  if ($form['#id'] == 'node-form' && isset($form['menu'])) {

Ah, I misread your first patch. Now I see you've used the same conditions as menu_form_alter().

The 'check on the form being processed' for both patches are more or less equivalent. I don't expect there to be a site out there which fails one but passes the other (and trying to predict this far is splitting straws too much, IMHO). Personally, I prefer the check against $form['#id'] because it is cleaner and because it is already in use.

> but trusting the #id property will fail if another form registers itself with the id 'node-type' first.

I think if another module registers a form with the id 'node-form', then large portions of node.module will break, so this is not likely to occur for a stable site.

So, overall, if my patch works for you and if you have no other objections, I'd go with it.

All good. I just had some concerns about a non-standard approach. But now this has two checks, #id and menu, so there should be very minimal risk. :)

Committed to cvs.

This fix will be incorporated into the next Submenutree release, whenever that is.

In the meanwhile, please patch your sites according to:

http://drupalcode.org/viewvc/drupal/contributions/modules/submenutree/su...

Thank you.