Implement Display, a type of config for use by layouts, et. all

The layouts patch (#1787846: Themes should declare their layouts), as it was committed, creates configuration objects that were never intended to be anything more than "layout configuration" - that is, how blocks are arranged into regions defined by a layout. You pair that config with a layout plugin, and you have a layout plugin instance.

I've seen this confusion before, but just because it is using .yml files, it does not mean it is "configuration" (at least in the sense that you can edit it, or create new things via CMI or that it would be staged with CMI). It is not managed by the CMI system and the only part of the config system we are using is the yml parser abstraction (which should probably be a level up from the CMI system anyway). We merely use the yml format to provide *metadata* about the layout. We could have picked XML or .info files just as well.

As for the code, I think the added data stores and containers look fine. I don't understand the $staticData piece and the code comments don't help either. What is the purpose of that? As for the rest:

+++ b/core/lib/Drupal/Core/Config/Entity/Display.phpundefined
@@ -0,0 +1,190 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\Core\Booze\Display.
+ */
+
+namespace Drupal\Core\Config\Entity;
+

There is not a lot of agreement in the docblock and the namespace as to where this is :D

+++ b/core/modules/system/system.moduleundefined
@@ -4118,6 +4118,29 @@ function system_archiver_info() {
 /**
+ * Implements hook_entity_info().
+ *
+ * Provides the core Display object, which contains all the info necessary for
+ * block-driven pages.
+ */
+function system_entity_info() {
+  return array(
+    'booze_display' => array(
+      'label' => t('Display'),
+      'entity class' => 'Drupal\Core\Config\Entity\Display',
+      'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
+      'config prefix' => 'booze.display',
+      'fieldable' => FALSE,
+      'entity keys' => array(
+        'id' => 'id',
+        'label' => 'label', // @todo may not need this if internal-only (?)
+        'uuid' => 'uuid',
+      ),
+    )
+  );

Do we even need to prefix this with something? Why not just use 'display'? We are in core, we can be bold and general.

On the label which is also marked elsewhere, I agree we might not need it if we always create displays as part of other things (page, node type, etc). Not sure how far would the reusability of displays go, it might be useful for reuse the same display for different pages, in which case having them as independently human readably labeled would be useful.

Also I'm wondering where would the creation of these display objects go. I mean they need to have a UI eventually to pick a layout, configure blocks, etc. If we want to reuse this with entity field/property displays and pages alike, the layout picking, block config, etc. would not go to either module, but would instead go to somewhere else. Where does it make sense? If we create a display.module for this and somehow pull in the interactions to creating pages and entity display's that might or might not be overkill (or just repurpose block module for this, since blocks without displays have no way to display themselves and we had blocks to themes relations and display in block module always).

Tagging.

bot

display.view_mode for storing displays describing view modes

Since entity display is configured by bundle, that would probably be something like display.entity.[entity_type].[bundle].[view_mode] - but yeah.

Well, we always had required modules. Just because filter or user module are reequired, they are not lumped into system module, so we can better find their code. So I am not sure that we need to put everything in system module, but again, this can be moved around later.

Nice to see the progress. I agree with #9 that the system_entity_info() should move to layout_entity_info() and the Display class and interface should move to the Drupal\layout namespace. We'll be building up this architecture over many issues, and keeping it all in layout module will help with clarity, I think. Later, after feature freeze, we can move relevant bits into more fundamental locations.

It would be helpful to see a sample display.SOMETHING.yml config file as part of this patch, maybe in a test module for starters until we're ready to do a real one for, say, the Standard profile's block assignments.

I'm noticing a similarity between the relationship of a Display entity and a Layout plugin in this patch as there is between a Grid entity and Grid plugin in #1816650: Add swappable (dynamic) grid systems to core. In other subsystems, we typically have a 1:n relationship (e.g., an ImageStyle config entity references multiple ImageEffect plugin instances, and a ViewsStorage config entity references multiple DisplayPlugin instances), but for Display/Layout and Grid, we have an interesting 1:1 relationship between config entity and plugin instance. I wonder what kind of patterns we want to introduce related to that (for example, should they be named the same, or is there a meaningful distinction between "display" and "layout" that warrants the different names)? Related to this, this issue title currently says "for use by layouts, et al", but is there really an "et al"? If we end up using Display config entities for rendering entity view modes, won't that also require us to use Layout plugins for them as well?

Looking at this from the 'entity display' angle.

- I like the fact that the calling code gets a list of componenents and gets to render them.
This adresses the worries I had about the field_attach_prepare_view($entities) step on list of entities. Should be doable.

- For our use case, though, $blockConfig keys will probably be field names rather than CMI "block config" filenames, though :

'field_foo' => array(
  'region' => 'content',
  'region-role' => 'content',
  'weight' => -100,
  'format' => 'text_trimmed',
  'settings' => array(
    'trim_length' => 200,
  ),
),
'field_bar' => array(
  'region' => 'sidebar_first',
  'region-role' => 'sidebar',
  'weight' => -200,
  'format' => 'integer_default',
  'settings' => array(
    'thousands_separator' => ',',
  ),
),

Maybe a flexible system should let the keys unspecified, free for the calling code to interpret ?

@effulgentsia: I think there is some confusion as to how layouts get their setup with their definition and their configuration was meant to store block placement (in the already committed code). That was introduced so that layout instances would hold block placement. @sdboyer suggests we move that to displays instead and layout instances would not have individual configuration (in my understanding). So:

That is, layouts provide a generic region placement capability, but no direct configuration. Blocks provide output and displays relate the two together with other metadata. Then a display is taken and stuffed up with visibility settings to display on a page. Or when used for field placement for a node type, imagine replacing Page with whatever it would use to apply to node types.

Question is how far should we bring this abstraction. Could block placement, etc. belong in layouts (like in current core committed code) or should be removed direct from Layouts and be only in Displays. How page and field displays are separated since even though they need specific blocks to be sourced (you should not be able to reuse a page display as a node type display since you use very different blocks), displays themselves lack the placement/visibility features on them to be useful at the front end.

Let's figure this out :)

Discussed the reasons for the above setup with @sdboyer in IRC:

[2:32pm] sdboyer: GaborHojtsy: by keeping the blocks decoupled from the layouts themselves, we can create and enforce contracts - styles (which we haven't dealt with yet, but probably should), caching, meta-things like an in-place-editor, without complicating the logic that has to be contained in layouts
[2:32pm] sdboyer: and that's good for everyone, b/c i can't imagine frontenders want to have to deal with that shit (even on the off chance that they WANT to write a full new layout plugin instead fo just a derivative)
[2:34pm] GaborHojtsy: sdboyer: yeah, that makes sense  what I wanted to point out is that displays are in themselves not yet useful, they just connect layouts to blocks and then still need something to "place" it
[2:34pm] GaborHojtsy: sdboyer: AND they are different based on the something tha tis used to place it
[2:34pm] sdboyer: GaborHojtsy: ahhh ok great. yeah we're definitely in agreement on that point
[2:34pm] sdboyer: *points

Suppose block plugins should be related to display in context of regions so we can change a layout but blocks will move to same -named regions in new layout. This also could be very useful for fieldUI - assign fields to region-names and allow to change layout

@andypost: Is that a summary of how the current patch works or a suggestion to change something? To me it sounds like a summary of how the current patch works?!

@Gábor Hojtsy, This is how the patch works, so suggestion to update diagram

+++ b/core/lib/Drupal/Core/Config/Entity/Display.php
@@ -0,0 +1,233 @@
+  public $layout;
...
+  public $layoutSettings = array();
...
+  public $blockConfig = array();
...
+  protected function sortBlocks() {
...
+  protected function remapBlocksToLayout(LayoutInterface $layout) {

This code

Well, in that case I believe that is already represented in the diagram.

re #21:
Well, for now I'm still really reluctant and unconvinced about the "fields displayed within entities are regular blocks" approach.

1) I do fear the perf overhead on this highly critical path, for gains that are not clear to me:
- Blocks are directly adressable : do we really need to load individual HTML for a field through ajax ?
- Block cacheability: cacheability of a rendered entity seems valuable at the entity level, but IMO much less down to the field level ?

2) This would mean that the stored "recipe" for a node display in 'full' or 'teaser' would be split in N CMI files, one per field (and per view mode) - aww.
For editability, manageability and reviewability of CMI files changes during deployments, that's a big -1 compared to a single file per view mode that would contain the structure shown in #12. I, for one, would love to be able to manually edit my node.article.full.yml (or something) files to adjust a view mode.
That's also a lot of config entries to read for a single entity view...

there are similarities, but maybe that just means we should try to mimic/reuse at a pattern level, rather than actually sharing code

That's a very possible outcome :-), although I don't lose all hope that at least some basic code / base class can be shared at some point.
It would make sense IMO to have a base class that "just" deals with storing and handling "a configured list of stuff in regions, mapped to regions in a Layout", without making assumptions what that 'stuff' actually is. That's what I liked about your current approach where rendering of the content happens outside the Display object.

But well, I might very well be mostly talking out of my back, here :-).

Well, page layouts are clearly the priority, with "layouts within entities / Field UI" only a very cool 'could have'. So the best option could be to design the "display / layout" supporting code for this use case primarily, with what you guys feel makes most sense architecturally for this, so that we don't risk getting nothing in. As I wrote in another issue, my goal when chiming on "displays in entities" was not to derail this. When attacking entity display, we can have this "fields as blocks, really ?" discussion with the various parties involved, and see whether we can identify some base code to share.

Then again, this is not my issue, and if you say a lot of people expressed interest / took position on this, I don't want to shut the discussion down here if people feel it needs to happen sooner.
Your call, @sdboyer :-)

I do fear the perf overhead on this highly critical path

In the spirit of #25, I also don't want to derail this issue, but just a quick response to this, and then I'll drop it until there's a better issue to continue that conversation: rather than accepting blocks as having a lot of overhead, and then making fields and other small items expose configuration and render APIs that aren't blocks, I would prefer us to make the Block API capable of handling small, simple units of HTML performantly. In other words, it should be possible to expose blocks that aren't directly addressable, that have their config embedded in a larger object, that aren't independently cacheable, and that don't invoke a subrequest. Then the display of fields can still be exposed as blocks as far as API and configuration UI is concerned. Maybe that's a pipe dream, but I put it out there as something to consider, because I agree that if every block render has a ton of overhead, then it won't make sense to use them at granular levels like individual fields. At the same time, I don't think we should slow down this issue, #1535868: Convert all blocks into plugins, or #1812720: Implement the new panels-ish controller [it's a good purple] worrying about lightweight blocks: I'd rather us get heavyweight blocks in first, then refine the architecture as needed to support lightweight blocks.

Thanks for #29. I'm reading it now, and will post a review if/when I have useful feedback to offer.

The basic architecture here seems sound to me, so on to nits to get this ready for commit:

+++ b/core/modules/layout/lib/Drupal/layout/Config/Display.php
@@ -0,0 +1,93 @@
+  public function __construct(array $values, $entity_type) {
+    // Forcibly restrict this from ever being built as a different entity type.
+    parent::__construct($values, 'display');
+  }

Why do we need this? Entities are instantiated with entity_create() or entity_load(). In either case, hook_entity_info() maps entity type to class. There's no need for us to second guess that hook.

+++ b/core/modules/layout/lib/Drupal/layout/Config/Display.php
@@ -0,0 +1,93 @@
+  public function getLayoutPluginInstance() {

Needs a PHPDoc.

+++ b/core/modules/layout/lib/Drupal/layout/Config/Display.php
@@ -0,0 +1,93 @@
+        throw new \Exception(sprintf('Display "%id" had no layout plugin attached.', array('%id' => $this->id())), E_RECOVERABLE_ERROR);

If we're gonna throw an exception, we should probably create our own namespaced class for it, to make it easier to catch.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+   * The human-readable label of this display.
+   * @todo we probably don't need/want to have this - but is it safe to remove?

Yes. Let's try. If it turns out there's code that breaks without a label, let's fix that code to be more robust.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+   * @todo we might possibly want to separate this into its own config object
+   */
+  public $layoutSettings = array();

How can we resolve this @todo? What are some use cases for layoutSettings?

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+  /**
+   * Storage for data sources that have been statically assigned - that is, not
+   * derived in some way from the Request.
+   *
+   * The array is keyed on the name of a data source, and the values are getters
+   * that are able to retrieve the appropriate data.
+   *
+   * @todo define how the getters work.
+   *
+   * @var array
+   */
+  public $staticData = array();

Would it be okay to remove this from this issue and make it the responsibility of the issue that first implements block data sources to add this? Until then, this is a useless property with no clear API.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+  protected $blocksInRegions;

Needs a PHPDoc.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+  public function getAllBlockInfo() {
+    return $this->blockInfo;
+  }

This and other methods and this class need a PHPDoc.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+  public function getSortedBlocksByRegion($region = '') {

Why default to empty string rather than make it a required parameter?

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+  /**
+   * @param $info
+   */
+  public function setBlockInfo($info) {
+    $this->blockInfo = $info;
+  }

This should either be protected or added to the interface.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+    $layout_instance = $this->getLayoutPluginInstance();
+    if ($this->layout !== $layout_instance->getPluginId()) {

It seems unfortunate that this can ever be true. I guess ideally, we'd make $layout not public, so it could only be changed via setLayout() allowing us to maintain consistency between $layout and the instance. Is there something about ConfigEntity that's requiring all configurable properties to be public, and if so, is there an issue to change that that we can reference in a @todo?

Additionally, why is this method on the abstract class when getLayoutPluginInstance() fails on UnboundDisplay?

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+        // @todo this logic means we only EVER assign to the first region of a type. ick.

Why is this a problem? If there are clear needs to do otherwise, let's add that info to the todo. Otherwise, I think we can remove the todo.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayInterface.php
@@ -0,0 +1,98 @@
+   * @todo this is fairly specific to pages - maybe not best for the interface
+   *
+   * @param Callable $callback
+   *   Any form of callable.
+   * @param array $args
+   *   An array of arguments to pass to the callable.
+   */
+  public function setMainContent($callback, array $args = array());

I recommend removing (set|has)MainContent() from this issue and dealing with it in the controller issue. There, we'll see what code requires it and can better evaluate what interface to add it to. For example, maybe a MainContentAwareInterface, but who knows.

+++ b/core/modules/layout/lib/Drupal/layout/Tests/DisplayInternalLogicTest.php
@@ -0,0 +1,137 @@
+    $this->twocol = display_load('test_twocol');
+    $this->onecol = display_load('test_onecol');
+    $this->unbound = display_load('test_unbound_display', 'unbound_display');

I don't think we want a display_load() wrapper function at all, but even if there's a IDE completion argument for having it, let's not call it from tests.

+++ b/core/modules/layout/lib/Drupal/layout/Tests/DisplayInternalLogicTest.php
@@ -0,0 +1,137 @@
+    $this->assertEqual(count($left), 2, 'Two blocks found in left region.');
+    list($first, $second) = $left;
+    $this->assertEqual($first, 'plugin.core.block.navigation_instance2', 'navigation_instance2 block instance is first in the left region.');
+    $this->assertEqual($second, 'plugin.core.block.main_block', 'main_block block instance is second in the left region.');

Here and elsewhere, this can be shortened to a single assertIdentical() on arrays. Also, the last argument can be omitted: doing so allows the message to be much more useful in case of failure, because then it includes the complete values of both comparables.

+++ b/core/modules/layout/tests/config/display.unbound.test_unbound_display.yml
--- /dev/null
+++ b/core/modules/layout/tests/config/plugin.core.block.main_block.yml

plugin.core is a useless prefix. The filename should start with "block.". block.main_block.yml risks collision with non-test configuration of the same block, so we should use a test-qualified name for the block (e.g., block.test_block.yml or block.layout_test_block.yml or block.main_block__layout_test.yml, etc.).

+++ b/core/modules/layout/tests/config/plugin.core.block.main_block.yml
@@ -0,0 +1,18 @@
+id: system_main_block
+status: '1'
+cache: '-1'
+visibility:
+  path:
+    visibility: '0'
+    pages: ''
+  role:
+    roles: {  }
+  node_type:
+    types:
+      article: '0'
+      page: '0'
+  visibility__active_tab: edit-visibility-path
+subject: 'main content'
+module: system
+region: content
+weight: '0'

Seems like way too many properties here considering the blocks as plugins issue hasn't landed yet. I'd reduce this to just what's needed by the test.

+++ b/core/modules/layout/tests/layout_test.module
@@ -26,7 +26,9 @@ function layout_test_page() {
+  // @fixme this implementation ignores blocks completely, so is inherently incomplete.

s/@fixme/@todo/

From #33:

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.php
@@ -0,0 +1,228 @@
+   * @todo we might possibly want to separate this into its own config object
+   */
+  public $layoutSettings = array();

How can we resolve this @todo? What are some use cases for layoutSettings?

From other "X as plugin" code, it would seem natural to me that the plugin settings are stored in the same location than the plugin id. The [plugin_id, settings] pair contains everything needed to create a layout instance, they go hand in hand, why would we store them in different places ?

Also, regarding this question of settings, shameless plug for #1764380: Merge PluginSettingsInterface into ConfigurableInterface, which is already in use by 'field formatter' and 'fiel widget' plugin types, and that I'll really try to push as a standard for D8 plugins where it applies.

Check the "Review log" fieldset on the test results page in cases like this. It shows this fatal error:

Fatal error: Class name must be a valid object or a string in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/entity.inc on line 263
FATAL Drupal\layout\Tests\DisplayInternalLogicTest: test runner returned a non-zero error code (255).

Also make sure to locally run tests with run-tests.sh, and if that doesn't yield the same errors, additionally try running the failing tests after disabling related modules (here: Layout module). Most often, a test only passes locally, because something of the parent/test runner environment unexpectedly leaks into the test environment - we're not running short on these kind of leakage problems.

I've moved the entity info to the annotation, and adjusted the existing use statements.

This will likely still break, because classes in the same directory didn't have the use statement to begin with.

+++ b/core/modules/layout/layout.moduleundefined
@@ -31,3 +31,36 @@ function layout_theme($existing, $type, $theme, $path) {
+      'config prefix' => 'display',
...
+      'config prefix' => 'display.unbound',

These would conflict. Config prefixes need foo.bar. I'd recommend layout.display and layout.unbound_display

Also, the Type subdirectory for the manager is being phased out in other parts of core, and I'm not sure the Config subdirectory is relevant any longer.

So, label is required. And it was in the hook_entity_info() already, I didn't add it in my patch, I just moved it.

All that was missing was two extra use statements, and the renaming of the test files to match the new config_prefix.

This passed locally for me.

I think this patch looks great. Reviewing it here are some details I found though:

- there are 7 @todo comments in the code; do you think all of those should be in followups, not in the initial patch
- there are *various* methods and classes where the first phpdoc comment line is multiline; that would be a minimal code style requirement I think
- the recently committed layouts in layout module are not actually covered in the patch(?)

Otherwise looks good to me.

@sdboyer: I meant these two new layouts (which are finally outside of tests): http://drupalcode.org/project/drupal.git/commitdiff/99e6cef6d165de3548f4...

Looks good overall, great work sam!

Only remark at this moment is the naming of Left and Right. This should probably be First and Second to respect the mobile first thought.

This will be awesome to work with.

Few remarks:

+++ b/core/modules/layout/layouts/static/twocol/two-col.ymlundefined
@@ -4,5 +4,9 @@ template: two-col
-  first: 'First column'
-  second: 'Second column'
+  first:

"Left side" should be replaced to "First column", no?

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -0,0 +1,194 @@
+   */
+  public $layout;
+

I personally prefer $layoutId of $layoutName as it has the value of getPluginId()

Also some files need a empty ending rule.

OMG IT'S GREEN!!!!! GREAT WORK, FOLKS!! :D :D

Oops. Thought I was on the blocks as plugins issue. ;) Carry on!

Re #59.1, I agree: this is not the issue to rename HEAD's existing region names, so this patch reverts that. Left/Right/Middle is still used by layouts used in tests, but that's ok: tests can have non-semantic names. This also adds missing newlines.

Improved (IMO) names and docs of BoundDisplayInterface. I'll want to do the same for the rest of the patch, but not today.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.php
@@ -0,0 +1,194 @@
+      $this->blocksInRegions[$region_name] = array_keys($blocks);

Is there a reason we call array_keys() here? It means getAllSortedBlocks() and getSortedBlocksByRegion() return only block instance names, not their info. Is this what's wanted? If so, we need to update the docs accordingly.

This one:
- Removes the plugin.core.block.* test config files, because they are not used by the tests.
- Removes staticData from the display.* config files, because that got removed from the scope of this issue in #37.
- Removes plugin.core. from block instance names in display.* config files, because #37 says it's there only for compatibility with #1535868: Convert all blocks into plugins, but that issue still has a lot of failing tests and needs a lot of code review, so very likely, this issue will land first, so this issue has no responsibility to follow that one's unagreed-upon patterns.
- Renames the test blocks to just test_block_1, etc. For the purposes of these tests, there's no need to imply some connection with particular block functionality.
- Cleans up DisplayInternalLogicTest

So looks like we need a well placed @todo then for the sorting issue. I agree if the blocks share the same weight and remapped to the same region, we don't need to provide any magic solution here. The definition for that could be alphabetic.

More docs related reviews:

+++ b/core/modules/layout/lib/Drupal/layout/Config/BoundDisplayInterface.phpundefined
@@ -0,0 +1,78 @@
+
+interface BoundDisplayInterface extends DisplayInterface {

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayInterface.phpundefined
@@ -0,0 +1,39 @@
+
+interface DisplayInterface {

+++ b/core/modules/layout/lib/Drupal/layout/Config/UnboundDisplayInterface.phpundefined
@@ -0,0 +1,36 @@
+
+interface UnboundDisplayInterface extends DisplayInterface {

Lacks docs.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.phpundefined
@@ -0,0 +1,130 @@
+   *
+   * @return array
+   */

What kind of array is returned? :)

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -0,0 +1,194 @@
+  /**
+   * Perform block remapping per mapBlocksToLayout(), but mutate this object
+   * with the remapping results instead of returning them.

Any way to shorten this?

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -0,0 +1,194 @@
+  public function getLayoutInstance() {
+    if ($this->layoutPlugin === NULL) {

Public function but no docs?

+++ b/core/modules/layout/lib/Drupal/layout/Tests/DisplayInternalLogicTest.phpundefined
@@ -0,0 +1,133 @@
+/**
+ * Tests the API and internal logic offered by Displays.
+ *
+ */

Extra newline.

so if eff and i have both substantially worked on this, what are the rules about who can RTBC it?

I feel like my changes have been fairly minor, so if you agree with them, I feel comfortable RTBC'ing. I'd like to do one more pass on the patch this afternoon.

if the blocks share the same weight and remapped to the same region, we don't need to provide any magic solution here. The definition for that could be alphabetic.

It matches what we've done for years for module hook invocation order: order by weight, name; so I agree it's an ok thing to do here as well.

@sdboyer

turning Displays effectively into a block factory has its own separation-of-concern drawbacks in the long term

FWIW, in the "EntityDisplay" work that has started in #1830868: Store entity display settings in EntityDisplay config objects, I'm also considering whether the EntityDisplay object (that basically stores which field is displayed where and how) should also take care of creating and persisting instanciated formatter plugins.

I can't really think of another good place to store instantiated formatters, and keeping them close to the definition from which they were instanciated adds runtime consistency (like, you can wipe them if the definition changes).

Just food for thought for later discussions, when we try to bring the two systems closer. This doesn't have to affect the status of this issue.

I did one more pass on docs and variable names, and am pretty happy with this. RTBC anyone?

@effulgentsia - cool, then maybe after your next pass, i'll RTBC. or you'll RTBC.

Ok, I'll take option 2.

This is holding up a ton of work on a UI for blocks and layouts, so escalating to major, and it's also not something that would be defined by feature freeze (it's a necessary component of SCOTCH), so switching to a task.

I would LOVE to commit this, but due to performance concerns cited above, I think it's best to try and let catch have a pass at this before committing. If that doesn't happen in 24 hours or so, then I'll go ahead and commit it anyway, and we can deal with any concerns in a follow-up.

Also, please make sure to credit at least Gábor and yched on commit. (in addition to everyone else Dreditor picks up)

I've discussed the overall structure of this several times with sdboyer and have been really happy with it.

I'll not commit this yet, but unassigning myself to make it clear I'm happy for webchick to when she wakes up (or I might later but there's a lot of stuff in the RTBC queue at the moment).

Comments follow, if these aren't addressed before commit then could we open a follow-up? I do agree with trying to get this in asap to unblock other work.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.phpundefined
@@ -0,0 +1,134 @@
+   * Block instance configuration is stored in a separate config object. This
+   * array is keyed by the config name that uniquely identifies each block
+   * instance. At runtime, various object methods will retrieve this additional

I'm definitely worried about this in the 'fields formatters are a block' case, since we've only just got to the point where field instance configuration caching is performant about a month ago. However on the assumption that we absolutely will not convert view modes + fields to this model without also avoiding hundreds of config() calls for separate objects, I'm OK with punting on that for now. I'm wondering if there's a compromise between re-usable instances across displays vs. allowing a block instance to be configured that only exists on a specific display (i.e. analogous to field configuration on display modes now) - so the display would inject that locally stored config into the block, or get it separately from the config system, dependent on how which type it is.

For top-level blocks on a page this doesn't seem too bad compared to all the other CMI conversions going on - i.e. that's already an extremely serious problem that has a critical task to fix it, and it's not made significantly worse here - just more of the same. block_list() has never been a thing of performance beauty either (hence mongodb_block module iirc), and at least by centralizing all this stuff we have the potential for optimizing all of it as well.

effulgentsia made a good point in the geenral field formatters as blocks discussion already - we should try to optimize blocks up the wazoo first, then look at other solutions for fields if that's not possible - but in the process we can probably improve things a bit for blocks even if it's not good enough. The idea of issuing full sub-requests for field formatters fills me with a deep deep horror so let's not ever get anywhere close to there please ;)

+   *      'block2-configkey' => array(
+   *        'region' => 'sidebar_first',
+   *        'region-type' => 'aside',
+   *        'weight' => -100,
+   *      ),
+   *      'block2-configkey' => array(
+   *        'region' => 'sidebar_first',
+   *        'region-type' => 'aside',
+   *        'weight' => 0,

When would two blocks be assigned to the same region, but to different region types? I can't really see the reason for the duplication here.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.phpundefined
@@ -0,0 +1,134 @@
+   *        // store the region type name here so that we can do type conversion w/out
+   *        // needing to have access to the original layout plugin

Well I guess that's it. In that case, why would access to the original layout plugin be a problem? Is that to handle the case where it goes missing or you're importing something into a site that's missing it?

If we absolutely can't have the original layout plugin available, couldn't we store the region -> type mapping in a separate key from the blocks so it only appears once in the file (and is optional)?

+      // Finally, fall back to dumping everything in the layout's first region.
+      else {
+        $info['region'] = reset($layout_regions_indexed);
+      }

Since $layout_regions_indexed is only used in the fallback case, we should do the array_keys() inside the else { statement instead of the top of the function. Or could probably remove the intermediate variable by just using key() (or key(array_values()) if there really is a chance the array pointer needs to be reset).

The actual logic of region first, then region type, then whatever seems great though. Was thinking last region might be better than first on most displays as the last resort but that's not important.

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.phpundefined
@@ -0,0 +1,134 @@
+  public function getAllRegionTypes() {
+    $types = array();
+    foreach ($this->blockInfo as $info) {
+      $types[$info['region-type']] = TRUE;
+    }
+    return array_keys($types);

I'd probably build a normal array then use array_unique() instead of relying on non-duplicate string keys and array_keys()? Or is there a particular reason to not do that?

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -0,0 +1,186 @@
+    foreach ($block_info as &$info) {
+      unset ($info['region']);
+    }

Stray space here after unset

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -0,0 +1,186 @@
+    if (!$entity instanceof UnboundDisplayInterface) {
+      throw new \Exception(sprintf('Attempted to create an unbound display using an invalid entity type.'), E_RECOVERABLE_ERROR);

Wondering if we couldn't reduce the amount of stuff that an Entity has to be instantiated with, then just pass a full object in here and add this information to it. That way it could use type-hinting on the method instead of instanceOf?

Failed to unassign myself. Didn't look at the new patch but thanks for the response, all those make sense!

Awesome. Committed and pudshed to 8.x. Thanks!

Issue to implement master display configuration: #1841584: Add and configure master displays. Issue to implement master display based page display configuration: #1840500: Add simple blank page creation capability.

Sorry, was out of town for a couple days.

re @catch #81:

I'm definitely worried about this in the 'fields formatters are a block' case

Yeah, so am I, but for now the work that started on EntityDisplays (#1830868: Store entity display settings in EntityDisplay config objects - in a sandbox for now, still in progress) stays out of the block concept entirely. The EntityDisplay object directly stores display properties for the entity components (fields and 'extra fields').

We'll see down the road if "speeding light blocks" can happen, but for now that's the only viable way to rework entity rendering without massive perf impact.

Initial patch for Entity displays posted at #1852966: Rework entity display settings around EntityDisplay config entity