Problem/Motivation

The layout module was removed in #2053879: Remove layout.module from core

We're not proposing adding it back in this issue, but instead just putting a very simple plugin manager for a layout plugin type into core.

Many modules need to declare or use layouts, for example: Display Suite, Panels, Layout, etc. We want to avoid the situation in D7 and earlier, where each module has their own silo'ed way of doing this.

We already have created the Layout Plugin module in contrib, which several modules (including Panels and Display Suite) have already standardized on and now have interchangeable layouts!

But we think it should be in core, in order to:

  • Promote standardization on a single layout plugin type in contrib. It's sooo simple that it's almost a shame that it takes a whole module that other modules have to depend on. :-)
  • Take the first little baby step towards adding more complete layout functionality in core. We're continuing to work on layouts in contrib, with modules like Tim Plunkett's Page Manager module, but once those have matured we'd like to progressively merge parts of it into core. This is the most minimal step in that direction.

Proposed resolution

Move Layout Plugin into core as an experimental module.

In 'layout_plugin', layouts are declared in a .yml file and simply give a template and library definition. Here's an example layout .yml file:

http://cgit.drupalcode.org/layout_plugin/tree/modules/layout_plugin_exam...

If you want to learn more about declaring layouts with layout_plugin here are the docs:

https://www.drupal.org/node/2578731

Remaining tasks

User interface changes

In this proposal, there is no user interface!

API changes

This would include API additions (no changes) to support this plugin type, ex: LayoutPluginInterface, LayoutPluginManager, etc.

CommentFileSizeAuthor
#188 2296423-layout_discovery-188.patch66.14 KBtim.plunkett
#188 2296423-layout_discovery-188-interdiff.txt732 bytestim.plunkett
#182 2296423-layout_discovery-182.patch66.02 KBtim.plunkett
#182 2296423-layout_discovery-182-interdiff.txt858 bytestim.plunkett
#160 layout-interdiff.txt41.58 KBtim.plunkett
#157 2296423-layout_plugin-layout_discovery-157-interdiff.txt649 bytestim.plunkett
#157 2296423-layout_plugin-layout_discovery-157.patch65.18 KBtim.plunkett
#154 2296423-layout_discovery-154.patch65.16 KBtim.plunkett
#154 2296423-layout_discovery-154-interdiff.txt5.79 KBtim.plunkett
#138 2296423-layout_plugin-layout_discovery-138.patch64.52 KBtim.plunkett
#138 2296423-layout_plugin-layout_discovery-138-interdiff.txt8.59 KBtim.plunkett
#128 2296423-layout_plugin-128.patch63.15 KBtim.plunkett
#128 2296423-layout_plugin-128-interdiff.txt9.67 KBtim.plunkett
#125 2296423-layout_plugin-125.patch62.89 KBtim.plunkett
#125 2296423-layout_plugin-125-interdiff.txt5.87 KBtim.plunkett
#123 2296423-layout_plugin-123-interdiff.txt26.21 KBtim.plunkett
#123 2296423-layout_plugin-123.patch61.35 KBtim.plunkett
#104 2296423-layout_plugin-104.patch63.13 KBtim.plunkett
#104 2296423-layout_plugin-104-interdiff.txt3.1 KBtim.plunkett
#104 layout selector.png24.3 KBtim.plunkett
#102 2296423-layout_plugin-102.patch62.61 KBtim.plunkett
#102 2296423-layout_plugin-102-interdiff.txt2.1 KBtim.plunkett
#96 2296423-layout_plugin-96-interdiff.txt9.32 KBtim.plunkett
#96 2296423-layout_plugin-96.patch62.4 KBtim.plunkett
#94 2296423-layout_plugin-94.patch63.54 KBtim.plunkett
#94 2296423-layout_plugin-94-interdiff.txt617 bytestim.plunkett
#91 2296423-layout_plugin-91.patch63.44 KBtim.plunkett
#90 2296423-layout_plugin-90-interdiff.txt4.03 KBtim.plunkett
#90 2296423-layout_plugin-90-PASS.patch63.52 KBtim.plunkett
#90 2296423-layout_plugin-90-FAIL.patch62.89 KBtim.plunkett
#86 2296423-layout_plugin-86.patch61.75 KBtim.plunkett
#86 2296423-layout_plugin-86-interdiff.txt11.81 KBtim.plunkett
#82 2296423-layout_plugin-82.patch56.33 KBtim.plunkett
#82 2296423-layout_plugin-82-interdiff.txt38.41 KBtim.plunkett
#73 2296423-layout_plugin-73.patch59.39 KBtim.plunkett
#73 interdiff-2296423.txt1.92 KBtim.plunkett
#72 interdiff-2296423.txt1.3 KBtim.plunkett
#72 2296423-layout_plugin-72.patch58.57 KBtim.plunkett
#71 2296423-70-71.txt9.33 KBtedbow
#71 2296423-70.patch58.37 KBtedbow
#70 2296423-layout_plugin-70.patch57.84 KBtim.plunkett
#70 2296423-layout_plugin-70-interdiff.txt802 bytestim.plunkett
#68 2296423-layout_plugin-68-interdiff.txt1 KBtim.plunkett
#68 2296423-layout_plugin-68.patch57.75 KBtim.plunkett
#65 2296423-layout_plugin-65-interdiff.txt21.73 KBtim.plunkett
#65 2296423-layout_plugin-65.patch57.62 KBtim.plunkett
#64 2796173-field_layout-65-interdiff.txt35.51 KBtim.plunkett
#64 2796173-field_layout-65.patch46.92 KBtim.plunkett
#63 2296423-layout_plugin-62.patch30.75 KBtim.plunkett
#63 2296423-layout_plugin-62-interdiff.txt8.47 KBtim.plunkett
#61 2296423-layout_plugin-61.patch28.92 KBtim.plunkett
#61 2296423-layout_plugin-61-interdiff.txt42.23 KBtim.plunkett
#57 interdiff.txt520 bytesdsnopek
#57 drupal8-layout_plugin-2296423-57.patch35.13 KBdsnopek
#56 interdiff.txt10.81 KBdsnopek
#56 drupal8-layout_plugin-2296423-56.patch34.62 KBdsnopek
#54 interdiff.txt881 bytesManuel Garcia
#54 implement_layout_plugin-2296423-54.patch25.25 KBManuel Garcia
#47 interdiff.txt993 bytesdsnopek
#47 drupal8-layout_plugin-2296423-47.patch25.49 KBdsnopek
#46 interdiff.txt54.46 KBdsnopek
#46 drupal8-layout_plugin-2296423-46.patch26.07 KBdsnopek
#45 interdiff.txt11.45 KBdsnopek
#45 drupal8-layout_plugin-2296423-45.patch29.09 KBdsnopek
#42 interdiff.txt3.29 KBdsnopek
#42 drupal8-layout_plugin-2296423-42.patch35.25 KBdsnopek
#41 interdiff.txt5.61 KBdsnopek
#41 drupal8-layout_plugin-2296423-41.patch34.58 KBdsnopek
#33 interdiff.txt7.73 KBdsnopek
#33 drupal8-layout_plugin-2296423-33.patch34.11 KBdsnopek
#32 interdiff.txt6.18 KBdsnopek
#32 drupal8-layout_plugin-2296423-32.patch38.85 KBdsnopek
#23 interdiff.txt1.15 KBdsnopek
#23 drupal8-layout_plugin-2296423-23.patch38.5 KBdsnopek
#21 interdiff.txt4.86 KBdsnopek
#21 drupal8-layout_plugin-2296423-21.patch38.3 KBdsnopek
#13 drupal8-layout_plugin-2296423-13.patch35.99 KBdsnopek
#16 interdiff.txt2.48 KBswentel
#16 drupal8-layout_plugin-2296423-16.patch36.42 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek’s picture

Issue summary: View changes

frega has refactored just the plugin part of his 'layout' module on GitHub into a sandbox on Drupal.org. Updated the issue summary to reflect where the continuing work on that will be!

dsnopek’s picture

If we can't get this into core, another possibility is merging it into the Layout module. However, that depends on what that team's plans are for the future: #2317333: Roadmap / future plans for layout.module

seanr’s picture

I definitely support this - it's a pain having multiple different kinds of layout engines in a single site, arbitrarily chosen by whatever dev I inherited the site from. :-/

dsnopek’s picture

Issue summary: View changes

Update issue summary now that we've promoted layout_plugin to a full project.

andypost’s picture

There's a couple of questions:
1) Any reason to separate layout yml and plugin? Looks that plugin annotation could define the same properties of layouts.
2) Region definition in yml has only Label so why not merge them into one line:

  regions:
-    left:
-      label: Left region
-    right:
-      label: Right region
+    left: Left region
+    right: Right region
dsnopek’s picture

1) Any reason to separate layout yml and plugin? Looks that plugin annotation could define the same properties of layouts.

The layout yml is actually an alternate discovery mechanism for the plugin, they're the same thing. See the LayoutPluginManager:

http://cgit.drupalcode.org/layout_plugin/tree/src/Plugin/Layout/LayoutPl...

It just wraps it's $this->discovery in YamlDiscoveryDecorator.

So, developers have the choice of implementing either the layout yml (for the simple case where it's just a template) or a plugin (where it's more advanced, like some kind of flexible layout builder).

2) Region definition in yml has only Label so why not merge them into one line:

So, "label" is the only required attribute of a region that we expect all layouts to implement. But we allow custom layout plugin classes to use other attributes on the region configuration for any purpose. For example, frega's layout module on GitHub implements a flexible layout builder which uses other region arguments, and we may need something similar for all of Panels layouts.

This should probably be improved in the documentation!

andypost’s picture

@dsnopek thanx for clarity and taking that!

2) is a question still because core uses "third_party_settings" to extend any yml because of schema issues.
Actually I think that part needs more work

dsnopek’s picture

2) is a question still because core uses "third_party_settings" to extend any yml because of schema issues.

So, I think this is different. This is plugin configuration, not configuration in CMI. Also, it's not really "third party" since any additional settings will be read by the plugin class itself, so it's still first party. The extra keys just aren't defined by the "standard interface" and plugin classes can define some new ones of their own.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

needs a beta evaluation. added link to instructions to do that in the summary.

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev

Looks certainly more than 8.1.x

Anonymous’s picture

:'(

dsnopek’s picture

Issue summary: View changes

Modernizing the issue summary for the current state of layout_plugin.

dsnopek’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs issue summary update +display suite
FileSize
35.99 KB

Here's a patch that just copies layout_plugin into core and removes the example modules and some deprecated code. Please let me know what you think!

swentel’s picture

