Add region module to Drupal core (for editable responsive layouts)

Having had this issue with both D6 and 7 I think this is a much needed, missing piece of functionality. I also think many projects currently doing page_alter injections (like admin_menu) would be better served by adding their own region and then positioning output within that. This makes projects that add to the UX/UI better fitting with the way drupal's core blocks / plugins would function in their interaction with screen output instead of being a series of one-off implementations.

I've taken a stable at creating a framework for defining regions and adding them to Drupal (theme-agnostic) at a system level with the module http://drupal.org/project/regions . Agreed though, Regions w/o layout abstraction are just kind of O.K. at best.

Quick review. Just a nit pick.

+  /**
+   * Tests editing a default region.
+   */
+  public function testDeleteDefaultRegion() {
+    // Create a new user, allow him to manage the blocks and the languages.
+    $admin_user = $this->drupalCreateUser(array(
+      'administer regions',
+    ));

First line should read "* Tests deleting a default region."

No other concerns, looks good to go. But I am no code expert, so another reviewer would be good before RTBC.

Actually, one other comment.

Looked through a lot of other modules tests, and all I can see declare tests as straight 'function'

Yours are declared as 'public function'

As I said previously, I am by no means an expert, is there a reason for this?

One more comment, but this is one that should not/will not be resolved here.

Many modules us the following format for function names

comment_confirm_delete

You use

region_delete_confirm

This lack of consistency is throughout Drupal, and I would like to see this changed.

See issue #469698: Establish naming convention for form IDs

Just adding here to see if I can gather some support for that issue :)

In function region_menu() you have the following

function region_menu() {
  $items = array();
  $items['admin/structure/regions'] = array(
    'title' => 'Regions',
    'description' => 'Manage list of regions that allow content to be placed.',
    'page callback' => 'region_page_list',
    'access callback' => 'user_access',
    'access arguments' => array('administer regions'),
    'file' => 'region.admin.inc',
  );
  $items['admin/structure/regions/add'] = array(

What's the reason for $items = array(); at the beginning.

Don't see this in other modules.

That concludes my in depth review.

Like I said before, you will need an adult to review as well. I am just a novice.
Hope I have been of some help.

Sorry I keep coming back :( Next review I do, will be done outside of here, and then into one comment.

Anyway, I see no help in your region.module. We surely need some before this can go.

This needs some more work, so changing to 'needs work'

#22: region-module-22.patch queued for re-testing.

I have tested this on a new install of 8.x-dev with simpletest, and also manually tested everything I could think of.

Tried hard to break it. All errors were correctly reported, everything works as expected.

I have read over the code, and tried to look for things not to standards, but as I am still learning the standards, not the most in depth review on that.

Something I am still not sure about. Should this

/**
 * Entity URI callback.
 *
 * @param Drupal\region\Region $region
 *   Region configuration entity instance.
 *
 * @return array
 *   Entity URI information.
 */

not have a backslash before Drupal\region\Region.
This occurs more then once.

Also the newly added create admin user, needs a short docblock.

This is my pass at that.

  /**
   * Create a new user, with administer regions permission.
   */

  function setUp() {
    parent::setUp();

    $admin_user = $this->drupalCreateUser(array('administer regions'));
    $this->drupalLogin($admin_user);
  }

Hope some adults will test, and looking forwards to seeing this all come together.

Won't jump in and create a patch as I am not sure these comments/changes are correct.

A nitpick. I know your default layout is just a sample but I was wondering why you define a 'body' region when I thought the drupalism was 'content'. I just thought it confusing to not use a common drupal label and one that might get confused with html's 'body'

In reference to #27.

Here is an extract from coding standards.

Note the bold line.

http://drupal.org/node/1354

The data type MAY be specified (as in: recommended) for other @param or @return directives, including those with primitive data types, arrays, and generic objects (stdClass).

* @param int $number
* Description of this parameter, which takes an integer.
* @param float $number
* Description of this parameter, which takes a floating-point number.
* @param string $description
* Description of this parameter, which takes a string.
* @param bool $flagged
* Description of this parameter, which takes a Boolean TRUE/FALSE value.
* @param array $args
* Description of this parameter, which takes a PHP array value.
* @param object $account
* Description of this parameter, which takes a PHP stdClass value.
* @param \Drupal\Core\Database\StatementInterface $query
* Description of this parameter, which takes an object of a class that
* implements the StatementInterface interface.
* @param string|bool $string_or_boolean
* Description of this parameter, which takes a string or a Boolean TRUE/FALSE
* value.
* @param string|null $string_or_null
* (optional) Description of this optional parameter, which takes a string or
* can be NULL.
*
* @return string
* Description of the return value, which is a string.

Also see further down on same page.

Using namespaces and namespaced classes in documentation

The following guidelines apply to the use of namespaces in documentation blocks:

1. When using a class or interface name immediately after any @-tag in documentation, prefix it with the fully-qualified namespace name (starting with a backslash). If the class/interface is in the global namespace, prefix it with a backslash.
2. Elsewhere in documentation, omit the namespace if the class/interface is within the use/namespace declaration context of the file.
3. If you refer to a class/interface that is not in the current file's use/namespace declaration context, or that is in the global namespace, prefix it with the fully-qualified namespace name (starting with a backslash).

Example -- given that classes Foo, Bar, and Thingy are in the A\B\C namespace, and class Baz is in the X\Y namespace:

And sorry, these are not complaints. I am learning how to contribute, and an easy entry point is coding standards.
Sorry if it seems I am being negative, not my intention.

Like you, I have also searched all core, and yes, the standards are not being used consistently. No one's fault, just some things are not clear/easy to find, and in a fast paced change environment, standards are normally applied later then earlier.

Keep up the great work, and hope to see this and other stuff your working on, committed. These are great end user features.
And I look forward to using them when D8 is released, and even before.

And you are correct on the setUp() function. Sorry about that. Did not realise at the time that tests had their own documentation standards.

There is no PHPDoc on this function since it is an inherited method.

Like I said, I am learning :).

Not impossible to add in a minor version.