Move Region

Hey, thanks for the speedy review!

I have already run the module through this awesome service and have fixed all errors except one before submitting the project application. It now only generates this error about the following doc block:

38 | ERROR | Doc comment for var $id does not match actual variable name $form | | at position 1

/**
 * Menu callback; Form builder for the configuration edit form.
 *
 * @param integer $id
 *   The unique ID of the configuration. Leave empty to create a new
 *   configuration.
 */
function move_region_configure_form($form, &$form_state, $id = NULL) {
}

In this case the script is confused because the default variables $form and $form_state are not documented. IMO this is a shortcoming of the automatic review script. I see this documentation pattern all over core. Here is an example out of block.admin.inc which has also omitted the documentation of $form and $form_state for a form builder:

/**
 * Form constructor for the main block administration form.
 *
 * @param $blocks
 *   An array of blocks, as returned by block_admin_display_prepare_blocks().
 * @param $theme
 *   A string representing the name of the theme to edit blocks for.
 * @param $block_regions
 *   (optional) An array of regions in which the blocks will be allowed to be
 *   placed. Defaults to all visible regions for the theme whose blocks are
 *   being configured. In all cases, a dummy region for disabled blocks will
 *   also be displayed.
 *
 * @return
 *   An array representing the form definition.
 *
 * @ingroup forms
 * @see block_admin_display_form_submit()
 */
function block_admin_display_form($form, &$form_state, $blocks, $theme, $block_regions = NULL) {
}

I am willing to change this however if it is essential that the automated review passes with no errors. I could simply remove the offending line from the doc block. This would make the module marginally less documented but would satisfy the review script.

Thanks a lot for the review! Nice catch, I have fixed it and attributed the patch to you.

Thanks a lot Brian!

I have fixed the missing closing tag. I do respectfully disagree about splitting the long instance of t() on line 75 of move_region.admin.inc. I have just spent some time looking at core to see how calls to t() are handled inside render arrays, and it is consistently put on one single line.

Here's an example that looks quite similar, from system_clean_url_settings():

  $form['clean_url_test_result'] = array(
    '#type' => 'markup', 
    '#markup' => '<p>' . t('Clean URLs cannot be enabled. If you are directed to this page or to a <em>Page not found (404)</em> error after testing for clean URLs, see the <a href="@handbook">online handbook</a>.', array('@handbook' => 'http://drupal.org/node/15365')) . '</p>',
  );

@scot.hubbard: I can't replicate this problem, can you give some more information? Perhaps the content of your header is hard coded in a template file?

@soncco: thanks for the RTBC! :)

Indeed, when you have multiple themes installed it can be difficult to pick the right regions, especially since the region names of different themes are often very similarly named. After enabling 5 contrib base themes it became practically impossible to choose the regions. This probably also caused the problem from #7, since it is possible to select regions from two different themes.

I've updated the module and the regions are now separated out by theme. Much better :) If you would like to try it, please reinstall the module because the database schema has changed.

Good point, I was assuming that newly introduced code == a new review, but having read the workflow I think this is not really the way forward.

Setting back to RTBC as per #8.

Great news, thanks a lot everybody!