Mostly nictpicks

  1. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,45 @@
    +<?php
    +/**
    + * @file
    + * Hook implementations for Layout Plugin module.
    + */
    +
    

    I don't know exactly what our coding standards say, but sometimes the @file docblock starts at the top, sometimes with a new line in this patch. Looking at core, it seems a newline is the standard, so let's make it consistent everywhere.

  2. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,45 @@
    +/**
    + * Prepares variables for layout templates.
    + *
    

    should we add a @see here to Layout::layoutPluginManager()->alterTheme... ?

  3. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,232 @@
    +class LayoutPluginManager extends DefaultPluginManager implements LayoutPluginManagerInterface {
    +
    +
    +  /**
    

    newline to much

  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,16 @@
    +namespace Drupal\layout_plugin\Plugin\Layout;
    +
    +
    

    One newline to much

  5. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,232 @@
    +class LayoutPluginManager extends DefaultPluginManager implements LayoutPluginManagerInterface {
    +
    +
    +  /**
    

    same here

I should probably create a patch for the actual module now ;)

The biggest question is: do we need some sort of upgrade path? If a site upgrades to 8.1.x and it had the contrib enabled, how will extension discovery behave then after a full cache clear. Will it pick up the contrib module or the one from core (my guess is the contrib because that's how it works now IIRC).

Another name for the module wouldn't work since the contrib modules would still have their dependencies set to layout_plugin, or we need to coordinate releases, but that seems hard, and we don't control custom code either right ? Moving the code into system module wouldn't work either for the dependency problem as well for the modules.

Other than that, RTBC of course ;)

(oh irony, we need a hook help haha)

swentel’s picture

Status: Needs work » Needs review
FileSize
36.42 KB
2.48 KB

Fix my nitpicks and composer test. No hook_help() for now though ...

swentel’s picture

Issue tags: +Needs change record

Guess this needs a change record too.

benjy’s picture

+++ b/core/modules/layout_plugin/src/Layout.php
@@ -0,0 +1,26 @@
+class Layout {
...
+  public static function layoutPluginManager() {

Should this move to \Drupal?

+++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
@@ -0,0 +1,231 @@
+          // @todo: Should be the version of the provider module or theme.
+          'version' => 'VERSION',

Needs fixing?

dsnopek’s picture

Re #19.1: If layout_plugin stays being a separate module, I don't think we should put that method on \Drupal because it will only return a valid value if the module is installed.

However, given how simple layout_plugin is, I could see an argument for merging it directly into \Drupal\Core (then adding the plugin manager to core.services.yml, and putting the hook implementations into 'system') in which case putting that method on \Drupal totally make sense!

The block system has it's code in \Drupal\Core and its plugin manager is registered in core.services.yml, so there is a similar-ish precedent! And this would solve our issues with an upgrade path from a contrib 'layout_plugin' module to the core code. But it's really the hook implementations in layout_plugin.module that give me pause... I think we need feedback from a core committer on if this would be acceptable.

Re #19.2: Ah, yes! I almost totally forgot about that. :-) I'll fix it in my next patch.

dsnopek’s picture

dsnopek’s picture

Status: Needs work » Needs review
FileSize
38.5 KB
1.15 KB

Ok, second try at getting the hook_help() right!

benjy’s picture

Issue tags: +Needs committer feedback

Adding the committer feedback tag to get some input on #20.

dawehner’s picture

  1. +++ b/core/modules/layout_plugin/layout_plugin.info.yml
    @@ -0,0 +1,5 @@
    +package: Layout
    

    IMHO we don't need the layout package or do we? IMHO it belongs into the main one as all the other ones?

  2. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,57 @@
    +  return Layout::layoutPluginManager()->getThemeImplementations();
    ...
    +  Layout::layoutPluginManager()->alterThemeImplementations($theme_registry);
    ...
    +  return Layout::layoutPluginManager()->getLibraryInfo();
    

    I kinda dislike to have those static wrappers

  3. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +   * Base path (relative to current module) to all resources (like the icon).
    

    module / theme

  4. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +  /**
    +   * The CSS file.
    +   *
    +   * If specified, then the layout_plugin module will register the library for
    +   * this CSS file automatically and the module or theme registering this layout
    +   * does not need to do it. This is mutually exclusive with 'library' - you
    +   * can't specify both.
    +   *
    +   * @var string optional
    +   */
    +  public $css;
    +
    

    Any reason to not also support js files? I could imagine JS files could be useful for layouts as well?

  5. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +   * The key of the array is the machine name of the region, and the value is
    +   * an associative array with the following keys:
    +   * - label: (string) The human-readable name of the region.
    

    What is the point of having just one key in the value? Why is it not a map of machine name and label like in .info.yml files?

  6. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +   * An remaining keys may have special meaning for the given layout plugin, but
    

    Shouldn't it be 'A remaining' instead?

  7. +++ b/core/modules/layout_plugin/src/Layout.php
    @@ -0,0 +1,27 @@
    +  public static function layoutPluginManager() {
    +    return \Drupal::service('plugin.manager.layout_plugin');
    +  }
    +
    

    IMHO we should get rid of that, it just sets a bad example.

  8. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutInterface.php
    @@ -0,0 +1,32 @@
    + */
    +interface LayoutInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurablePluginInterface, PluginFormInterface {
    +
    

    Does that interface have to expose all of the subinterfaces automatically? For example the ConfigurablePluginInterface and PluginFormInterface fells totally optional and orthogonal.

  9. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,248 @@
    +    foreach ($plugins as $id => $plugin) {
    +      if ($group_by_category) {
    +        $category = isset($plugin['category']) ? (string) $plugin['category'] : 'default';
    +        if (!isset($options[$category])) {
    +          $options[$category] = array();
    +        }
    +        $options[$category][$id] = $plugin['label'];
    

    Is there a reason the LayoutManager doesn't leverages the CategorizingPluginManagerInterface machinery?

  10. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,248 @@
    +              '/' . $definition['css'] => [],
    

    This is confusing, we just allow absolute css paths?

  11. +++ b/core/modules/layout_plugin/tests/src/Unit/PluginManagerTest.php
    @@ -0,0 +1,371 @@
    +
    +    // A layout with a template path.
    +    $definition = [
    +      'label' => 'Split layout',
    +      'category' => 'Test layouts',
    +      'template' => 'templates/split-layout',
    +      'provider' => 'layout_plugin_test',
    +      'path' => 'layouts',
    +      'icon' => 'images/split-layout.png',
    +      'regions' => [
    +        'first' => ['label' => 'First region'],
    +        'second' => ['label' => 'Second region'],
    +      ],
    +    ];
    +    $plugin_manager->processDefinition($definition, 'split_layout');
    +    $this->assertEquals('modules/layout_plugin_test/layouts', $definition['path']);
    +    $this->assertEquals('modules/layout_plugin_test/layouts/templates', $definition['template_path']);
    +    $this->assertEquals('modules/layout_plugin_test/layouts/images/split-layout.png', $definition['icon']);
    +    $this->assertEquals('split_layout', $definition['theme']);
    

    IMHO you could split up this test method into multiple single ones to be more descriptive.

andypost’s picture

Suppose in real would layouts should be a part of library so separate CSS/JS assets are question, here a question howto aggregate this assets
Also relation with entity displays a bit depends on that issue cos they could really a nice example of "usage" instead of "unusable module"

main code related concerns are about location of code:
1) base class instead of base implementation
2) interface definition
3) boilerplate code without traits

  1. +++ b/core/modules/layout_plugin/layout_plugin.info.yml
    @@ -0,0 +1,5 @@
    +name: Layout Plugin
    

    I find this name very technical, maybe Layout management or Layouts?
    In long run that should be a part of core folder somehow

  2. +++ b/core/modules/layout_plugin/layout_plugin.info.yml
    @@ -0,0 +1,5 @@
    +package: Layout
    

    Please make it Experimental to polish in next minors

  3. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,57 @@
    +      return t('Layout Plugin allows modules or themes to register layouts, and for other modules to list the available layouts and render them. Without a module or theme that provides layouts, or a module that uses them to render something, this module does not do anything. For more information, see the <a href=":layout-plugin-documentation">online documentation for the Layout Plugin module</a>.', [':layout-plugin-documentation' => 'https://www.drupal.org/node/2619128']);
    

    >this module does not do anything.
    bad part of help...sentence

  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +   * Get the plugin's description.
    ...
    +  public function getDescription() {
    ...
    +   * Get human-readable list of regions keyed by machine name.
    ...
    +  public function getRegionNames() {
    ...
    +   * Get information on regions keyed by machine name.
    ...
    +  public function getRegionDefinitions() {
    ...
    +   * Get the base path for all resources.
    ...
    +  public function getBasePath() {
    ...
    +   * Get the full path to the icon image.
    ...
    +  public function getIconFilename() {
    

    This is a part of interface, is not it?

  5. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +   * Get the asset library name.
    ...
    +   * @return string
    +   *   The asset library.
    ...
    +  public function getLibrary() {
    +    return isset($this->pluginDefinition['library']) && $this->pluginDefinition['library'] ? $this->pluginDefinition['library'] : FALSE;
    ...
    +   * Get the theme function for rendering this layout.
    ...
    +  public function getTheme() {
    +    return isset($this->pluginDefinition['theme']) && $this->pluginDefinition['theme'] ? $this->pluginDefinition['theme'] : FALSE;
    

    I think this should be optional (empty array by default, not bool type)
    Why so slightly theme, that can be a render element

  6. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +  public function build(array $regions) {
    +    $build = $regions;
    +    $build['#layout'] = $this->getPluginDefinition();
    +    $build['#settings'] = $this->getConfiguration();
    +    if ($theme = $this->getTheme()) {
    +      $build['#theme'] = $theme;
    +    }
    +    if ($library = $this->getLibrary()) {
    +      $build['#attached']['library'][] = $library;
    

    $build['#type'] = 'layout' . '__' . $this->getPluginDefinition();
    #settings and #attached

    this should simplify default template

  7. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    ...
    +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
    ...
    +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    ...
    +  public function getConfiguration() {
    ...
    +  public function setConfiguration(array $configuration) {
    ...
    +  public function defaultConfiguration() {
    ...
    +  public function calculateDependencies() {
    

    there should be traits somehow

  8. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,15 @@
    + * Provides a default class for Layout plugins.
    ...
    +class LayoutDefault extends LayoutBase {
    

    Why that is not fallback implementation for other plugins

  9. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutInterface.php
    @@ -0,0 +1,32 @@
    +interface LayoutInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurablePluginInterface, PluginFormInterface {
    ...
    +   * Build a render array for layout with regions.
    ...
    +  public function build(array $regions);
    

    toRenderable() existing interface method

  10. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,248 @@
    +    $this->defaults += array(
    +      'type' => 'page',
    ...
    +      'class' => 'Drupal\layout_plugin\Plugin\Layout\LayoutDefault',
    

    why not simply '#type' = 'layout'

  11. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,248 @@
    +  public function getThemeImplementations() {
    ...
    +  public function alterThemeImplementations(array &$theme_registry) {
    

    in case of render element plugin is not needed

  12. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManagerInterface.php
    @@ -0,0 +1,65 @@
    +   * Get all available layouts as an options array.
    ...
    +  public function getLayoutOptions(array $params = []);
    

    so this is a options of the layout or categorizable list?

  13. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManagerInterface.php
    @@ -0,0 +1,65 @@
    +   * Get theme implementations for layouts that give only a template.
    ...
    +  public function getThemeImplementations();
    ...
    +   * Modifies the theme implementations for the layouts that we registered.
    ...
    +  public function alterThemeImplementations(array &$theme_registry);
    

    any strong reason to expose theme implementations?
    I suppose they defined by modules.

  14. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManagerInterface.php
    @@ -0,0 +1,65 @@
    +   * Get library info for layouts that want to automatically register CSS.
    ...
    +  public function getLibraryInfo();
    

    Library contains also JS and could carry SVG of preview

Manuel Garcia’s picture

Status: Needs review » Needs work
Wim Leers’s picture

I'm reviewing this specifically from the asset library system POV.

  1. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +  /**
    +   * The asset library.
    +   *
    +   * If specified, it's assumed that the module or theme registering this layout
    +   * will also register the library in its *.libraries.yml itself. This is
    +   * mutually exclusive with 'css' - you can't specify both.
    +   *
    +   * @var string optional
    +   */
    +  public $library;
    +
    +  /**
    +   * The CSS file.
    +   *
    +   * If specified, then the layout_plugin module will register the library for
    +   * this CSS file automatically and the module or theme registering this layout
    +   * does not need to do it. This is mutually exclusive with 'library' - you
    +   * can't specify both.
    +   *
    +   * @var string optional
    +   */
    +  public $css;
    

    Let us please not do this.

    We standardized on asset libraries in Drupal 8. Let's not knowingly add exceptions to that rule.

    If even themes can do that (issue: #2377397: Themes should use libraries, not individual stylesheets, CR: https://www.drupal.org/node/2379475), then so can this module.

  2. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,156 @@
    +   * The path to the preview image (relative to the base path).
    

    The Drupal base path? And with or without a leading slash? This is not sufficiently well defined.

    And what about a HiDPI version?

  3. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +   * Get the asset library name.
    

    "library name" is wrong.

    Given the library core/ckeditor, "ckeditor" is the name, "core" is the extension, "core/ckeditor" is the library.

    I think this is supposed to be the latter.

  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +   * Get the theme function for rendering this layout.
    ...
    +  public function getTheme() {
    ...
    +    if ($theme = $this->getTheme()) {
    +      $build['#theme'] = $theme;
    +    }
    

    "theme" vs "theme function", which is it?

    This leads to the confusing code cited.

  5. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,161 @@
    +  public function calculateDependencies() {
    +    return [];
    +  }
    

    If the asset library is provided by a different extension, this is wrong.

  6. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,15 @@
    +/**
    + * Provides a default class for Layout plugins.
    + */
    +class LayoutDefault extends LayoutBase {
    +
    +}
    

    Eh… this looks extremely bizarre.

  7. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutPluginManager.php
    @@ -0,0 +1,248 @@
    +        $library_info[$library_name] = [
    +          'version' => $this->getProviderVersion($definition['provider_type'], $definition['provider']),
    +          'css' => [
    +            'theme' => [
    +              '/' . $definition['css'] => [],
    +            ],
    +          ],
    +        ];
    

    What about CSS for separate media types? And why the theme SMACSS category, shouldn't this be layout, since this is CSS for … layout plugins?

    https://www.drupal.org/developing/api/8/assets

    All the more reason to let layout plugins be more specific, and not automatically generate asset libraries for them. One way to do something versus two.

dsnopek’s picture

Thanks, everyone, for all the review! I had a saved response for #25 from last week that I didn't quite finish (because I got caught up in other things) but now I've got a quite a bit more to chew on. :-) I'll do my best to follow-up ASAP!

tim.plunkett’s picture

Title: Implement layout plugin type in core? » Implement layout plugin type in core

I think this should be in \Drupal\Core, as opposed to a module. There is no UI, nor will there ever be for this functionality.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
38.85 KB
6.18 KB

Ok! I had these changes and response from 3 months ago - there's loads more changes I want to make based on some conversations with @tim.plunkett, but getting this out there because I have it. More to come!

@dawehner: Thanks for the review!

#25.1: The logic was the same as how modules that provide field types are in the "Field types" category. There are contrib modules like Radix Layouts and Bootstrap Layouts that also use the "Layout" category, and so this groups all this stuff together. However, I suppose the "Field" module itself is under the "Core" category! So, I'll move this to the "Core" category as well.

#25.2: I'm not attached to the Layout::layoutPluginManager(), it's just a convenience. If necessary, I'd be happy to remove it (that would solve the "should it be on \Drupal?" question from #19 as well. Leaving it in for now.

#25.3: Fixed!

#25.4: We do support Javascript, you just have to declare your own asset library and use the 'library' key instead. The 'css' key is a convenience for super simple layouts, that declares the asset library for you. Using Javascript is an advanced enough that I'm (personally) fine requiring developers to declare the asset library for the Javascript themselves.

#25.5: This is to allow layouts to define their own extra data that could be attached to regions, to be used by the layout plugin for its own purposes. It's used by frega's flexible layout builder (which is now totally out of date and won't work any more :-/) and there's talk of putting "region weights" in the Panels layouts to allow the Panels region changing to work better. There's an attempt at an explanation further in the docblock which was quoted in #25.6, but with a type-o.

#25.6: Actually, the "An remaining" was meant to be "Any remaining". Fixed! Any suggestions on making this explanation clearer are welcome!

#25.7: Responded in #2 above. Happy to remove if others agree!

#25.8: Hm, yeah. So, you're thinking that layouts which support configuration would implement ConfigurablePluginInterface and PluginFormInterface themselves and we'll do an instanceof check for them before trying to show the settings form or doing setConfiguration()? This makes sense to me. Moved those interfaces to LayoutBase.

#25.9: Heh, because I didn't know that existed. :-) Fixed!

#25.10: No, we just convert the relative path (from the module or theme) to a path relative to the Drupal install in processDefinition(). The slash is added because its required by the libraries asset API (where as $definition['path'] is a filesystem path we can do file_exists() on).

dsnopek’s picture

Thanks, @Wim Leers, for your review in #28!

@tim.plunkett and I discussed #28.1 a month or so ago, and decided that you're right - if this is going to be part of core, it shouldn't provide a convenience feature to help people not directly use the library asset system. We don't want one part of core to undermine another. :-)

Here's a patch that simply pulls out that functionality - it doesn't address any of the rest of the review in #28.

swentel’s picture

So if we drop the css property, that would mean we have to create a new branch for DS because our default layouts use that feature,
Or we need to register it ourselves for backwards compatibility in the same branch ?

In both cases, I feel nervous dropping it because I don't know how exactly this will effect existing sites that already use DS.
Can we mark it as deprecated somehow to be removed in D9 ?

tim.plunkett’s picture

+++ b/core/modules/layout_plugin/layout_plugin.module
@@ -0,0 +1,50 @@
+function layout_plugin_theme() {
+  return Layout::layoutPluginManager()->getThemeImplementations();
+}
...
+function layout_plugin_theme_registry_alter(&$theme_registry) {
+  Layout::layoutPluginManager()->alterThemeImplementations($theme_registry);
+}

I still believe this should not be a module, meaning these will move to system.module, which is very strange.

As far as #35 goes, we should talk/think more about that. Putting a BC layer into core seems strange. Perhaps the module could live on in contrib just to provide that?

dsnopek’s picture

@swentel: A separate branch shouldn't be necessary - DS could start manually registering it's libraries right now! It's just an additional YAML file.

As far as backwards compatibility: we could make the layout_plugin module add a shim to support the 'css' property so layouts created by users continue to work?

dsnopek’s picture

I agree with Tim in #36 that this shouldn't be a module and I may have an idea to remove the preprocess function, which I'll attempt at the sprint tomorrow. But those two hooks will still need to be in system in that case.

swentel’s picture

@dsnopek Sure, DS can register it for our layouts, and adding it then if wanted - we actually have a killswitch option to remove the css, oh irony :)
I was indeed more wondering about people who've created their own layouts and used DS as a (bad) example using a css file.
Granted, I guess that number will be fairly small, so maybe I'm worrying to much.

dsnopek’s picture

@swentel: I think that's a valid worry, for sure! But I think we make the version of the contrib layout_plugin module act as a backwards compatibility shim after this merged into core.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
34.58 KB
5.61 KB

Here's a patch where I went through and tried to fix the docblocks for the Layout annotation class make them match with LayoutBase to address Wim's concerns in #28.2, 3 and 4. #28.7 was already address by removing the automatic library registration in #33.

#28.6: The LayoutDefault class is the one used if you register your layout via YAML. It doesn't need anything beyond what's in LayoutBase, but it seems weird/wrong to have layout plugins which are instances of LayoutBase (ie. concrete instances of a base class). :-) Maybe LayoutBase is doing too much and we should move some of that stuff into LayoutDefault? If nothing else, this reserves the possibility that the layouts registered via YAML could do something differently than the LayoutBase.

#28.5: Is a valid issue (thanks for catching it, Wim!) that still needs to be handled.

dsnopek’s picture

Here's an attempt to fix #28.5!

dawehner’s picture

Just another driveby review.

+++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
@@ -0,0 +1,184 @@
+   * @return \Drupal\Core\Annotation\Translation|NULL
+   *   The human-readable name.
...
+   * @return \Drupal\Core\Annotation\Translation|NULL
+   *   The layout description.
...
+   * @return \Drupal\Core\Annotation\Translation
+   *   The human-readable category.
...
+   * @return \Drupal\Core\Annotation\Translation[]
+   *   An array of human-readable region names keyed by machine name.

This feels just wrong ... aren't we returning strings / translation objects?

dsnopek’s picture

Here's a patch that kills hook_theme_registry_alter() and the preprocess function, but in order to do it it has to put render arrays under '#content' in the layout render array, which isn't super clean, but it allows removing a whole lot of code.

Thanks @dawehner! My latest patch switches all of those to strings too.

dsnopek’s picture

Alright! Here's a patch with the big move from a module to being under \Drupal\Core\Layout. It only requires minimal stuff added to the system module.

dsnopek’s picture

Here's a patch to remove a left-over function that should have been removed when I pulled out the automatic library registration in #33

swentel’s picture

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -350,6 +350,13 @@ block.settings.local_tasks_block:
+layout.settings:
+  type: mapping
+  mapping: { }
+
+layout.settings.*:
+  type: layout.settings
+

Can't we move this into a file in core/config/schema ? Otoh, if we can't move the call in system_theme, it kind of make sense for it to be here I guess.

dsnopek’s picture

re #48: I think it could be moved there from a technical perspective! I'd love some feedback from someone on the core team if that's preferable or not?

jibran’s picture

I think we should add a layout test module with some kernel tests to wire all this up and check it is really working. It'd be a good example for dev's as well.

dsnopek’s picture

#50: I concur! In the contrib version of layout_plugin we have functional tests that depend on Page Manager, which we obviously can't use here, but kernel tests that go further than the unit tests would be really, really good. Or I suppose we could even do some functional tests with a completely contrived example (ie. configuring a set of three hard-coded strings to appear in layout regions), but I'm not sure it's worth all the extra boilerplate?

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

but I'm not sure it's worth all the extra boilerplate?

It's always worth it.
Let's move the config changes to core/config/schema/core.layout.schema.yml as per #48.

andypost’s picture

Version: 8.2.x-dev » 8.3.x-dev

Looks missed release window

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
25.25 KB
881 bytes

Moving layout schema into its own file as per #48.

andypost’s picture

green, so now NW for tests and reviews

dsnopek’s picture

Here's a new patch that has some kernel tests that actually renders some example layouts. The layouts were ported from the 'layout_plugin_example' module.

dsnopek’s picture

Yay, the tests pass!

The one thing that still isn't really tested is the config schema stuff. Here's a new patch that adds a config schema for the one layout with settings, but doesn't add tests for it yet. I'm not super sure how to test that (without building a whole thing to configure and save layout configuration) so if any one has any ideas, please go for it!

mlhess’s picture

Issue tags: +MWDS2016
romainj’s picture

Issue https://www.drupal.org/node/2786541 is a start to a UI that list registred layouts provided by any module/theme. It could be improved for sure but at least it helps to know all available layouts.

dsnopek’s picture

Status: Needs review » Needs work

There's been some minor changes in layout_plugin in contrib that need to get integrated into this patch, so marking as "Needs work" so that we don't push forward without doing that. I'd still love some more reviews of the current patch, though! :-)

tim.plunkett’s picture

Rerolling as an experimental module again. Yes we'd like to get it into \Drupal\Core eventually, but this will expedite things.

Made some tweaks, and removed some things. It now works for DS and for field_layout (#2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts). Need to decide if we want to include getLayoutOptions() or not.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
30.75 KB

Copied a bunch more code over from layout_plugin.

tim.plunkett’s picture

tim.plunkett’s picture

Had to reimplement CategorizingPluginManagerInterface directly due to object-based definitions.
Added test coverage for everything in LayoutPluginManager.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
802 bytes
57.84 KB

Apparently PHPUnit is "broken" in 5.5/5.6: If you have an expectation of an exception, it should ignore all other predictions.
This works fine in PHP 7.

Working around it!

tedbow’s picture

Looking good!
Here is a patch that is mostly doc fixes.

I will continue to look at not non-nit things.

tim.plunkett’s picture

More nits I noticed.

tim.plunkett’s picture

Okay final set. Going to try and stop now :)

jhedstrom’s picture

+++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinitionInterface.php
@@ -0,0 +1,223 @@
+  public function getTheme();
...
+  public function setTheme($theme);
...
+  public function hasThemeImplementation();
...
+  public function getTemplate();
...
+  public function setTemplate($template);
...
+  public function getTemplatePath();
...
+  public function setTemplatePath($template_path);

If the theme implementations are meant to be optional, could this instead be a separate interface that relevant layouts could implement?

This could could then just use instanceof ...:

+++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
@@ -0,0 +1,218 @@
+    // If 'template' is set, then we'll derive 'template_path' and 'theme'.
+    $template = $definition->getTemplate();
+    if (!empty($template)) {
...
+    foreach ($definitions as $definition) {
+      if ($definition->hasThemeImplementation()) {
+        $hooks[$definition->getTheme()] = [
tim.plunkett’s picture

99.9% of these plugins will be defined via YAML. There is no mechanism for them to choose which implementation of LayoutDefinitionInterface they use.
Also, those methods are really only used internally by the manager themselves.

If you feel strongly about inventing a new mechanism for that, I can try. But otherwise I'd just as soon leave it as is.

jhedstrom’s picture

@tim.plunkett ah, I get it now. I hadn't made the connection to the yaml definitions. I don't feel very strongly, it just stood out as a potential simplification :)

jhedstrom’s picture

The IS needs to be updated to reflect the experimental module direction. As I understand it, there will then need to be a meta issue that tracks remaining tasks to make it non-experimental...

tim.plunkett’s picture

Updated. There is a parent issue in the Ideas queue, it needs more detail.

andypost’s picture

The only confusing here is usage of *theme* in variables and methods, I suggest to use themeHook
Also wondered mix of template & them, not sure it's possible to use template without themehook defined in registry

  1. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,129 @@
    +   * The theme hook used to render this layout.
    ...
    +  public $theme;
    

    I'd better named this as $theme_hook

  2. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,78 @@
    +  public function calculateDependencies() {
    +    return [];
    

    maybe add plugin definition provider by default?

  3. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -0,0 +1,78 @@
    +  public function getPluginDefinition() {
    +    return parent::getPluginDefinition();
    

    Any reason for that? just for typehinting...

  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinition.php
    @@ -0,0 +1,314 @@
    +  public function getTheme() {
    +    return $this->theme;
    ...
    +  public function hasThemeImplementation() {
    +    return $this->getTemplate() && $this->getTheme();
    

    This is a bit confusing.
    How it's possible to have template without theme hook?

    Also I think getThemeHook() better name

  5. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinitionInterface.php
    @@ -0,0 +1,223 @@
    +  public function getTheme();
    ...
    +  public function setTheme($theme);
    

    ThemeHook to prevent confusion with theme (theme could be provider)

  6. +++ b/core/modules/layout_plugin/tests/modules/layout_test/config/schema/layout_test.schema.yml
    --- /dev/null
    +++ b/core/modules/layout_plugin/tests/modules/layout_test/css/layout-test-1col.css
    
    +++ b/core/modules/layout_plugin/tests/modules/layout_test/css/layout-test-1col.css
    +++ b/core/modules/layout_plugin/tests/modules/layout_test/css/layout-test-1col.css
    @@ -0,0 +1 @@
    
    @@ -0,0 +1 @@
    +
    
    +++ b/core/modules/layout_plugin/tests/modules/layout_test/layout_test.layouts.yml
    @@ -0,0 +1,21 @@
    +  library: layout_test/layout_test_1col
    
    +++ b/core/modules/layout_plugin/tests/modules/layout_test/layout_test.libraries.yml
    @@ -0,0 +1,11 @@
    +layout_test_1col:
    ...
    +      css/layout-test-1col.css: {}
    

    any reason for empty file, maybe better to leave this without library for testing layout without library

dsnopek’s picture

Thanks @tim.plunkett for pushing this forward! I'm super behind on reviews here -- sorry about that -- but I'm going to start by looking at changes in this patch relative to my last core patch in #57, to try and make sure we aren't losing anything from previous reviews/changes here.

  1. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +        $hooks[$definition->getTheme()] = [
    +          'render element' => 'content',
    +          'template' => $definition->getTemplate(),
    +          'path' => $definition->getTemplatePath(),
    

    The current patch is going back to a theme hook based on 'render element' (like in the contrib layout_plugin). Switched to using 'variables' back in #45 per review from #36, so that we could elminate the hook_theme_registry_alter() (which also been added back in the current patch) to reduce the number of hooks it needs to just one and move it into \Drupal\Core rather than having it be a module.

    Since the ultimate goal is still for it to not be a module and move it into \Drupal\Core, I think we should stick with that change, so we aren't changing the theme hook definition for all layouts at that point when other code depends on it. (We didn't make this change in the contrib layout_plugin for backcompat figuring we can make the transition with the transition to core and the other changes it needs).

  2. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,47 @@
    +function layout_plugin_theme_registry_alter(&$theme_registry) {
    +  \Drupal::service('plugin.manager.layout_plugin')->alterThemeImplementations($theme_registry);
    +}
    

    This (and the code its calling) is what we can eliminate by switching from 'render element' to 'variables' in the point above.

  3. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -50,20 +38,6 @@ class Layout extends Plugin {
    -   * An optional description for advanced layouts.
    -   *
    -   * Sometimes layouts are so complex that the name is insufficient to describe
    -   * a layout such that a visually impaired administrator could layout a page
    -   * for a non-visually impaired audience. If specified, it will provide a
    -   * description that is used for accessibility purposes.
    -   *
    -   * @var string
    -   *
    -   * @ingroup plugin_translatable
    -   */
    -  public $description;
    

    Why is this being removed? Is there another property on the definition that's replacing this?

    This is a valid accessibility problem (originally raised by the UC Berkeley accessibility team for Panels in D7 - which is still unresolved there :-/) and I'd prefer to start out on the right foot in D8!

  4. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -113,13 +87,6 @@ class Layout extends Plugin {
    -   * The path to the preview image (relative to the 'path' given).
    -   *
    -   * @var string optional
    -   */
    -  public $icon;
    

    Why was the 'icon' removed? This is used in both Panels and DS.

  5. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -82,24 +95,16 @@ public function processDefinition(&$definition, $plugin_id) {
    -    // Add a dependency on the provider of the library.
    -    if (!empty($definition['library'])) {
    -      list ($library_provider, ) = explode('/', $definition['library']);
    -      if ($this->moduleHandler->moduleExists($library_provider)) {
    -        $definition['dependencies'] = ['module' => [$library_provider]];
    -      }
    -      elseif ($this->themeHandler->themeExists($library_provider)) {
    -        $definition['dependencies'] = ['theme' => [$library_provider]];
    -      }
    

    Why was this removed? It was added in #42 in order to address #28.5

  6. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
    @@ -173,7 +78,7 @@ public function defaultConfiguration() {
    -    return isset($this->configuration['dependencies']) ? $this->configuration['dependencies'] : [];
    +    return [];
    

    Ditto

dsnopek’s picture

Only one bit of review when looking at the latest full patch:

+++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinitionInterface.php
@@ -0,0 +1,223 @@
+interface LayoutDefinitionInterface extends PluginDefinitionInterface {

Do we really want/need an interface for a value object? I worry this could limit us changing or evolving the LayoutDefinition, similar to the problems with FormState/FormStateInterface

tim.plunkett’s picture

Issue tags: -Needs committer feedback
FileSize
38.41 KB
56.33 KB

#79
1) Good idea! Changed.
2) See \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies(), this is the responsibility of the caller
3) Just typehinting.
4) Indeed confusing, I simplified this.
5) Yep.
6) Good idea, removed.

#80
1+2) Thanks! I really hated that code anyway.
3+4+5) Restored
6) Switched to using 'config_dependencies' as documented by \Drupal\Core\Config\Entity\ConfigDependencyManager and used by \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies()

#81 Also a very good idea, did that

Already ported my field_layout work to this, and it works 100%. Now going to try converting DS and Panels!

drumm’s picture

(Seeing if a new comment triggers testing here.)

drumm’s picture

Status: Needs review » Needs work

(Sorry for the noise, one more after this.)

drumm’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Worked on converting Display Suite and Panels to the new system, each module has an issue now:
#2821201: Update Display Suite for layout_plugin in core
#2821226: Update Panels for layout_plugin in core

While doing the conversion, I made some changes to the new core module:

Opened #2821189: Allow object-based plugin definitions to be processed in DerivativeDiscoveryDecorator and #2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait as follow-ups in addition to #2818653: Allow object-based plugin definitions to be processed in DefaultPluginManager::findDefinitions()
Added code to handle manually declared layout classes with leading slashes
Allowed arbitrary properties to be set on the definition, the same way as EntityType
Fixed the schema name to be layout_plugin.settings, was layout.settings
Restored a simplified version of getLayoutOptions() to the manager
Added a new getRegionLabels() to the definition object to replace the generated 'region_names' property in contrib.

dsnopek’s picture

Status: Needs review » Reviewed & tested by the community

I don't really have an opinion about the new DerivablePluginDefinitionInterface and ObjectDefinitionContainerDerivativeDiscoveryDecorator, other than to say that derivatives are very important for layout_plugin! So, so long as they work, I'll leave review of those to their independent issues.

But as for the rest, the changes in #82 and #86 look great to me and I don't have any further review. :-) The patches to Panels and DS both look great too.

I think this ready to go up to framework/product owners and committers to look at, so, RTBC!

tim.plunkett’s picture

Issue tags: -Needs change record

Yay! Added a change record https://www.drupal.org/node/2821240

tim.plunkett’s picture

Priority: Normal » Major

I think this qualifies as a major feature, considering the follow-ups are major, and Panels/DS are pretty widely used.

tim.plunkett’s picture

Noticed one further issue while updating field_layout with the recent changes.

+++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutBase.php
@@ -29,9 +29,10 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
-    $build = array_intersect_key($regions, $this->pluginDefinition->getRegions());
+    $build['#content'] = array_intersect_key($regions, $this->pluginDefinition->getRegions());

Because of this change, layouts containing form elements stopped working properly. #content is needed as a key because of the change to hook_theme, switching from 'render element' to 'variables'. But some mechanism is needed to allow forms to be processed by layouts AND the form builder.

tim.plunkett’s picture

+++ b/core/modules/layout_plugin/tests/src/Kernel/LayoutTest.php
@@ -121,7 +153,16 @@ public function renderLayoutData() {
+    $data = ['layout_test_1col_with_form' => $data['layout_test_1col_with_form']];

One mistake in the PASS patch, meant to delete this.

dawehner’s picture

  1. +++ b/core/modules/layout_plugin/config/schema/layout_plugin.schema.yml
    @@ -0,0 +1,6 @@
    +
    +layout_plugin.settings.*:
    +  type: layout_plugin.settings
    

    <3

  2. +++ b/core/modules/layout_plugin/src/DerivablePluginDefinitionInterface.php
    @@ -0,0 +1,37 @@
    + */
    +interface DerivablePluginDefinitionInterface extends PluginDefinitionInterface {
    

    We can remove that later, as its an experimental module, right?

  3. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +    if (!$definition instanceof LayoutDefinition) {
    +      throw new InvalidPluginDefinitionException($plugin_id, sprintf('The "%s" layout definition must extend %s', $plugin_id, LayoutDefinition::class));
    +    }
    

    It would be nice to have that a generic feature of the plugin system

  4. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +
    +    // Keep class definitions standard with no leading slash.
    +    $definition->setClass(ltrim($definition->getClass(), '\\'));
    

    It is a bit weird that we care about that.

  5. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +    $categories = array_unique(array_values(array_map(function (LayoutDefinition $definition) {
    +      return $definition->getCategory();
    +    }, $this->getDefinitions())));
    

    This is quite jerkable code.

  6. +++ b/core/modules/layout_plugin/src/ObjectDefinitionDiscoveryDecorator.php
    @@ -0,0 +1,73 @@
    +/**
    + * Ensures that all array-based definitions are converted to objects.
    + */
    +class ObjectDefinitionDiscoveryDecorator implements DiscoveryInterface {
    +
    

    Can we have a todo to move this to core? This sounds like a great idea!

  7. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutInterface.php
    @@ -0,0 +1,33 @@
    +interface LayoutInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurablePluginInterface {
    

    Given that most layouts won't be configureable, I'm wondering whether layouts should really be configurable by default.

tim.plunkett’s picture

1) I've learned my lesson :)
2) Yes, completely removable
3) We absolutely need more helpers/support for object-based definitions, but I'm not sure how abstractable this is. Will ponder.
4) Yes it is weird, but DS has if ($info->getClass() == DsLayout::class) { and some passed and some didn't, better to keep it clean.
5) Not sure what jerkable means in this context, but I'll take it as a compliment? :)
6) Sure, #2822752: Allow object-based plugin definitions to be created by non-annotated discovery
7) #2749375: LayoutBase incorrectly implements PluginFormInterface and ConfigurablePluginInterface says:

Move ConfigurablePluginInterface to LayoutInterface: In the case of ConfigurablePluginInterface, I think an argument can be made for this being part of the LayoutInterface at all times. This simplifies a ton of logic that could depend on Layouts and removes the need for various instanceof checks. If we agree on this, then the LayoutInterface should be extending the ConfigurablePluginInterface instead of making it optional for all Layout plugins.

This is an issue for the contrib layout_plugin, and @dsnopek explicitly asked that I follow that for this conversion. And honestly it DOES make things easier.

Leaving at RTBC since I'm just adding an @todo

Berdir’s picture

Looked at this for the first time, if things were discussed before, just tell me :)

  1. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +    // Suppress errors because PHPUnit will indirectly modify the contents,
    +    // triggering https://bugs.php.net/bug.php?id=50688.
    +    @uasort($definitions, function (LayoutDefinition $a, LayoutDefinition $b) {
    +      if ($a->getCategory() != $b->getCategory()) {
    +        return strnatcasecmp($a->getCategory(), $b->getCategory());
    +      }
    +      return strnatcasecmp($a->getLabel(), $b->getLabel());
    +    });
    

    Would it be worth to use array_multisort for this to avoid the @stuff? Should also be faster but I guess that makes no difference for the amount of layouts compared to everything else that's going on during plugin discovery.

  2. +++ b/core/modules/layout_plugin/src/LayoutPluginManagerInterface.php
    @@ -0,0 +1,65 @@
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @return \Drupal\layout_plugin\Plugin\Layout\LayoutDefinition|null
    +   */
    +  public function getDefinition($plugin_id, $exception_on_invalid = TRUE);
    

    still technically not allowed to do this :(

  3. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,10 @@
    +/**
    + * Provides a default class for Layout plugins.
    + */
    +class LayoutDefault extends LayoutBase {
    

    do we really need a default and a base class?

  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinition.php
    @@ -0,0 +1,575 @@
    +   */
    +  public function getOriginalClass() {
    +    return $this->originalClass ?: $this->class;
    

    we use the original class in EntityType to be able reliably identify the entity_type_id based on the class. There is no usecase for this here, why have it?

  5. +++ b/core/modules/layout_plugin/tests/modules/layout_test/layout_test.layouts.yml
    @@ -0,0 +1,20 @@
    +layout_test_1col:
    +  label: 1 column layout
    +  category: Layout test
    +  template: templates/layout-test-1col
    +  regions:
    

    in the initial layout module (which I'm still using..), there used to be a type, additionally to the category: https://github.com/frega/layout/blob/master/modules/layout_example/layou...

    The idea was to have different types of templates, like a whole page and something like a small two-column layout that you'd embed somewhere.

    Is there no use case for this in panels/DS? how are they solving this now? is every layout available for everything?

tim.plunkett’s picture

1) This is identical to \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getSortedDefinitions(), except with $a->getCategory() instead of $a['category']. So if you think array_multisort would be a better fit, let's do that in both as a follow-up

2) It's still all over core, is prevalent and necessary throughout the plugin system, and is valid in major IDEs.

3) No we don't.

4) No reason, just copied. Good call.

5) Opened #2822758: Introduce a way to distinguish between different types of layouts

xjm’s picture

Great to see this RTBC! Looks like this needs both framework manager review for the architecture and release manager review for the plan to make the plugin stable, so tagging for both per discussion with @webchick and @effulgentsia.

I think #2811175: [plan] Add layouts to Drupal probably at least needs to have the list of followups and the timeframe for completing them, at least.

dsnopek’s picture

Regarding #93.7 (which @tim.plunkett also responded to in #94):

Given that most layouts won't be configureable, I'm wondering whether layouts should really be configurable by default.

Even though most layout plugin implementations won't be configurable (although, all DS layouts are and all Panels layouts will be once they reach feature parity with D7), all code that uses layout plugins needs to take configurable layouts into account.

A module that wants to use layout plugins to render stuff can conceivably get passed any layout plugin, including configurable ones, and failing to take into account that the plugin might be configurable will cause everything to break as soon as configurable layout is used. (We've encountered this in contrib a couple time already.) So, from the perspective of consumers of this API, all layouts are effectively configurable!

Having LayoutInterface extend ConfigurablePluginInterface prevents all code using layouts from having to do $layout instanceof ConfigurablePluginInterface in order to do something that effectively required in order to use the API.

I kinda went in circles there, but hopefully that isn't total gibberish :-)

dawehner’s picture

I still don't get why the rare case have to dictate the common case. In my naivety I would assume that doing instanceof checks should be always possible.

tim.plunkett’s picture

#97, I expanded #2811175: [plan] Add layouts to Drupal

#98/99, I don't feel strongly either way. Also keep in mind that this is experimental, and we can iterate on that.

alexpott’s picture

Just some thoughts - not a final review at all.

  1. +++ b/core/modules/layout_plugin/layout_plugin.module
    @@ -0,0 +1,28 @@
    +/**
    + * Implements hook_theme().
    + */
    +function layout_plugin_theme($existing, $type, $theme, $path) {
    +  return \Drupal::service('plugin.manager.layout_plugin')->getThemeImplementations();
    +}
    

    Do we want to pass the arguments in?

  2. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +    // Suppress errors because PHPUnit will indirectly modify the contents,
    +    // triggering https://bugs.php.net/bug.php?id=50688.
    +    @uasort($definitions, function (LayoutDefinition $a, LayoutDefinition $b) {
    +      if ($a->getCategory() != $b->getCategory()) {
    +        return strnatcasecmp($a->getCategory(), $b->getCategory());
    +      }
    +      return strnatcasecmp($a->getLabel(), $b->getLabel());
    +    });
    

    Can use array_multisort() instead of the silencing. Bit more complex and less readable though... php--

  3. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,99 @@
    +  /**
    +   * Moves the content so that it can be processed properly by the form builder.
    +   */
    +  public static function processContent($element) {
    ...
    +  /**
    +   * Moves the content back to the location expected by the #theme hook.
    +   */
    +  public static function preRenderContent($element) {
    

    Missing @param and @return.

tim.plunkett’s picture

not a final review at all.

Ominous.

1) Removing the unused params.

2) As I said in #96

This is identical to \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getSortedDefinitions(), except with $a->getCategory() instead of $a['category']. So if you think array_multisort would be a better fit, let's do that in both as a follow-up

3) Fixed.

xjm’s picture

Notes from reading the patch:

  1. +++ b/core/modules/layout_plugin/layout_plugin.info.yml
    @@ -0,0 +1,6 @@
    +name: 'Layout Plugin'
    

    This probably isn't the best user-facing name for the module (as the word "plugin" is overloaded for the average web admin), but since it's experimental, we can make picking the best user-facing name a followup that's part of adding the whole suite of intended modules.

  2. +++ b/core/modules/layout_plugin/src/Annotation/Layout.php
    @@ -0,0 +1,150 @@
    +   * The human-readable category.
    +   *
    +   * @var string
    +   *
    +   * @ingroup plugin_translatable
    +   */
    +  public $category;
    

    What kinds of categories are these?

    Also, I noticed there are a whole lot of public properties, but looking at other instances in core I guess that's normal for plugin annotation classes.

  3. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +   *   The theme handle to invoke the alter hook with.
    

    Typo: handler.

  4. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +    // Keep class definitions standard with no leading slash.
    +    $definition->setClass(ltrim($definition->getClass(), '\\'));
    

    Seems odd to hardcode this in this specific plugin implementation.

  5. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +        $template_path .= '/' . implode('/', $template_parts);
    

    Shouldn't this use DIRECTORY_SEPARATOR instead of /?

  6. +++ b/core/modules/layout_plugin/src/LayoutPluginManager.php
    @@ -0,0 +1,218 @@
    +  public function getCategories() {
    +    // Fetch all categories from definitions and remove duplicates.
    +    $categories = array_unique(array_values(array_map(function (LayoutDefinition $definition) {
    +      return $definition->getCategory();
    +    }, $this->getDefinitions())));
    +    natcasesort($categories);
    +    return $categories;
    

    So based on this it looks like implementations can define any categories they want, and we compile the list from what they've defined.

  7. +++ b/core/modules/layout_plugin/src/ObjectDefinitionContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,32 @@
    + * @todo In https://www.drupal.org/node/2821189 merge into
    + *    \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator.
    

    So we would want to either do this before it's in beta, or provide BC when we do move it into core.

  8. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefault.php
    @@ -0,0 +1,112 @@
    +   * Moves the content back to the location expected by the #theme hook.
    

    Moves the content?

    Ah. It moves the content from 'content' to '#content'. Er. So why is it set in 'content' in the first place? I guess whatever reason could just be documented here.

  9. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinition.php
    @@ -0,0 +1,548 @@
    +   * @todo Make protected after https://www.drupal.org/node/2818653.
    ...
    +   * @todo Make protected after https://www.drupal.org/node/2821191.
    

    Other things that would need to be done before beta, or provide BC.

  10. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinition.php
    @@ -0,0 +1,548 @@
    +  /**
    +   * The template file to render this layout (relative to the 'path' given).
    +   *
    +   * @var string|null
    +   */
    +  protected $template;
    +
    +  /**
    +   * The path to the template.
    +   *
    +   * @var string
    +   */
    +  protected $templatePath;
    +
    +  /**
    +   * The theme hook used to render this layout.
    +   *
    +   * @var string|null
    +   */
    +  protected $theme_hook;
    +
    +  /**
    +   * Path (relative to the module or theme) to resources like icon or template.
    +   *
    +   * @var string
    +   */
    +  protected $path;
    

    I think it would be good to have @Cottser or a theme maintainer review the implementation of the theme bits. Hopefully there is a test implementation or such later in the patch. :) Edit: And indeed there is.

  11. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutDefinition.php
    @@ -0,0 +1,548 @@
    +   * Gets any arbitrary property.
    +   *
    +   * @param string $property
    +   *   The property to retrieve.
    +   *
    +   * @return mixed
    +   *   The value for that property, or NULL if the property does not exist.
    +   */
    +  public function get($property) {
    ...
    +   * Sets a value to an arbitrary property.
    +   *
    +   * @param string $property
    +   *   The property to use for the value.
    +   * @param mixed $value
    +   *   The value to set.
    +   *
    +   * @return $this
    +   */
    +  public function set($property, $value) {
    

    These made me "hmm". Aren't they essentially making every protected property public API? Is this to support extensibility somehow, or?

  12. +++ b/core/modules/layout_plugin/tests/src/Kernel/LayoutTest.php
    @@ -0,0 +1,167 @@
    +    // Assume each layout is contained by a form, in order to ensure the
    +    // building of the layout does not interfere with form processing.
    

    Eh?

Going over https://www.drupal.org/core/experimental#requirements:

No known security or data integrity issues.

The one potential risk for data integrity problems I saw would be if there were bugs with config dependencies. The implementation looked correct (however many hours or days ago I read it), but that's one spot to keep an eye on for this API as it moves from experimental to stable. Couldn't think of any security risks.

No disruption or regression to stable functionality (including being able to remove the experimental module without lasting disruption).

Since the module provides an as-yet unimplemented API, and the only data impacted will be implementations of these plugins, there is no disruption to stable functionality. I did manually test and confirm that the module can be uninstalled. ;) (IIRC InstallTest does not test the experimental module group, or at one point it didn't. I can't remember if we actually fixed that or not.)

Functional test coverage for the primary usecase, with no test failures in HEAD.

The API includes er, has functional and unit test coverage, including a test theme implementation.

Basic API documentation for the minimum requirements, and a plan for completing more comprehensive documentation in the codebase and handbook.

Docs are fine. I made a couple notes above about questions I had while reading the patch (e.g. about the category key).

Ideas/prototypes for user interfaces that are planned and minimally validated.

N/A, no UI.

Resolution of other critical issues that maintainers identify as hard blockers.

Nothing else I found.

Followups to meet the core gates.

Usability: We should have a followup to consider the name for this module in the context of the other intended core module names on the roadmap, as well as considering whether having three modules (versus adding additional functionality to this one) is a concern. That's literally the only user-facing part to review since this is an API prototype.

Accessibility: N/A

Documentation: https://www.drupal.org/node/2619128 exists and is actually pretty thorough. Is it accurate for the core experimental implementation?

Testing: I spent a good hour reading the tests and did not catch any missing coverage.

Performance: No implications I could think of.

Followups to address concerns with the experimental UI and architecture, and iterate on them as needed.

This will depend on @alexpott's review I guess. Edit: and @Cottser's if applicable.

Any other issues identified during planning and review.

There are a few small things in the issue above that might merit followups, e.g. the sorting thingy. I haven't read the issue thoroughly to ensure there aren't other points that might be missing a followup, but plenty of feedback does have followups filed already. There is a higher-level question in that the roadmap for this hasn't gotten "idea" approval, but it does seem promising to me.

A timeframe for completing the remaining work (typically two minor releases), a plan for addressing followups, and roughly which followups must be completed to keep the module in core.

We would expect this to be stable according to #2811175: [plan] Add layouts to Drupal (including the contrib implementations being available at least as a forward dev branch) by 8.5.0-alpha1, around August 2017. I think there is a good chance of success.

So other than checking for other needed followup work, backend and maybe frontend framework manager review, and approval of the overall plan, I didn't see anything that needed to be a blocker, and overall this is in great shape I think.

tim.plunkett’s picture

1) I should describe this on the plan issue, but when this is stable it would cease being a module and move to \Drupal\Core\Layout, making the naming much less consequential

2) There's no telling what $category will be, but it is used for the optgroup of the select list.

Blocks have a similar category, their docblock says "The category in the admin UI where the block will be listed.", which isn't that much more informative.
Adding an @see to \Drupal\Component\Plugin\CategorizingPluginManagerInterface, which is what this is consumed by.
And yes, plugin annotation properties are always public

3) Fixed

4) Opened #2824655: Plugin definitions should store their class in a consistent manner (without leading slashes) to make this generic.

5) \Drupal\Core\Theme\Registry uses '/', and that's where this ends up. So does Views, which this mimics.

6) Yep! That's also identical code to \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getCategories(), but for object-based definitions instead of arrays.

7) Yes.

8) Expanded the documentation a bit.

9) Yep.

10) Fine by me, but this matches how Views does things, and incidentally also how the former core Layout module did it

11) This is absolutely necessary for extensibility, DS uses this. Consider that the only other object-based definition, EntityType, also has these. Yes, it makes protected properties available, but at least you can track WHERE that's being called from.

12) This is related to 8. We need layouts for straight Render API as well as Form API.

if there were bugs with config dependencies.

It is the responsibility of those storing the data of configurable plugins to properly manage dependencies. Field Layout, DS, Panels, etc would need to do this. Core has a PluginDependencyTrait intended for this, it needs to be improved in #2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait

tim.plunkett’s picture

xjm’s picture

I should describe this on the plan issue, but when this is stable it would cease being a module and move to \Drupal\Core\Layout, making the naming much less consequential

So this merits discussion. If that's the plan, I think we should document it explicitly on the classes and interfaces that are going to move, and have an explicit followup issue. Presumably Panels and DS maintainers are in the loop about a planned BC break there, but we want to make sure everyone who implements the API knows that might happen and what they will need to change (hopefully just some use statements). And I guess we should have framework manager signoff on that end goal as well. And yeah, let's make sure the plan reflects that.

xjm’s picture

Mmm, also, what about upgrade paths for configured layout plugins? That could be nontrivial. We ran into this question as well for the datetime_range experimental module. Edit: Though this is less complicated than that since that has plugin backing config storage backing content storage, and this is just plugin backing config storage.

tim.plunkett’s picture

Opened #2824667: Update Layout Plugin handbook documentation for updating the docs.

#107, the plugin IDs won't change, only the classes they point to, so \Drupal::service('plugin.manager.layout_plugin')->clearCachedDefinitions() should be sufficient.

xjm’s picture

@alexpott and @Berdir, do you think we should have a followup to discuss using array_multisort() instead of @uasort() for getSortedDefinitions() here and elsewhere?

alexpott’s picture

There's a number of issues around sorting - see #2757207: Introduce "stable sort by weight" functions to sort arrays (and objects), #2759479: Proof of concept: Replace uasort() with dedicated stable-sort-by-weight functions, where possible and #2522712: Simplify Element::children() stable sort.. Imo since this the @ it should do the array_multisort() since the usage of the silence operator can hide other errors. There's only one runtime usage of @u.?sort in core and thats to sort blocks which I guess is why @tim.plunkett thinks it is okay but in other patches we've been moving to array_multisort() when this occurs. For example the recent criticals around sorting config dependencies.

tim.plunkett’s picture

That sort is admin-runtime not real runtime


So, blocker or follow-up? We'd need a follow-up anyway to fix the core trait.
tim.plunkett’s picture

Opened #2824890: Convert uasort to array_multisort in \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getSortedDefinitions() for the existing code, which this mimics exactly. If this lands first (I hope it does!) we can fix both at once.

alexpott’s picture

  1. The big remaining question for me is whether or not creating an experimental module is the best way to do this. I asked @tim.plunkett in IRC where he thought the plugin and manager would live in Drupal 9 and he replied \Drupal\Core. If we add a service to core with no usages other than tests the code will not be loaded at runtime. And we could add @internal annotations and note that it is experimental and remove these when the classes at not. OTOH we could do this as is now and just make the last step of making this stable in Drupal 8 moving everything to core and gutting all the code from the module apart from a very thin BC layer. I'm not sure which is best but given recent discussions in #2779647: Add a workflow component, ui module, and implement it in content moderation we should seriously consider putting this straight into \Drupal\Core as this will result in less busy-work. Before starting on that I think we need a +1 from @catch and @xjm to ensure we have consensus.
  2. I think it is fine to address the array sorting in a followup - I think the error suppression sucks but as long as the follow up exists we're good.
  3. The amount of documentation in the code is really nice to see - makes reading the patch way easier
  4. +++ b/core/modules/layout_plugin/src/Plugin/Layout/LayoutInterface.php
    @@ -0,0 +1,33 @@
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @return \Drupal\layout_plugin\Plugin\Layout\LayoutDefinition
    +   */
    

    Mixing @inheritdoc and other stuff doesn't work :(

  5. +++ b/core/modules/layout_plugin/tests/modules/layout_test/css/layout-test-2col.css
    @@ -0,0 +1,16 @@
    +
    ...
    +
    

    Unnecessary empty lines

alexpott’s picture

+++ b/core/modules/layout_plugin/layout_plugin.module
@@ -0,0 +1,26 @@
+/**
+ * Implements hook_theme().
+ */
+function layout_plugin_theme() {
+  return \Drupal::service('plugin.manager.layout_plugin')->getThemeImplementations();
+}

This is the one problem with putting this straight into core. We'd need to move this to system.module. (from @tim.plunkett in IRC)

I think that making this change is okay as opposed to the issues of moving all the code around and maintaining an experimental module that does not need to exist.

catch’s picture

+++ b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-1col.html.twig
@@ -0,0 +1,14 @@
+{#
+/**
+ * @file
+ * Template for an example 1 column layout.
+ */
+#}
+<div class="layout-example-1col clearfix">
+  <div class="region-top">
+    {{ content.top }}
+  </div>
+  <div class="region-bottom">
+    {{ content.bottom }}
+  </div>
+</div>
diff --git a/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig

diff --git a/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig
new file mode 100644

new file mode 100644
index 0000000..11433ee

index 0000000..11433ee
--- /dev/null

--- /dev/null
+++ b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig

+++ b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig
+++ b/core/modules/layout_plugin/tests/modules/layout_test/templates/layout-test-2col.html.twig
@@ -0,0 +1,14 @@

@@ -0,0 +1,14 @@
+{#
+/**
+ * @file
+ * Template for an example 2 column layout.
+ */
+#}
+<div class="layout-example-2col clearfix">
+  <div class="region-left">
+    {{ content.left }}
+  </div>
+  <div class="region-right">
+    {{ content.right }}
+  </div>
+</div>

There aren't any real layouts in the patch yet, which is fine. However I'm not sure about defaulting to a theme hook + template per layout. It's going to result in a lot of theme hooks.

Both of these templates are the same, just with different class names. I'd expect the majority of layouts to be like this, but with a varying number of regions on top.

So why not default to a single template with a wrapper, that loops over regions and the the layout name as a theme suggestion?

If there's a layout that deviates from that pattern it could still provide its own template/theme hook.

dsnopek’s picture

Both of these templates are the same, just with different class names. I'd expect the majority of layouts to be like this, but with a varying number of regions on top.

So why not default to a single template with a wrapper, that loops over regions and the the layout name as a theme suggestion?

While this would make an interesting contrib module for people who don't want to write Twig/HTML, I don't think it makes sense as a default.

Most layouts in practice aren't just a variable number of columns, but also include rows, and have things that can be configured by the user. Depending on how the layout is implemented (ie. which grid system is used, or how the custom CSS is done) the markup for this will have a different structure. So, the only practical way to implement most layouts, would be to describe the full structure of the HTML markup in YAML. In the default case, I'd really prefer not to invent another way to express HTML - in Drupal 8, at least to me, it seems like if you want to write HTML, you should create a Twig template.

Here's some example real world layout templates in contrib right now...

Radix Layouts (based on Bootstrap - needs 'container' and 'row' div's):

http://cgit.drupalcode.org/radix_layouts/tree/plugins/layouts/radix_hews...

http://cgit.drupalcode.org/radix_layouts/tree/plugins/layouts/radix_sutr...

Display Suite (markup is highly configurable):

http://cgit.drupalcode.org/ds/tree/templates/ds-3col-stacked-fluid.html....

Bootstrap Layouts (also based on Bootstrap - decided to completely omit regions that are empty, which is a valid choice, but different than Radix):

http://cgit.drupalcode.org/bootstrap_layouts/tree/templates/bootstrap-6-...

Panels (custom CSS with layouts about as simple as they can be made):

http://cgit.drupalcode.org/panels/tree/layouts/threecol_33_34_33_stacked...

http://cgit.drupalcode.org/panels/tree/layouts/twocol/panels-twocol.html... (even Panels' twocol is a little more complicated, because you can configure the CSS ID - for parity with D7, you'll eventually be able to configure the CSS classes too)

Anyway, my point is really that complex HTML/templates for layouts is really the rule. Super simple layouts like used in the tests are the exception. We hope that dynamic layout builders will eventually be a thing, whether that's in the UI, or even a special syntax in YAML, but based on what people are doing in contrib right now (and have been doing in D7 a long time), I don't think that makes sense as a default.

Also, putting these in templates makes it easy to replace them in the theme. If you're using Panels, and your theme is based on Bootstrap, you probably want to replace the templates for any Panels layouts you use to be based on Bootstrap, so that they use the same responsive behavior and breakpoints as you setup in your Bootstrap variables. With a dynamic layout builder of some kind, that's also a solvable problem, but more complex.

tim.plunkett’s picture

Assigned: Unassigned » catch

I happen to agree with @dsnopek 100%.
@catch does that answer your question?

Additionally, @alexpott raised the question of lib vs module. We've had this discussion before in this issue, and flip-flopped several times.

To me, it is better to have the not-yet-solidified interfaces in an experimental module than to put them into core/lib marked as @internal and un-internal them later.
Contrib using this functionality is already depending on a separate module for this, this is no different. If it were a subset of a whole module's functionality that we were moving in, I'd say this approach was wrong. But this is the whole module.

catch’s picture

Assigned: catch » Unassigned

I don't like the potential explosion of theme hooks, but don't see a way 'round it if each layout needs its own template.

Overall I'd lean more towards having this in core than a module, but as with the workflow patch we're seeing complications when we try to do that. Still haven't reviewed the whole patch so unassigning.

alexpott’s picture

For me since this is replacing https://www.drupal.org/project/layout_plugin and using the same module name I really think that this is a bad idea. It creates all sorts of problems for both consumers of the API having to detect whether or not the core experimental or contrib module is being used - for no gain since eventually the core experimental module is going to be gutted and moved to core. If we move everything straight to core then we completely avoid this scenario and sites have to remove the layout plugin module because panels, ds etc can just migrate to core/lib and be done.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Discussed with @catch on IRC and we agreed to go straight to core/lib because of all of the problems with module name clashes between core and contrib and breaking things the least number of times. In #2779647: Add a workflow component, ui module, and implement it in content moderation, I added

+ * @internal
+ *   The workflow system is currently experimental and should only be leveraged
+ *   by experimental modules and development releases of contributed modules.

To all the class docblocks - we should do something similar here.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Thanks for the feedback!

catch’s picture

This also means that we save one set of breakages for modules depending on this - i.e. if it goes in as a module, then again moves to core/lib, modules get broken at least twice as opposed to at least once.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
61.35 KB
26.21 KB

I'm having déjà vu all over again!

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work

Forgot the @internal parts, hah. Will get to it first thing tomorrow

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
5.87 KB
62.89 KB

There we go.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs release manager review

As per #113 we still need consensus from @xjm on the approach to move into core/lib.

For me core/lib is the correct approach because it values the 10,000 existing sites using the contrib layout_plugin module rather than theoretical problems from being able to autoload experimental code on a live site. If non-experimental code does that then it is a bug in that code.

effulgentsia’s picture

  1. +++ b/core/modules/system/system.module
    @@ -161,7 +161,23 @@ function system_help($route_name, RouteMatchInterface $route_match) {
    +    \Drupal::service('plugin.manager.layout')->getThemeImplementations()
    

    I like the move of classes to core/lib, but I'm concerned about this being in system.module. It means the theme registry will be filled with theme hooks for every provided layout, even if no layout rendering module (like Panels, DS, etc.) is enabled. It also means that enabling a module like Panels or DS won't trigger any UI warning that it depends on an experimental core feature. I suggest moving just this line, and the service definition, to an experimental module.

  2. +++ b/core/core.services.yml
    @@ -633,6 +633,9 @@ services:
    +  plugin.manager.layout:
    +    class: Drupal\Core\Layout\LayoutPluginManager
    +    arguments: ['@container.namespaces', '@cache.discovery', '@module_handler', '@theme_handler']
    

    Per above, this is the other hunk to move into an experimental module. I think the module could perhaps then be named layout_discovery? I think the service ID can stay the same though.

  3. +++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
    @@ -0,0 +1,221 @@
    +        $hooks[$definition->getThemeHook()] = [
    +          'variables' => [
    +            'content' => [],
    +            'settings' => [],
    +            'layout' => [],
    +          ],
    ...
    +++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
    @@ -0,0 +1,125 @@
    +    $build['#content'] = array_intersect_key($regions, $this->pluginDefinition->getRegions());
    ...
    +    $element['content'] = $element['#content'];
    +    unset($element['#content']);
    ...
    +    $element['#content'] = $element['content'];
    +    unset($element['content']);
    

    I don't like this. I read the issue comments to see how this came about, and it looks like it was done to avoid needing a hook_theme_registry_alter() implementation that inserts a preprocess function for every layout. However, what if we registered these theme hooks as suggestions (i.e., 'base hook' => 'layout')? Couldn't we then get *_preprocess_layout() functions registered for free? Which would allow for both the template_preprocess_layout() that we need, plus make room for modules to implement hook_preprocess_layout() if they need to. Note that I'm not disagreeing with #116: I agree that most layouts need their own template file: I'm only saying that they can and should share common preprocess functions, which would allow for the above code to be cleaned up. However, to be honest, I haven't fully followed all of the changes to how the theme registry works for theme hook suggestions between Drupal 7 and Drupal 8, so maybe there's some implementation reason why this idea isn't feasible. But if that's the case, I'd like to know what that reason is before we proceed with the above hack. Then again, I'd also be ok with this getting punted to a follow-up, so long as folks wouldn't be upset with the corresponding disruptions (e.g., the layout and theme hook names changing to follow a layout--foo instead of layout-foo pattern) getting committed later.

tim.plunkett’s picture

Okay, the proposals are now

  1. core/modules as experimental (xjm's preference, in #104)
  2. core/lib with @internal (alexpott's preference, in #125)
  3. both (effulgentsia's preference, in #128)

I leave it to the 3 committers that are proposing each to decide that. I'm here if you need some insight, but I have no strong opinion.

As for #127.3, it gets ugly.

We'd need to have an empty 'layout' theme hook that does nothing but allow preprocessing.
Or we'd need to have a functioning 'layout' theme hook that is generic, which we don't want.


Note that #104, #125, and #128 are all functionally equivalent.

Status: Needs review » Needs work

The last submitted patch, 128: 2296423-layout_plugin-128.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Side effects of the changes. Not going to bother fixing it until an approach had been decided upon

webchick’s picture

IMO, @effulgentsia's proposal seems like it makes everyone happy since:

1) It keeps the various dread warnings about experimental functionality intact (which xjm will like)
2) It avoids all the nasty consequences in implementing modules when that code eventually moves from layout_disovery to system; simply a cache clear, no? (which catch/alexpott will like)

I like it. ;) Now there are 4 core committers with 3 different opinions. :D

larowlan’s picture

If we do item 2, we could put items in a Drupal\Core\Experimental namespace

Drupal\Core\Experimental\...

then it is clear they're experimental.

e.g. Drupal\Core\Experimental\SomeApi\SomeClass

When we move them to stable, we end up with

Drupal\Core\SomeApi\SomeClass

and we leave the old class behind, extending the new one, as an empty shell and mark it as deprecated.


namespace Drupal\Core\Expirmental\SomeApi;

use Drupal\Core\SomeApi\SomeClass as BaseClass;

/**
 * Does something amazing.
 *
 * @deprecated  - to be removed in 8.4.x - use Drupal\Core\SomeApi\SomeClass
 */
class SomeClass extends BaseClass {

}

We did something similar with BrowserTestBase, its old skeleton is still present in the Drupal\simpletest namespace

alexpott’s picture

I didn't think Drupal\Core\Experimental\ was a good way to go because that just means more breakage for no reason when we move the classes to the correct place. But then I realised that you're proposing to leave them behind. I think we should leave behind till until 9.x. So contrib can upgrade as they want - they never have to version juggle in 8.x - once both the patch version and next minor version have the non-experimental classes they can move to them. This is a good idea. And combining that with @effulgentsia's proposal in #127 (also a good idea) seems clever.

So I'm +1 to #132 and #127.

The one reservation I had when thinking about things was that the service name might exist in the existing contrib module... however the service name is plugin.manager.layout_plugin. Which brings me to something I've been thinking for a while. We really should have a naming standard for services where they begin with their provider.whatever to avoid name clashes. Which means that I think the service should be named: core.plugin.manager.layout since at some point we're going to remove the layout_discovery module and put it into core. We can document this in the services.yml file.

tstoeckler’s picture

We really should have a naming standard for services where they begin with their provider.whatever to avoid name clashes. Which means that I think the service should be named: core.plugin.manager.layout since at some point we're going to remove the layout_discovery module and put it into core. We can document this in the services.yml file.

Well we currently have a standard of all plugin managers being called plugin.manager.*, so - while I don't necessarily disagree your proposal is good - that would break the existing standard, which we are actually pretty consistent with in core at the moment.

Berdir’s picture

@alexpott: In general, I agree with prefixing things (also plugins and so on). However, the plugin.manager prefix is special, see \Drupal\Core\Plugin\PluginManagerPass::process(), we don't *have to* use that but then we need to explicitly add a tag.

Given that, I'd recommend plugin.manager.core.layout.

alexpott’s picture

Okay +1 to #135... for plugin managers we go with plugin.manager.PROVIDER.whatever... I thought adding the tag was okay - less magic but this is also okay.

alexpott’s picture

After discussing with @catch for a bit about the core/experimental idea - he's moved my position back to "just means more breakage for no reason" since contrib will have to update twice and there's no real difference with having it in an experimental module then. I think we should be prioritising the least number of breaks for people experimenting once we've worked out how to definitely not affect live sites. So that seems fulfilled by #127. I remove my support for #132.

Damn BC and experimental is tricky.

tim.plunkett’s picture

In order to continue with #127, we need a generic 'layout' theme hook. So we get to do what @catch wanted in #115 after all.
I still agree with @dsnopek that 99% of layout authors will want their own template, but if they are lazy then they can have this one.

As of #125 I renamed the service to plugin.manager.layout anyway (away from plugin.manager.layout_plugin). Begrudgingly adding the .core because you's asked for it, not because I think it's in scope to redefine our plugin manager naming conventions here.

Status: Needs review » Needs work

The last submitted patch, 138: 2296423-layout_plugin-layout_discovery-138.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » xjm
Status: Needs work » Needs review

Random fail from #2828045: Tests failing with unrelated „Translation: The French translation is not available”

@alexpott's comment in #137 implies that @catch is also in favor of @effulgentsia's proposal, as is @webchick, so the only one left is @xjm

dsnopek’s picture

In order to continue with #127, we need a generic 'layout' theme hook. So we get to do what @catch wanted in #115 after all.
I still agree with @dsnopek that 99% of layout authors will want their own template, but if they are lazy then they can have this one.

If this is going to be suggestions on a single theme hook (ick!) then at the very least we need to make sure we're packing enough information to allow people to preprocess the theme hook and target a specific layout, since it's super normal for a module providing layouts to also preprocess some or all of them. I didn't have time to look at the patch on #138 to check if that's the case or not, but without that we'd make current use cases in contrib impossible.

For what it's worth, I think having a single theme hook with suggestions is going to make it harder and more confusing to use this API in practice, but it seems like that's the way this is going :-/

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
@@ -142,16 +148,16 @@ public function processDefinition(&$definition, $plugin_id) {
+    $hooks['layout'] = [
+      'render element' => 'content',
+    ];
...
       if ($template = $definition->getTemplate()) {
         $hooks[$definition->getThemeHook()] = [
-          'variables' => [
-            'content' => [],
-            'settings' => [],
-            'layout' => [],
-          ],
+          'render element' => 'content',
+          'base hook' => 'layout',
           'template' => $template,
           'path' => $definition->getTemplatePath(),
         ];

In the last RTBC patch, there were N theme hooks for every layout that specifies a template.

Now there are N+1.

There is always a 'layout' theme hook present, but it will almost never be used.

Specifying it as a 'base hook' does only a couple things:

  • Copies any explicit 'includes' over (none in this case)
  • Gives them a shared template_preprocess_layout() (why we want it)
  • Adds 'layout' as yet another suggestion for hook_theme_suggestions_HOOK() (not important)

This is NOT converting the theme hooks for layouts to suggestions, they are full fledged theme hooks.

dsnopek’s picture

Ah, ok, thanks! That sounds good. Sorry, I didn't actually read the last patch, just skimmed the comments (I'm on paternity leave now - so, not much time)

If we're still able to use full fledged theme hooks (as opposed to converting all to single theme hook with suggestions which is what I thought was happening) then this is totally fine and contrib will be able to keep on doing what it's doing :-)

tim.plunkett’s picture

The only change for existing contrib is that their preprocess will change from
mymodule_preprocess_mylayoutname()
to
mymodule_preprocess_layout__mylayoutname()

which *looks* very suspiciously like a suggestion, but is not.

\Drupal\Core\Theme\Registry::completeSuggestion() is used to process this, which once again seems like it's a suggestion, but it's just confusingly named.
As the calling code says:

          // Look for a previously defined hook that is either a less specific
          // suggestion hook or the base hook.
andypost’s picture

I think having experimental module is good idea

1) It will allow to bring default one col layout as BC for fields so it can be used for default "layout" theme hook
2) Theme suggestions are runtime so we have hew hooks https://www.drupal.org/node/2100775
3) Later will allow more layouts to polish in minors

+++ b/core/modules/layout_discovery/templates/layout.html.twig
@@ -0,0 +1,18 @@
+ * Template for a generic layout.
...
+{% set classes = [
+  'layout--' ~ layout.id|clean_class,
...
+<div{{ attributes.addClass(classes) }}>
+{% for region in layout.getRegionNames %}

+++ b/core/modules/system/tests/modules/layout_test/templates/layout-test-1col.html.twig
@@ -0,0 +1,14 @@
+<div class="layout-example-1col clearfix">
+  <div class="region-top">

why not move this "one col" to module itself? and move test module here as well?

andypost’s picture

Also having default fallback "Reset" layout very useful

effulgentsia’s picture

As of #125 I renamed the service to plugin.manager.layout anyway (away from plugin.manager.layout_plugin). Begrudgingly adding the .core because you's asked for it, not because I think it's in scope to redefine our plugin manager naming conventions here.

I agree with @tim.plunkett. I don't see why this issue should start prefixing .core when no existing core plugin manager service IDs currently do. What if we rename the module to simply layout? The info.yml could still retain the human-friendly name of 'Layout Discovery' if we think that's helpful.
https://www.drupal.org/project/usage/layout currently lists 0 sites using the 8.x version, and with no commits in 3 years, I doubt those branches even work at all, so I don't see much of a conflict problem with that. Then the service ID could be plugin.manager.layout, which would adhere to our current naming patterns both while it's in layout.module, and when it eventually moves to core.services.yml.

+++ b/core/config/schema/core.data_types.schema.yml
@@ -361,6 +361,13 @@ display_variant.plugin:
+layout_plugin.settings:
+  type: mapping
+  label: 'Layout settings'
+
+layout_plugin.settings.*:
+  type: layout_plugin.settings

What about renaming these from layout_plugin to layout as well? Also, it doesn't look like we have any config objects anywhere testing the .* one, or do we? There's the schema in layout_test.schema.yml, but that's explicit rather than a reference to layout_plugin.settings.*, and even that one, is that schema ever tested, since I think testRenderLayout() bypasses actual configuration read/write?

tim.plunkett’s picture

I'm very hesitant to call this layout.module. To me, and many others, that name implies a UI. Specifically, a UI for building layouts (aka "flexible layout builder"). Note that's what layout.module in D7 contrib did.

If layout.module *were* to exist, it would like be stored as a config entity with a plugin deriver, similar to how menu entities and menu blocks work.
So in the schema I'd prefer to keep "layout" for that eventuality, and stick with layout.plugin here for clarity.

As far as layout_plugin.settings.* is concerned, I don't know that we need explicit tests for this, but it is an extremely good practice IMO, and one I think we're mistaken in skipping before. For example, #2815829: Adding or editing a block through the UI saves the entity twice is a follow-up to #2811519: Blocks do not appear after being placed with the Rules module enabled (or other missing schemata for Condition plugins), both are trying to add this to save contrib from itself:

condition.plugin.*:
  type: condition.plugin

In fact, my addition of layout_plugin.settings.* was explicitly called out in #93.1 as a good thing.


For the service name, I really don't care. I think adding .core. is out of scope and could be discussed elsewhere, but it doesn't hurt anyone to add it here.
It's a trivial change to fix, it only is used in 3 places:

$ ag plugin.manager.core.layout
core/modules/layout_discovery/layout_discovery.module
25:  return \Drupal::service('plugin.manager.core.layout')->getThemeImplementations();

core/modules/layout_discovery/layout_discovery.services.yml
2:  plugin.manager.core.layout:

core/tests/Drupal/KernelTests/Core/Layout/LayoutTest.php
33:    $this->layoutPluginManager = $this->container->get('plugin.manager.core.layout');

I can reroll without .core. if needed, or it could be fixed on commit.

tim.plunkett’s picture

#145.1 Yes, that's what #2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts is for

#145.2 Theme suggestions are runtime, but these are full theme implementations using a base hook.

#145.3 We don't want to add any real layouts yet to runtime until #2822758: Introduce a way to distinguish between different types of layouts is resolved.

xjm’s picture

So I've read all the comments since my previous review and spent a good time thinking about this. I think we have a number of separate questions that affect some of the same things, and so they are being discussed together. But they are separate questions:

  1. Does the code for this API belong in a module, or in \Drupal\Core?
  2. How do we denote that the code is experimental for developers and site owners?
  3. How do we avoid namespace collisions with contributed and custom projects?
  4. How do we minimize disruption for code, configuration, and data that may depend on the API?

Honestly, I'm still stuck on #1. I don't think this belongs in core/lib (not in Drupal 9 either). @tim.plunkett has said that it belongs there, but I am not sure what the case is for it being there.

Let's take a look at what's in core/lib/Drupal/Core/:

Access                    Entity                    PhpStorage
Action                    EventSubscriber           Plugin
Ajax                      Executable                PrivateKey.php
Annotation                Extension                 ProxyBuilder
AppRootFactory.php        Field                     ProxyClass
Archiver                  File                      Queue
Asset                     FileTransfer              README.txt
Authentication            Flood                     Render
Batch                     Form                      RouteProcessor
Block                     GeneratedLink.php         Routing
Breadcrumb                GeneratedNoLink.php       Serialization
Cache                     GeneratedUrl.php          Session
CacheDecorator            Http                      Site
Command                   Image                     SitePathFactory.php
Composer                  ImageToolkit              StackMiddleware
Condition                 Installer                 State
Config                    KeyValueStore             StreamWrapper
Controller                Language                  StringTranslation
CoreServiceProvider.php   Link.php                  Template
Cron.php                  Locale                    Test
CronInterface.php         Lock                      Theme
Database                  Logger                    Transliteration
Datetime                  Mail                      TypedData
DependencyInjection       Menu                      Update
DestructableInterface.php Operations                Updater
Diff                      PageCache                 Url.php
Discovery                 ParamConverter            Utility
Display                   Password                  Validation
DrupalKernel.php          Path
DrupalKernelInterface.php PathProcessor

Most of those things are low-level "you can't have a Drupal without this" functionality. Forms, routing, content rendering, data storage, configuration. Lower subsystems and utilities. There are a few exceptions. (I've thought numerous times over the past year that we should have put BTB and kin in a module as well.)

I was surprised to see Block in there since there is also an optional Block module. Then I opened a class in there (in BlockBase) and saw this:

namespace Drupal\Core\Block;

use Drupal\block\BlockInterface;

So from that the Block module is not just a Block UI module, and we have something in Drupal core that depends on code in an optional module. Oopsie. That could lead to some interesting errors.

Do you need to have a layout API on every Drupal site? I'm not sure you do. Do we have any case for code in core/lib to depend on Layout API code? The two related things I see in there are Block (but see caveat above) and Theme (I don't think we want our theme system of the future to depend on these plugins... or do we)?

I think things should be in modules if:

  • They are optional.
  • A site administrator might want to disable them.
  • They provide a specific user interface (this is kind of a sub-point of the previous point).

I think things should be in core/lib if:

  • If there are other things in core/lib that might need to depend on them. (Circular but true.)
  • They are required for all sites.
  • There is never a case for a site administrator to disable them.

Our answers to questions #2-4 depend on #1, so let's evaluate what the answer to #1 should be. (I have thoughts on questions #2-4 for either answer to question #1, but don't want to add even more of a wall of text here if there is a strong answer to #1.)

(Aside: as much as I am in favor of the continuous upgrade path, I don't think we should make decisions for Drupal 8 based only on what Drupal 9 might look like. Really, we don't know yet. We might want to get rid of System module finally somehow, or make our namespacing truly unique and safe, or even decide we want to do away with the module system as it currently exists, but I wouldn't make a decision for this issue based only on such far-off ideas.)

xjm’s picture

Oh and I guess question #5 might be "What should its name actually be?"

xjm’s picture

Also just noticed that we do have a README.txt in there, which says:

Code in the Drupal\Core namespace represents Drupal Subsystems provided by the
base system.  These subsystems MAY depend on Drupal Components and other
Subsystems, but MAY NOT depend on any code in a module.

Each Subsystem should be in its own namespace, and should be as self-contained
as possible.

tim.plunkett’s picture

Opened #2829680: BLOCK_LABEL_VISIBLE is defined on the wrong interface to fix the Drupal\Core\Block vs Drupal\block issue.


Here are my reasons for layout being in \Drupal\Core.

As stated on the parent issue, there are two issues "to validate the implementation".
Both would initially add experimental modules at first.
But both are altering existing functionality in a way that we would eventually want baked into core itself, not living forever as a module containing only alters. Once stable, ideally each would be merged directly into the system they are altering.

#2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts would need to rewrite parts of Field UI, but also code in \Drupal\Core\Entity.
#2811179: Allow themes to provide Layout API integration would need to rewrite parts of system.module itself.

I highly doubt we'd want to add another module as a dependency of system.module, and we couldn't have \Drupal\Core\Entity using module code as would be necessary.

Most of those things are low-level "you can't have a Drupal without this" functionality

Yes this is true. Except for all the things that aren't essential. Which happen to be the ones I'm changing. Maybe they don't truly belong in Drupal\Core and should be somewhere else (\Drupal\MaybeNotEssentialButNotAModule ?)
But if we're going to continue to have things like theme regions and entity displays outside the module system, then to improve them we need the Layout API available as a subystem as well.

There are no UIs changed here, and even the follow-up issues have their UIs and their functionality working separately. For example, the field layout one either changes \Drupal\Core\Entity or field_ui.module. There's already a pre-existing split there, but both halves need to be modified.
In the theme layout issue, the underlying code modifies the results of system_get_info(), and the UI is left for the Block UI to handle.


The tl;dr is the answer to your 5 questions.

  1. It belongs in \Drupal\Core, either now or after becoming stable
  2. By any of the 3 approaches above
  3. By the second two proposals (straight into \Drupal\Core or the hybrid with "layout_discovery")
  4. Same as #3
  5. Either Drupal\Core\Layout, or that plus "layout_discovery" (the conflict with contrib has put me off on the original approach)
tim.plunkett’s picture

Per discussion in IRC with @xjm, expanding the @internal docs to explicitly link to the experimental code policy.

Status: Needs review » Needs work

The last submitted patch, 154: 2296423-layout_discovery-154.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Random fail from DrupalCI, and now it's retesting twice. First retest passed!

tim.plunkett’s picture

@xjm points out that one of the kernel tests in Drupal\KernelTests\Core\Layout depends on layout_discovery, moving that to Drupal\Tests\layout_discovery\Kernel.

xjm’s picture

Thanks @tim.plunkett. The potential of entity displays depending on this API matches the requirement for things in core/lib to only depend on other things in core/lib, and so supports this belonging in \Drupal\Core in 8.x at least, since there is not currently any plan to move entity display functionality into a module.

I talked about this more with @tim.plunkett in IRC.

Things I like about this approach:

  • Not trying to move code around between the initial and stable implementations. I think that, in general, experimental modules should remain in the module that we added during the D8 cycle (e.g. DateTime Range). Otherwise, it creates lots of headaches, even for experimental code. So as a best practice, I think getting the name and code path right (as best we can think) should be on our experimental module checklist.
  • layout_discovery is a better name. (I didn't like having "plugin" in a module name.)
  • layout_discovery plus core library has less risk of namespace collisions with previous implementations.
  • Code is not loaded without the service, and the service is in an experimental module. So most extending modules still add a dependency on the experimental module (or have to go out of the way to implement their own) and site owners still get an experimental module warning. Thanks @tim.plunkett for explaining this bit; I had only been considering the hooks/theme support being in the proposed module and it seemed "meh", but providing the service there actually makes a ton of sense.
  • Having a path forward to have entity displays implement this API in core in 8.x, with BC.
  • Clearly documenting expectations for an experimental module on each class.
  • Explicitly indicating that the classes are not (yet) part of the public API.

Things I don't like:

  1. Abusing @internal for something that is actually intended to be public API.
  2. Further confusing what our public API is because of #1. "Please extend and test this but keep in mind it's pre-release" is the opposite of "This is internal and don't use it", and setting a precedent that contrib not only can but should use @internal code is a bummer.
  3. Having to rely only on the @internal notation to communicate that the code will not necessarily provide BC. At least with experimental modules, we have the notion that they have a different release number than stable core.
  4. Adding (more) stuff that is experimental or optional to core/lib in general. I just don't like the precedent it sets.
  5. Having to figure this out for optional code in core/lib when we already have a solution for modules.

To mitigate #1, we should remove the @internal tags only in a minor, with a change record, once the API is stable. Increasing the visibility of code from @internal to public in a minor is equivalent with an API addition and allowed by semver. #2 is more of a problem, and I don't know how to solve it.

#3 is already mitigated by having the discovery module, and well we never got actual separate version numbers for experimental modules working anyway. (I did like @larowlan's idea of an experimental namespace but apparently @catch did not.) I noticed that api.d.o doesn't even surface or format @internal in a useful way, but then, you don't get any special message on code that is actually in experimental modules either. (It used to hide @internal code completely, so at least we don't have that to contend with anymore.)

For #4, I guess the solution would be for committers to agree "Don't use it as a precedent", but we already have another issue in the RTBC queue that wants to do the same thing. I guess, at the least, we need to have a really compelling reason that the code belongs in core/lib and not a module, and a really strong expectation that it is going to be a stable, required part of core in 9.x.

So, overall, I guess I'm in agreement that the current approach is better than the alternatives, despite the drawbacks, and that most of the drawbacks are sufficiently mitigated. But let's be careful in similar issues to always ask if the code really needs to be in core/lib and whether there is any possible impact on installed sites.

Thanks @tim.plunkett for taking the time to work through this with us.

xjm’s picture

Assigned: xjm » Unassigned
tim.plunkett’s picture

FileSize
41.58 KB

Since originally being RTBC'd in #87 by @dsnopek, this has bounced around a lot.
The committers seem to have reached consensus on the direction.

So, here's an interdiff between the patch in #86 and #157!

xjm’s picture

The CR and handbook should be updated for the current approach (totally makes sense to wait on that until the issue has settled). I think it would be worth mentioning that the core plugin type is declared as internal while it's under development because there's no BC promise yet, but intended to be public API, and that modules wishing to implement the API should declare a dependency on the (currently experimental) discovery module.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

I spent a bit of time reading through 157 today, and this looks very convincing to me. I'm very happy to RTBC this.

Eclipse

alexpott’s picture

Before this issue is committed we need to address @xjm's points in #161 - so we need to update policy docs and the CR.

tim.plunkett’s picture

Updated the CR: https://www.drupal.org/node/2821240
The handbook already has a dedicated follow-up: #2824667: Update Layout Plugin handbook documentation

https://www.drupal.org/core/experimental needs to be updated with something reflecting #158
However I'm not clear on whether we do want to document this as a precedent, or not.

Maybe something like this?

## Experimental subsystems
A "subsystem" is code that lives in the \Drupal\Core namespace. Unlike modules, subsystems cannot be separately versioned. Instead when an experimental subsystem is added, all code is marked @internal to denote that there is no promise for backwards compatibility. Once the code is stable, the @internal tag is removed.

xjm’s picture

Before this issue is committed we need to address @xjm's points in #161 - so we need to update policy docs and the CR.

I only meant update the change record to describe the changes being made, including the expectation of stability/public API vs. internal for this API only in this issue's CR only; and update https://www.drupal.org/node/2619128 to use the correct module name etc. (because it's really confusing otherwise).

I definitely do not want to update any policy docs as part of this issue, because as I've said, I do not want this to be a basis for a policy, and we should avoid it as much as possible.

alexpott’s picture

So I misinterpreted @xjm's comment

The CR and handbook should be updated for the current approach

thinking that this meant the experimental module page needed to be be updated to explain what experimental code was doing in core/lib. I'm ok with not changing the handbook in order to make progress in this issue. However, in my mind, not documenting on the experimental page that experimental code can be found in core/lib in order to not set a policy about this feels awkward. Because we have experimental code in core/lib. But discussing this elsewhere is best.

dsnopek’s picture

Reviewing the interdiff on #160:

+++ b/core/config/schema/core.data_types.schema.yml
@@ -361,6 +361,13 @@ display_variant.plugin:
+layout_plugin.settings:
+  type: mapping
+  label: 'Layout settings'
+
+layout_plugin.settings.*:
+  type: layout_plugin.settings

If we're no longer calling anything 'layout_plugin' internally, should this just be 'layout.settings' and 'layout.settings.*'? I think that's what we used in the last patch that had the code in core/lib rather than a module. (Of course, having it as 'layout_plugin.settings.*' would help with migrating from the contrib module, but would be confusing without the history.)

Otherwise, this all looks good to me! Thanks @tim.plunkett for continuing to push this forward!

tim.plunkett’s picture

I purposefully did not rename the schema because there is a distinct possibility that we end up with something like a flexible layout builder that would need a config object, and that layout.* would be better for it.
I don't feel strongly either way.

alexpott’s picture

So I don't know if this is a problem or not but if you have both layout_discovery and layout_plugin enabled everything blows up. I'm pondering about how a module like Display Suite moves to the core plugin type once the core part is stable. It can't disable the layout_plugin module because it does not know if there is another module that depends on it. Or does it depend on a new version of layout_plugin that is no code but just exists so that things can transition to core's plugin. And if we do that then all things that depend on layout_plugin need to add be updated. And ideally we'd have this all mapped out and documented in the CR.

So, I think we have a problem but one that can be mitigated by having a good plan and communicating it. My plan would be to:

  1. Create an 8.x-2.x release of layout_plugin with no functionality
  2. Add dependencies to on the 8.x-1.x branch of layout_plugin to Display Suite and Panels
  3. When, for example, Display Suite uses stable core layout plugins update there dependency on layout_plugin to 8.x-2.x so the clash does not happen.
alexpott’s picture

#169 another option would be to use a different annotation - that should fix this... \Drupal\layout_plugin\Annotation\Layout and \Drupal\Core\Layout\Annotation\Layout. This is one reason why Doctrine don't like SimpleAnnotationReader and not adding use statements for annotations.

swentel’s picture

@alexpott

I haven't wrapped my mind mind completely around it either. At first I though a new branch would be ok for DS. That's why there's a 8.x-3.x branch already with a dependency on core >= 8.3.x (although it still has a dependency on layout_plugin now haha). It made sense in my head because when this patch, and also field layouts would land, we need to rewrite large parts of our code base anyway if want to use the core modules.

However, #2796581: Fields must store their region in entity displays affects us as well so, I'm starting to wonder if we need an additional branch so that we end up with this:
8.x-2.x: core 8.2.x for people who never upgrade (tsss)
8.x-3.x: for #2796581: Fields must store their region in entity displays and dependency on core >= 8.3.x
8.x-4.x: for dependency on the new experimental modules in core

see also #2821201: Update Display Suite for layout_plugin in core

effulgentsia’s picture

Abusing @internal for something that is actually intended to be public API.

I wonder if @version 0.1.0 (or whatever other number seems appropriate) instead of @internal would address this concern. A practical difference between the two is that IDEs show you (e.g., via a strikethrough) when you're referencing @internal code, but I don't think they do anything like that for @version 0.* code.

alexpott’s picture

Re #172. I think that the @internal is actually fine for this. Because it is internal until we decide to make it stable. Panels, DS etc can't do stable releases relying on this functionality. If they choose to experiment with it they are relying on internal APIs. These APIs are internal because they are no yet finished and therefore we can't consider them public API. And when we make it stable then great we just remove the @internal tags. I think this is simpler than using @version and deciding what @version 0. means in terms of a stable 8.3.x Drupal core release.

tim.plunkett’s picture

The contrib layout_plugin only has alpha releases, we can do whatever is necessary in the next alpha release, or just mark it obsolete and abandon the project.

alexpott’s picture

@tim.plunkett but Display Suite has a stable release and Panels a beta. They can't just move to use core's layout plugin once this is in. And once a site has those modules it becomes problematic to install the layout_discovery. So if we introduce an experimental module in core that using layouts for entity display then sites with display suite or panels can't experiment with it.

xjm’s picture

Yesterday the committers discussed this issue and agreed to go forward with the @internal notation currently in the patch, and explore whether to add @version and/or the idea proposed in #2833347: Allow experimental modules to rely on \Drupal\Core\ namespaced classes that are only loadable when the module is enabled as part of a followup.

So that means AFAIK the one thing we need to do is make sure we have an agreed solution for #171. Part of the reason @alexpott is concerned is from experience with a similar contrib to core change for the Workflow Initiative that has turned out to be a lot more difficult than expected.

I'll try to follow up with @tim.plunkett and @alexpott to agree on the way forward.

xjm’s picture

tim.plunkett’s picture

8.x-2.x: core 8.2.x for people who never upgrade (tsss)
8.x-3.x: for #2796581: Fields must store their region in entity displays
8.x-4.x: for dependency on the new experimental modules in core

If this lands in 8.3.x, why wouldn't that just be two branches?
Sure you'd be depending on an experimental core module/subsystem, but is that really any worse than depending on an alpha release of a contrib module?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 157: 2296423-layout_plugin-layout_discovery-157.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Random failure

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
858 bytes
66.02 KB

Per discussion with @alexpott, @xjm, and @swentel in IRC, adding a hook_requirements that explicitly checks for layout_plugin.

Note that #2833462: hook_requirements($phase = 'install') does not work as expected for experimental modules was committed to 8.3.x already.

@xjm reviewed this interdiff so leaving at RTBC

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Meant to leave at RTBC

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
@@ -0,0 +1,223 @@
+      // Prepend 'layout__' so the base theme hook will be used.
+      $definition->setThemeHook('layout__' . strtr($template, '-', '_'));

I discussed this with @tim.plunkett. Turns out that this is needed due to a bug with the way that Drupal\Core\Theme\Registry collects preprocess functions. Seems that without the theme hook name starting with 'layout__', we don't get template_preprocess_layout() executing for it, but we need that. Ideally, the theme registry would collect preprocess functions purely based on the 'base hook' hierarchy, not on anything magical with '__'. But fixing that is out of scope for this issue.

However, I'm somewhat concerned with theme hook names not matching up with template names. In principle, it's ok. In fact, drupal_find_theme_templates() has some code in it already to handle such a situation. Except, that code is incomplete. If a theme wants to implement an even more specific suggestion of such a template, I don't think that would work. E.g., if a module defines foo-1col.html.twig, but this gets entered into the theme registry as layout__foo_1col, then a theme could implement either foo-1col.html.twig or layout--foo-1col.html.twig to override it, but foo-1col--SOME_SUGGESTION.html.twig would fail to get discovered, whereas layout--foo-1col--SOME_SUGGESTION.html.twig would be discovered ok I think (just basing all this on reading the code in drupal_find_theme_templates(), not in trying any of this).

Maybe suggestions of specific layouts is not a needed use case, I don't know? But I'm just highlighting some of the WTFs that occur when the theme hook name and the template name don't match up. While such a mismatch has always been a possibility in theory (given that people can put whatever value they want into the 'template' key), I don't think we've ever yet committed code to core that deliberately made use of it, so if we do so now, we're opening ourselves to having to deal with that latent technical debt getting surfaced.

+++ b/core/modules/system/tests/modules/layout_test/layout_test.layouts.yml
@@ -0,0 +1,29 @@
+  template: templates/layout-test-1col

What if, instead, we created a convention that layout templates should be defined here as layout--MODULE_NAME-*? In other words, this would become template: templates/layout--layout-test-1col. That way, the LayoutPluginManager code above wouldn't need to prefix the hook name and create a mismatch between the hook name and the template name. But we'd still get the benefit of the magic naming that allows template_preprocess_layout() to run.

I'm leaving this issue as RTBC, because potentially, since this is all experimental code, we can deal with all this in a follow-up. However, if we end up changing the naming convention of templates, as I'm proposing, then that affects modules that will want to start providing layouts, so maybe we'd rather settle on it before commit?

tim.plunkett’s picture

I don't think this is a commit blocker, since this is experimental.

I agree with the WTF, I'd rather fix the Registry.

I think forcing templates to be layout-- prefixed is worse, I'd rather not dictate such things to themers.

Also the #theme part isn't used directly, only by LayoutDefault::build()

Berdir’s picture

See #2559825: Preprocess functions from base hooks not triggered for theme hooks not using the __ pattern for that problem, has AFAIK a working patch and needs reviews :)

xjm’s picture

@alexpott and I have both updated #2811175: [plan] Add layouts to Drupal with additional followups that have been identified since it was posted. Now would be a good time to review/give feedback there!

Additionally, here are other followups I think we may still need:

  1. "Must" followup to remove the @internal once we are ready to make it stable
  2. "Should" followup (maybe blocked on https://www.drupal.org/node/2296423) for #184
  3. "Could" followup for discussing the service name and schema name more if needed (?)
  4. Related followup (not part of Layouts roadmap) for @internal vs. @version vs. etc.
  5. Related followup (not part of Layouts roadmap) for adding various additional/discovered best practices to experimental handbook (try to get name/code location right, needs to be disable-able, note about core/lib, etc.)

Am I missing anything?

tim.plunkett’s picture

#186, good to know there's an issue!

#187.1 #2834025: Mark Layout Discovery as a stable module
#187.2 #2834019: Remove the "layout__" prefixing of theme hooks
#187.3 #2834023: Discuss service ID and config schema naming for Layouts
#187.4 I haven't seen or heard anything about @version, so I don't know enough to file this one.
#187.5 Also not filing this one

Adding explicit @todo for 2, confirmed that all the @internal's are sufficient and don't need @todo's in addition.

xjm’s picture

Added this note to all three handbook pages for the module:
https://www.drupal.org/node/2619128/revisions/view/9126312/10243380

  • xjm committed e950006 on 8.3.x
    Issue #2296423 by tim.plunkett, dsnopek, swentel, Manuel Garcia, tedbow...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.3.0 release notes

Alright, I think this has ended up in a really good spot, we've covered our bases as best we can for the path forward for core and contrib, and the best way to make improvements is to get this in and move forward on both the field layout work and the contrib implementations.

I'll file followups for #187 points 4 and 5.

Committed and pushed to 8.3.x. Thanks everyone for your patience!

xjm’s picture

If anyone has any additional feedback or potential followups, please add them to #2811175: [plan] Add layouts to Drupal.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

Component: layout.module » layout_discovery.module