Overview of menu items in menu

Starting on review.

Review:

Passing arguments to form

function paddle_menu_manager_menu_overview_form($form, &$form_state) {
...
  $lang = paddle_menu_manager_load_language($form_state['build_info']['args'][1]);
...
}

Do not retrieve your form arguments from the 'args' in the form state, but pass them as function arguments, like this:

function paddle_menu_manager_menu_overview_form($form, &$form_state, $menu, $langcode) {
...
  $lang = paddle_menu_manager_load_language($langcode);
...
}

Dummy menu item

function paddle_menu_manager_menu() {
  $items = array();
...
  // Add menu router item for use as target with dummy menu items.
  $items['menu-dummy'] = array(
    'title' => 'Placeholder for menu item dummies.',
    'page callback' => 'paddle_menu_manager_menu_dummy',
    'access arguments' => array('manage paddle_menu_manager'),
    'type' => MENU_CALLBACK,
  );

  return $items;
}

• You can use 'drupal_not_found' directly as the page callback, it is not necessary to create a wrapper page callback function.
• The access arguments should be set to TRUE. The content editors may decide to enable certain dummy links which makes them visible in the front end for anonymous users. So this page should be accessible to anonymous users, so they get a 404 instead of a 403.
• I would change this path to 'menu-placeholder', these links might be output in the frontend and 'dummy' might have negative connotations.

Trailing spaces in action links

function paddle_menu_manager_menu_overview_form($form, &$form_state) {
...
    $form['items'][$item->mlid]['actions']['delete'] = array(
      '#type' => 'link',
      '#title' => t('delete') . ' ',
      '#href' => 'admin/structure/menu/manage/' . $item->mlid . '/delete',
    );

    $form['items'][$item->mlid]['actions']['edit'] = array(
      '#type' => 'link',
      '#title' => t('edit') . ' ',
      '#href' => 'admin/structure/menu/manage/' . $item->mlid . '/edit',
    );
...
}

Don't append spaces to the action link titles. If you need spacing between these links this can be done by either formatting them as table cells, or as an unordered list.

• Example of action links in table cells: _menu_overview_tree_form() in combination with theme_menu_overview_form().
• Example of action links in an unordered list: node_admin_nodes().

Form submit not tested

The actual submitting of the form is not covered by the test suite. Try to add at least 2 items to a menu and changing the order, save, reload the form to see if the order is preserved.

Reviewing.

The form submit test is still missing, and I found one minor issue. For the reset everything is looking good. I will fix these problems myself and then merge the branch, leaving this assigned to me for the moment.

Passing arguments to form

function paddle_menu_manager_menu_overview_form($form, &$form_state, $menu, $langcode) {
...
    $form['#empty_text'] = t('There are no menu items yet. <a href="@link">Add link</a>.', array('@link' => url('admin/structure/menu/manage/' . $form_state['build_info']['args'][0]['menu_name'] . '/add')));
...
}

There is one instance left where the passed-in variables are not used.

While writing the test I have encountered this notice:

Notice: Trying to get property of non-object in paddle_menu_manager_menu_overview_form() (line 72 of /home/pieter/v/kanooh/drupal/sites/all/modules/paddle_menu_manager/paddle_menu_manager.admin.inc).

Fixed remarks from #9 and #10, and expanded the test coverage, but the test has discovered a problem in the sorting of the menus: when two menus are added at the same depth and their order reversed, the menus pop back to their original place when the form is submitted. The weights have been set correctly in the database, they just appear in the wrong order. This now causes two tests to fail.

Reviewing.

Review:

Attribute core functions

You have now used modified versions of core functions, which is great as this is solid code. If you do this, it is considered proper to attribute this in the function's docblock. This helps other people understand where this code is coming from. Also describe briefly what is different to the core functions, and an @see line for Doxygen parsing. Example:

/**
 * Form for editing an entire menu tree at once.
 *
 * Shows for one menu the menu links accessible to the current user and
 * relevant operations.
 *
 * This is based on menu_overview_form(), but filters the results on
 * $language_content.
 * @see menu_overview_form()
 */
function paddle_menu_manager_menu_overview_form($form, &$form_state, $menu) {
...
}

Code duplication

The function paddle_menu_manager_menu_overview_form_submit() seems to be identical to menu_overview_form_submit(). I did not compare every line in detail, but if they really are identical you can remove paddle_menu_manager_menu_overview_form_submit() and have your form call the original submit handler. You can do this by adding the following line to the form builder:

function paddle_menu_manager_menu_overview_form($form, &$form_state, $menu) {
...
  $form['#submit'] = array('menu_overview_form_submit');
...

Add a second test for inverse sorting with weights

It is a good point that the initial menu order test is no longer valid as menu items are shown in alphabetical order now. We do need another test now to make sure that the order actually changes when the form is submitted, because if it would remain alphabetical there is a 50% chance that the test will pass. I suggest adding a test that sets the weights first so that the first item has weight 0 and the second weight 1, and testing the result.

Don't use <front> for the placeholders

Currently all links that point to the frontpage are marked as 'Placeholder'. It would be better if they would get the type 'Frontpage' instead. We should detect the actual path "/menu-dummy" for the placeholder pages. Instead of the path we should show the text "no url BLANCO" according to the wireframes. I just talked with cyberwolf about this and he suggests to use this phrase for now, we will change this later to something more fitting.

Change page title

The page title is hard coded to "Menu". This should instead show the name of the menu that is being edited. You can fix this easily by adding these two lines to the hook_menu() configuration:

    'title callback' => 'menu_overview_title',
    'title arguments' => array(4),

I've addressed some of these remarks:

• Add a second test for inverse sorting with weights: commit 68fb0e3
• Don't use <front> for the placeholders: commit 6e87b17
• Change page title: commit 08414a0

I also added some functionality:

a99662 Issue #1934808: Added tests for the menu link types in the menu overview.
6e87b17 Issue #1934808: Reworked support for menu link types in the menu overview.
a8d4e2d Issue #1934808: Allow to pass in options when generating menu links during automated testing.

Looking good! Has been merged in 7.x-1.x. Awesome work, thanks!