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.
Comment | File | Size | Author |
---|---|---|---|
#22 | region-module-22.patch | 17.72 KB | Gábor Hojtsy |
#22 | interdiff.txt | 2.48 KB | Gábor Hojtsy |
#21 | region-module-21.patch | 18.31 KB | Gábor Hojtsy |
#21 | interdiff.txt | 520 bytes | Gábor Hojtsy |
#20 | region-module-21.patch | 18.31 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyFollowing @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.
Comment #4
Gábor HojtsyRdfaMarkupTest.php was failing, that should not be related.
Comment #5
Gábor Hojtsy#2: region-module-2.patch queued for re-testing.
Comment #6
btopro CreditAttribution: btopro commentedHaving 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.
Comment #7
Gábor HojtsyApplying 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? :)
Comment #8
mbrett5062 CreditAttribution: mbrett5062 commentedQuick review. Just a nit pick.
First line should read "* Tests deleting a default region."
Comment #9
Gábor HojtsyGood find, updated patch attached. That is the only difference, so no interdiff. Any other concerns/comments?
Comment #10
mbrett5062 CreditAttribution: mbrett5062 commentedNo other concerns, looks good to go. But I am no code expert, so another reviewer would be good before RTBC.
Comment #12
mbrett5062 CreditAttribution: mbrett5062 commentedActually, 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?
Comment #13
mbrett5062 CreditAttribution: mbrett5062 commentedOne 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 :)
Comment #14
mbrett5062 CreditAttribution: mbrett5062 commentedIn function region_menu() you have the following
What's the reason for
$items = array();
at the beginning.Don't see this in other modules.
Comment #15
mbrett5062 CreditAttribution: mbrett5062 commentedThat 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.
Comment #16
mbrett5062 CreditAttribution: mbrett5062 commentedSorry 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.
Comment #17
mbrett5062 CreditAttribution: mbrett5062 commentedThis needs some more work, so changing to 'needs work'
Comment #18
Shyamala CreditAttribution: Shyamala commentedComment #19
Gábor HojtsyApplied *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?
Comment #20
Gábor HojtsyOne minor typo in a code comment.
Comment #21
Gábor HojtsyOne more code comment change was missing.
Comment #22
Gábor HojtsySimplify test code by centralising common user creation to setUp method.
Comment #24
webchick#22: region-module-22.patch queued for re-testing.
Comment #25
mbrett5062 CreditAttribution: mbrett5062 commentedI 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
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.
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.
Comment #26
mbrett5062 CreditAttribution: mbrett5062 commentedComment #27
Gábor HojtsyCan 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.
Comment #28
Gábor Hojtsy#22: region-module-22.patch queued for re-testing.
Comment #30
Gábor HojtsyThe fail is due to #1831774: Config import assumes that 'config_prefix' contains one dot only, not introduced with this patch.
Comment #31
dcrocks CreditAttribution: dcrocks commentedA 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'
Comment #32
mbrett5062 CreditAttribution: mbrett5062 commentedIn 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:
Example -- given that classes Foo, Bar, and Thingy are in the A\B\C namespace, and class Baz is in the X\Y namespace:
Comment #33
mbrett5062 CreditAttribution: mbrett5062 commentedAnd 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.
Comment #34
mbrett5062 CreditAttribution: mbrett5062 commentedAnd you are correct on the setUp() function. Sorry about that. Did not realise at the time that tests had their own documentation standards.
Like I said, I am learning :).
Comment #35
Gábor HojtsyWell, 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.
Comment #36
Gábor HojtsyGiven 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.
Comment #37
catchNot impossible to add in a minor version.