Introduce Display Variants, use for the block rendering flow

+ return \Drupal::service('uuid'); +
The full page variant could override this and inject the uuid service, it already implements container factory plugin interface

Looks great

+++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
@@ -0,0 +1,146 @@
+class FullPageVariant extends VariantBase implements ContainerFactoryPluginInterface {

class doc missing.

I don't see anything in here that makes me think we shouldn't commit this now.

Eclipse

Looks good. The @Annotation and @inheritdoc blocks make me want to poke my eyes out but that's not invented here.

+    $page += \Drupal::service('plugin.manager.display_variant')
+      ->createInstance('full_page')
+      ->render();

The method should be called 'build'. We have been careful in the past to reserve the word render to refer to the process of turning a render array into HTML. In this case we are building the render array, not rendering it.

Still RTBC.

Eclipse

Awesome, I like this issue. :)

+++ b/core/lib/Drupal/Core/Display/VariantBase.php
@@ -0,0 +1,133 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateConfigurationForm(array &$form, array &$form_state) {
+  }

Do we need this if it's empty?

Probably this patch should have been waved through, in that case sorry. Can we please add a tag for that?

1. +++ b/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
   @@ -0,0 +1,35 @@
   +
   +/**
   + * Defines a display variant annotation object.
   + *
   + * @Annotation
   + */
   +class DisplayVariant extends Plugin {
   
   +++ b/core/lib/Drupal/Core/Display/VariantBase.php
   @@ -0,0 +1,133 @@
   +/**
   + * Provides a base class for DisplayVariant plugins.
   + */
   +abstract class VariantBase extends PluginBase implements VariantInterface {
   
   +++ b/core/lib/Drupal/Core/Display/VariantInterface.php
   @@ -0,0 +1,77 @@
   +/**
   + * Provides an interface for DisplayVariant plugins.
   + */
   +interface VariantInterface extends PluginInspectionInterface, ConfigurablePluginInterface, PluginFormInterface {
   
   +++ b/core/modules/block/tests/src/Display/FullPageVariantTest.php
   @@ -0,0 +1,146 @@
   +/**
   

   Nothing in this patch explains what a display variant actually means.

2. +++ b/core/lib/Drupal/Core/Display/VariantInterface.php
   @@ -0,0 +1,77 @@
   +  /**
   +   * Determines if this display variant is accessible.
   +   *
   +   * @return bool
   +   *   TRUE if this display variant is accessible, FALSE otherwise.
   +   */
   +  public function access();
   

   Is there a reason why the account is not passed in here? This seems to be something which could be done in a follow up,
   but why should we ... this patch introduces new code already.

3. +++ b/core/lib/Drupal/Core/Display/VariantManager.php
   @@ -0,0 +1,37 @@
   +    parent::__construct('Plugin/DisplayVariant', $namespaces, $module_handler, 'Drupal\Core\Display\Annotation\DisplayVariant');
   

   OT: Did we ever considered to move the namespace, annotation class, cache key and alter hook name into parameters in the service definition?

4. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -0,0 +1,160 @@
   + * Contains \Drupal\block\Plugin\DisplayVariant\FullPageVariant.
   ...
   +class FullPageVariant extends VariantBase implements ContainerFactoryPluginInterface {
   
   +++ b/core/modules/block/tests/src/Display/FullPageVariantTest.php
   @@ -0,0 +1,146 @@
   + * @file
   + * Contains \Drupal\block\Tests\Display\FullPageVariantTest.
   + */
   

   Don't we no longer sync the namespace in the actual code and in the tests?

5. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -0,0 +1,160 @@
   +   */
   +  protected function getTheme() {
   +    if (!$this->theme) {
   +      $this->theme = $this->themeNegotiator->getActiveTheme();
   +    }
   +    return $this->theme;
   +  }
   

   Doesn't the theme negotiator provides its own caching? One less variable is one less possible static caching bug.

6. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -0,0 +1,160 @@
   +    foreach ($this->getRegionAssignments() as $region => $blocks) {
   +      /** @var $blocks \Drupal\block\BlockInterface[] */
   ...
   +   * @return array
   +   *   The array is first keyed by region machine name, with the values
   +   *   containing an array keyed by block ID, with block entities as the values.
   +   */
   +  protected function getRegionAssignments() {
   

   If getRegionAssignments() would document the return array the inline docs would not be needed ...

7. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -0,0 +1,160 @@
   +      @uasort($assignment, 'Drupal\block\Entity\Block::sort');
   

   Should we better document why we have to hide errors here? This does not seems to be obvious for me.

8. +++ b/core/modules/block/tests/src/Display/FullPageVariantTest.php
   @@ -0,0 +1,146 @@
   + * Tests the full page display variant.
   + *
   + * @group Drupal
   + * @group Block
   + * @group Display
   + */
   +class FullPageVariantTest extends UnitTestCase {
   ...
   +  /**
   +   * Tests the rendering of a full page variant.
   +   */
   +  public function testRender() {
   

   Missing @covers annotations.

9. +++ b/core/modules/block/tests/src/Display/FullPageVariantTest.php
   @@ -0,0 +1,146 @@
   +    $this->themeNegotiator = $this->getMockBuilder('Drupal\Core\Theme\ThemeNegotiator')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   

   Can't we use the interface? This might be a leftover from my previous fuckups.

1. +++ b/core/modules/block/block.module
   @@ -125,33 +112,6 @@ function block_page_build(&$page) {
   - * Gets a renderable array of a region containing all enabled blocks.
   ...
   -function block_get_blocks_by_region($region) {
   
   @@ -242,43 +202,6 @@ function block_theme_initialize($theme) {
   - * Returns all blocks in the specified region for the current user.
   ...
   -function block_list($region) {
   
   +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -0,0 +1,157 @@
   +class FullPageVariant extends VariantBase implements ContainerFactoryPluginInterface {
   ...
   +  protected function getRegionAssignments() {
   

   Is API change, also leaving no API to get regions and their blocks

2. +++ b/core/modules/system/system.api.php
   @@ -600,6 +600,19 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
   +  $definitions['full_page']['admin_label'] = 'Block layout';
   

   admin label better to wrap in t()

Those were not API functions to begin with, and you can still get regions and blocks the normal way, see system_region_list and entityQuery.

Not sure how fair this is tbh. Drupal people have a massive amount of missuage of all the "API"s.

7) Because PHPUnit invocation matching triggers https://bugs.php.net/bug.php?id=50688 :(

Is there a chance we document that?

OT: Did we ever considered to move the namespace, annotation class, cache key and alter hook name into parameters in the service definition?

Yes, this was considered and discussed in Portland, whThiere I worked on DefaultPluginManager. However, the needed arguments were a bit more complex back then (an array), and it was not possible to have everything in services.yml, and @yched was against splitting this up.

Seems like it should be possible not, would be quite nice. We would have to make those methods public, though, I think they are protected right now.

Having worked quite a bit on BlockViewBuilder and touched block_get_blocks_by_region() etc., the overall approach and changes in this patch look great!

Spent a good hour on this with Tim, mostly with docs. The patch itself is pretty straight-forward... it introduces a new concept of a "display variant" and uses one of them ("full page") to render out full page views from Block module, in lieu of calling functions like block_get_blocks_by_region(). Apart from that change it's 90% copy/paste boilerplate code that you need to implement a plugin, and then tests.

We struggled with the docs of "display variant" for awhile though. It's tricky because core doesn't (yet) use the concept of displays; that may come or not as a result of #2292733: [meta] Introduce Display objects. And display variants themselves are incredibly generic, and can be used for everything from a full page display ala Block module to a single "block" content ala mini panels to even a simple http response code type of deal. In the end, we settled for a description of what HEAD does with this patch as an example, and kind of left it at that. I think that's enough for someone who comes across this term and says "wtf?" to have a reasonable shot at being directed how to read up on how it works, and this entire description will get overhauled anyway when/if the display issue makes it in, so didn't make sense to go from 80% => 100% in this case.

Apart from that, the only things that stood out to me were the losing of some "orientation" comments around the removed code in block_page_build (restored a one-liner there) and the @uasort thing that's already been mentioned above, but that's got docs next to it explaining what's going on.

This is a nice change overall that's not very invasive on its own, but nevertheless introduces some very nice flexibility that hopefully all of the various "core block/layout replacement" modules out there can make use of, without each inventing their own wheels. Change notice looks fine.

Ergo, committed and pushed to 8.x (with some docs modifications). Thanks!