Layout regions should be in the order defined by the definition [#2851648]

Problem/Motivation

The goal of #2844302: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase is to move the functionality of Field Layout directly into the EntityDisplay system and Field UI.

In order to accomplish that move, several changes must be made to existing core code, this is one of them.

Layout regions are defined by the layout definition, and should be rendered on the page in that order.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#5	2851648-layoutregions-5.patch	2.97 KB	tim.plunkett
#5	8.4.x: PHP 5.6 & MySQL 5.5 21,634 pass
#5	2851648-layoutregions-5-interdiff.txt	911 bytes	tim.plunkett
#2	2851648-layoutregions-2-PASS.patch	2.95 KB	tim.plunkett
#2	8.4.x: PHP 5.6 & MySQL 5.5 21,622 pass
#2	2851648-layoutregions-2-FAIL.patch	2.02 KB	tim.plunkett
#2	8.4.x: PHP 5.6 & MySQL 5.5 21,620 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

10 February 2017 at 21:37

tim.plunkett created an issue. See original summary.

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 10 February 2017 at 21:40

Status:

Active

» Needs review

File	Size
2851648-layoutregions-2-FAIL.patch	2.02 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,620 pass, 1 fail
2851648-layoutregions-2-PASS.patch	2.95 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,622 pass

Comment #3

10 February 2017 at 22:29

The last submitted patch, 2: 2851648-layoutregions-2-FAIL.patch, failed testing.

Comment #4

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 11 February 2017 at 00:54

+++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
@@ -34,7 +34,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+    foreach ($this->getPluginDefinition()->getRegionNames() as $region) {

Let's call it $region_name. It helps in avoiding the confusion between $region and $regions.

File	Size
2851648-layoutregions-5-interdiff.txt	911 bytes
2851648-layoutregions-5.patch	2.97 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,634 pass

Comment #6

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 13 February 2017 at 15:29

Status:

Needs review

» Reviewed & tested by the community

Thanks!

Comment #7

13 February 2017 at 23:17

xjm committed fee5c76 on 8.4.x

Issue #2851648 by tim.plunkett, jibran: Layout regions should be in the...

Comment #8

13 February 2017 at 23:17

xjm committed 5dc0e21 on 8.3.x

Issue #2851648 by tim.plunkett, jibran: Layout regions should be in the...

The number of regions should not be large enough that which way it is sorted matters, although since these are render arrays the child elements could get quite massive. But the render system has to do orders more manipulation that that, and after all that's what render caching is for. Would be premature optimization to worry about that more here, I think.

It's tested. Expected ordering of things is good. I asked @tim.plunkett about config sorting but he pointed out that the only configurable core implementation yet just adds a single region as one configured property of each field, so the sorting of the regions is not applicable to that.

Since this is an alpha experimental module, this change is eligible for 8.3.x. Committed to 8.4.x and 8.3.x. Thanks!

Comment #10

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 13 February 2017 at 23:29

For the record, this works instead of the foreach, but I found it incomprehensible:

$defined_regions = array_fill_keys($this->getPluginDefinition()->getRegionNames(), []);
$build = array_filter(array_intersect_key(array_replace($defined_regions, $regions), $defined_regions));