This is a sub-issue to #1813898: [META] Add editable responsive layouts to Drupal core

Problem/Motivation

Drupal core only supported static layouts so far, with defining characteristics of a fixed set of regions and a static markup structure with pre-baked CSS positioning. It is not possible to edit these layouts in a user friendly way, so with #1813898: [META] Add editable responsive layouts to Drupal core, we are proposing a set of modules to serve this need in Drupal core.

Proposed solution

This issue is about the dynamic region list handler, which is a very simple module that handles a list of machine name + label combinations with some pre-defined regions shipped with the module in configuration, all regions being editable and deletable. These regions are going to be used in the editable layouts later on in other modules.

Admittedly, this module has not much use without editable layouts given these regions are not expected to be used or useful outside of editable layouts.

The proposed module is named region with a configuration entity called Region and the respective form controllers and default configuration. Test coverage for creating regions, editing regions, deleting regions and doing these with built-in regions is included.

Files: 
CommentFileSizeAuthor
#22 region-module-22.patch17.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 48,167 pass(es), 0 fail(s), and 105 exception(s).
[ View ]
#22 interdiff.txt2.48 KBGábor Hojtsy
#21 region-module-21.patch18.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,553 pass(es).
[ View ]
#21 interdiff.txt520 bytesGábor Hojtsy
#20 region-module-21.patch18.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,539 pass(es).
[ View ]
#20 interdiff.txt533 bytesGábor Hojtsy
#19 interdiff.txt7.4 KBGábor Hojtsy
#19 region-module-20.patch18.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,549 pass(es).
[ View ]
#9 region-module-9.patch17.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 46,364 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#7 interdiff.txt4.48 KBGábor Hojtsy
#7 region-module-7.patch17.78 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,375 pass(es).
[ View ]
#2 region-module-2.patch17.63 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,830 pass(es).
[ View ]
#2 interdiff.txt644 bytesGábor Hojtsy
#1 region-module-1.patch17.63 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,720 pass(es).
[ View ]

Comments

StatusFileSize
new17.63 KB
PASSED: [[SimpleTest]]: [MySQL] 42,720 pass(es).
[ View ]

StatusFileSize
new644 bytes
new17.63 KB
PASSED: [[SimpleTest]]: [MySQL] 42,830 pass(es).
[ View ]

Following @andypost's advice that (bool) $region->id() should be replaced with !$region->isNew() in favor of the possible introduction of #1813832: Entity wrongly checks existence of ID in isNew() method.

Status:Needs review» Needs work
Issue tags:-Spark Sprint 7

The last submitted patch, region-module-2.patch, failed testing.

RdfaMarkupTest.php was failing, that should not be related.

Status:Needs work» Needs review

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

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.

StatusFileSize
new17.78 KB
PASSED: [[SimpleTest]]: [MySQL] 46,375 pass(es).
[ View ]
new4.48 KB

Applying best practices from other patches:

- rename /regions/region/%region/edit, etc. to /regions/manage/%region/edit, etc.
- make edit and delete local tasks
- update tests accordingly

Anything else we should do here? :)

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."

StatusFileSize
new17.78 KB
FAILED: [[SimpleTest]]: [MySQL] 46,364 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Good find, updated patch attached. That is the only difference, so no interdiff. Any other concerns/comments?

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

Status:Needs review» Needs work

The last submitted patch, region-module-9.patch, failed testing.

Status:Needs work» Needs review

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.

Status:Needs review» Needs work

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

Issue tags:+Responsive Design

Status:Needs work» Needs review
StatusFileSize
new18.31 KB
PASSED: [[SimpleTest]]: [MySQL] 46,549 pass(es).
[ View ]
new7.4 KB

Applied *all* changes suggested by @mbrett5062. See attached interdiff.

Also, since #1763974: Convert entity type info into plugins landed, we needed hook_entity_info() converted to annotations, or 90% of the patch did not work at all.

Looks good now?

StatusFileSize
new533 bytes
new18.31 KB
PASSED: [[SimpleTest]]: [MySQL] 46,539 pass(es).
[ View ]

One minor typo in a code comment.

StatusFileSize
new520 bytes
new18.31 KB
PASSED: [[SimpleTest]]: [MySQL] 46,553 pass(es).
[ View ]

One more code comment change was missing.

StatusFileSize
new2.48 KB
new17.72 KB
FAILED: [[SimpleTest]]: [MySQL] 48,167 pass(es), 0 fail(s), and 105 exception(s).
[ View ]

Simplify test code by centralising common user creation to setUp method.

Status:Needs review» Needs work
Issue tags:-Responsive Design, -Dynamic layouts, -Spark

The last submitted patch, region-module-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Responsive Design, +Dynamic layouts, +Spark

#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.

Status:Needs review» Needs work

Status:Needs work» Needs review

Can you point to a standards entry for the "missing backslash"? If I search current Drupal 8 for "@param Drupal", I find hundred if not thousands of use of that *without* a backslash to start it out. Also for the setUp() method, it is a common method of test classes, and I believe they should not be documented, similar to getInfo(). I looked through all block module tests for example and it was perfectly as-is.

So looks like no other complaints with the current code then? Sending for a retest.

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

Status:Needs review» Needs work
Issue tags:+Responsive Design, +Dynamic layouts, +Spark

The last submitted patch, region-module-22.patch, failed testing.

The fail is due to #1831774: Config import assumes that 'config_prefix' contains one dot only, not introduced with this patch.

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 :).

Status:Needs work» Postponed

Well, anyway, due to core things not happening below this layer that we wanted to build on, we just need to go and focus on those things and will pretty likely not have a responsive layout builder (and therefore this module) in core. Unless someone comes and want to run with all these. Start from #1813898: [META] Add editable responsive layouts to Drupal core.

Version:8.x-dev» 9.x-dev
Status:Postponed» Needs work

Given feature freeze, this is not going to happen in Drupal 8. Looks like Panels will serve the dynamic page builder role in Drupal 8. Moving this to Drupal 9.