Implement style plugin type

I've been doing some work towards this. Will get a patch up asap.

This code is just experimental. (I haven't tested anything yet).

It's written to depend on fregas layout module. Hopefully it will provide a very rough starting point to move forward from.

+++ b/panels.services.yml
@@ -0,0 +1,4 @@
+  plugin.manager.style:

suppose plugin.manager.panels.style is a proper name

Here's an updated patch, no inter-diff because most of it is from scratch. Added a style dialog to both regions and blocks on the edit page. Allowed a choice between two plugins, Default and List. Next step is to add settings to the dialog so that style plugins can be configurable.

Changed the plugin name to panels.style, as per comment #6.

This applies against d8panels.

Patch is not applying anymore for me. I'll try again within PhpStorm and see if it pulls some magic and re-roll.

$ curl https://www.drupal.org/files/issues/2296437-7.patch | git apply -v
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 23280  100 23280    0     0  45992      0 --:--:-- --:--:-- --:--:-- 45917
<stdin>:16: trailing whitespace.
    
Checking patch panels.routing.yml...
<stdin>:26: new blank line at EOF.
+
Checking patch panels.services.yml...
Checking patch src/Annotation/PanelsStyle.php...
Checking patch src/Form/PanelsBlockEditPanelsStyleForm.php...
Checking patch src/Form/PanelsRegionEditFormBase.php...
Checking patch src/Form/PanelsRegionEditPanelsStyleForm.php...
Checking patch src/Plugin/DisplayVariant/PanelsDisplayVariant.php...
error: while searching for:
          $this->contextHandler()->applyContextMapping($block, $contexts);
        }
        if ($block->access($this->account)) {
          $block_render_array = [
            '#theme' => 'block',
            '#attributes' => [],
            '#weight' => $weight++,
            '#configuration' => $block->getConfiguration(),
            '#plugin_id' => $block->getPluginId(),
            '#base_plugin_id' => $block->getBaseId(),
            '#derivative_plugin_id' => $block->getDerivativeId(),
          ];
          if (!empty($block_render_array['#configuration']['label'])) {
            $block_render_array['#configuration']['label'] = String::checkPlain($block_render_array['#configuration']['label']);
          }
          $block_render_array['content'] = $block->build();

          $build[$region][$block_id] = $block_render_array;
        }
      }
    }
    $build['#title'] = $this->renderPageTitle($this->configuration['page_title']);
    return $build;

error: patch failed: src/Plugin/DisplayVariant/PanelsDisplayVariant.php:176
error: src/Plugin/DisplayVariant/PanelsDisplayVariant.php: patch does not apply
Checking patch src/Plugin/PanelsStyle/PanelsStyleBase.php...
Checking patch src/Plugin/PanelsStyle/PanelsStyleDefault.php...
Checking patch src/Plugin/PanelsStyle/PanelsStyleInterface.php...
Checking patch src/Plugin/PanelsStyle/PanelsStyleList.php...
Checking patch src/Plugin/PanelsStyle/PanelsStylePluginManager.php...

Moving the concept of styles to ctools so it can be abstracted and used across the Block system is pretty nifty. I unfortunately read #10 after fixing patch :) Had issues in code.

Sounds like, though, regions would just be an annotated extension of styles.

I saw some twitter discussion about style plugins: https://twitter.com/dsnopek/status/661527359619600385

I agree with David that it is a good idea for style plugins to simply change the theme hook on the block. The same concept could be applied to regions. Does this issue need to switch to the CTools queue?

To clarify, David, do you think the theme hook would change from 'block' to 'my_style_plugin' or do you mean using suggestions so 'block' becomes 'block__my_style_plugin'?

IRC convo today with EclipseGC:

japerry: EclipseGc: yah block styles
[2:11pm] EclipseGc: japerry: so… I could see that moving out of panels
[2:11pm] EclipseGc: japerry: fwiw
[2:11pm] japerry: EclipseGc: heh I was just thinking the same thing
[2:11pm] EclipseGc: japerry: ctools if you like, or we can discuss other options
[2:11pm] japerry: block styles sounds like something ctools should be managed now
[2:16pm] EclipseGc: japerry: ok, I’m on board

Now that panel panes are blocks, there is a much better argument that the implementation of styles should be done in ctools, and that other modules (like classy panels styles) can extend the ctools implementation.

How does this sound?

I guess as long as we can pass in other variables which can be configured on the style form, that all sounds good to me. If not, that would be a regression for us the way that we use styles in D7 on panes.

Is this issue still valid since the Block Styles module exists?

Thats a good find! It solves one of the two uses for the panels style plugins. Panels style plugins also allow you to choose a particular style for a region. That said, we should look at building those plugins on top of the styles API

Today faces with absence of styles) thought to implement a-la https://www.drupal.org/project/panels_accordion

Here's a code/implementation review of patch #11

The main confusion is inability to separate plugin's availability/applicability for each kind of build (region and block)
Suppose they need separate interfaces
Plugin should expose something static::getBuilders() or define in annotation "no_block|no_region" to be faster (prevent classloading)

Also plugins could have "settings form" to be exposed somehow, nice to have one example (List type: ul/ol)

So overall is good starting point

PS: https://www.drupal.org/project/block_styles looks interesting

1. +++ b/panels.routing.yml
   @@ -0,0 +1,19 @@
   +    _entity_access: page.update
   +    ¶
   +### Blocks
   

   trailing whitespace

2. +++ b/src/Annotation/PanelsStyle.php
   @@ -0,0 +1,35 @@
   +/**
   + * @file
   + * Contains Drupal\panels\Annotation\Style.
   + */
   

   need to be removed

3. +++ b/src/Form/PanelsBlockEditPanelsStyleForm.php
   @@ -0,0 +1,93 @@
   +  protected function submitText() {
   +    return $this->t('Update style');
   ...
   +      '#value'       => $this->submitText(),
   
   +++ b/src/Form/PanelsRegionEditFormBase.php
   @@ -0,0 +1,108 @@
   +   * Returns the text to use for the submit button.
   ...
   +  abstract protected function submitText();
   ...
   +      '#value'       => $this->submitText(),
   
   +++ b/src/Form/PanelsRegionEditPanelsStyleForm.php
   @@ -0,0 +1,29 @@
   +  protected function submitText() {
   +    return $this->t('Update style');
   

   Any reason for separate method?

4. +++ b/src/Form/PanelsBlockEditPanelsStyleForm.php
   @@ -0,0 +1,93 @@
   +    $form_state->set('block_id', $this->block->getConfiguration()['uuid']);
   

   why not $this->block->uuid()?

5. +++ b/src/Form/PanelsBlockEditPanelsStyleForm.php
   @@ -0,0 +1,93 @@
   +    foreach (\Drupal::service('plugin.manager.panels.style')->getDefinitions() as $definition) {
   

   better to inject

6. +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -0,0 +1,54 @@
   +  public function buildRegion(PanelsDisplayVariant $display, array $build, $region, array $blocks) {
   +    $region_name = Html::getClass("block-region-$region");
   +    $build['#prefix'] = '<div class="' . $region_name . '">';
   +    $build['#suffix'] = '</div>';
   +
   +    return $build;
   +  }
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleDefault.php
   @@ -0,0 +1,19 @@
   + * @PanelsStyle(
   + *   id = "panels_default",
   + *   title = @Translation("Default")
   ...
   +class PanelsStyleDefault extends PanelsStyleBase {
   +}
   

   I think default implementation should be "naked"
   so default will not be empty

7. +++ b/src/Plugin/PanelsStyle/PanelsStyleInterface.php
   @@ -0,0 +1,43 @@
   +interface PanelsStyleInterface extends PluginInspectionInterface {
   ...
   +   * Renders a region.
   ...
   +  public function buildRegion(PanelsDisplayVariant $display, array $build, $region, array $blocks);
   ...
   +   * Render a block.
   ...
   +  public function buildBlock(PanelsDisplayVariant $display, BlockPluginInterface $block);
   

   This leaves no way to implement a region-only style... suppose regression

   Probably there should be only one static method **getBuilders()**

   Anyway there should be separate interfaces for each type of builder.

   Also plugin abilities cound be pointed in annotation so no reason to instantiate plugin to decide applicability of plugins to panel part.

8. +++ b/src/Plugin/PanelsStyle/PanelsStyleList.php
   @@ -0,0 +1,35 @@
   + *   title = @Translation("List")
   ...
   +class PanelsStyleList extends PanelsStyleBase {
   ...
   +  public function buildRegion(PanelsDisplayVariant $display, array $build, $region, array $blocks) {
   +    $build = [
   +      '#theme' => 'item_list',
   +      '#list_type' => 'ul',
   ...
   +    return parent::buildRegion($display, $build, $region, $blocks);
   

   We can't wrap block into list?
   So plugin implements default block builder from base implementation?

   Suppose this plugin should be only for region that means not-applicable to block!

9. +++ b/src/Plugin/PanelsStyle/PanelsStylePluginManagerInterface.php
   @@ -0,0 +1,16 @@
   + * Provides an interface for the discovery and instantiation of layout plugins.
   ...
   +interface PanelsStylePluginManagerInterface extends PluginManagerInterface {
   +
   +}
   

   Any reason to have empty interface?

Current patch is really old, I managed to make it work with blocks by re-rolling - kindoff - the previous patch from mglaman to work with the new version.
please note that this still needs lots of work and review (sorry im not that familiar with panels code)

re-roll with support form regions styles

This is awesome, thanks so much :)

I fixed up the testing issues with PanelsDisplayVariantTest.php so only the StandardDisplayBuilder.php remains. I am still learning prophecy but with regards to the following:

+++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
@@ -103,43 +106,21 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
+          $build[$region][$block_id] = $panels_display->getBlockStyle($block_id)->buildBlock($panels_display, $block);
...
+     $build[$region] = $panels_display->getRegionStyle($region)->buildRegion($panels_display, $build[$region], $region, $blocks);

Can this be mocked with Prophecy or is it a Demeter violation?

Correct patch.

One more try, sorry for the noise!

Down to three errors in StandardDisplayBuilderTest.php

Updating patch to latest panels dev / 8.x-3.0-beta5.

After applied the patch #32 to latest stable module version, drupal said:

Class 'Drupal\panels\Plugin\PanelsStyle\PanelsStylePluginManager' not found

I made some changes on top of patch #32 so that style plugins could provide custom settings again. Attached is a patch against 8.x-3.x-dev and an interdiff between patch #32 and this patch. Let me know what you think.

Fix remaining errors in StandardDisplayBuilderTest.

Skimmed last patch and I think "base" should be moved to "default"

1. +++ b/panels.routing.yml
   @@ -26,3 +26,10 @@ panels.delete_block:
   \ No newline at end of file
   
   +++ b/src/Annotation/PanelsStyle.php
   @@ -0,0 +1,35 @@
   \ No newline at end of file
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleDefault.php
   @@ -0,0 +1,19 @@
   \ No newline at end of file
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleList.php
   @@ -0,0 +1,35 @@
   \ No newline at end of file
   
   +++ b/src/Plugin/PanelsStyle/PanelsStylePluginManager.php
   @@ -0,0 +1,39 @@
   \ No newline at end of file
   

   needs to be added

2. +++ b/src/Annotation/PanelsStyle.php
   @@ -0,0 +1,35 @@
   + * @file
   + * Contains Drupal\panels\Annotation\Style.
   
   +++ b/src/Form/PanelsRegionEditFormBase.php
   @@ -0,0 +1,219 @@
   + * @file
   + * Contains \Drupal\panels\Form\PanelsRegionEditFormBase.
   
   +++ b/src/Form/PanelsRegionEditPanelsStyleForm.php
   @@ -0,0 +1,29 @@
   + * @file
   + * Contains \Drupal\panels\Form\PanelsRegionEditPanelsStyleForm.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -0,0 +1,110 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle\PanelsStyleBase.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleDefault.php
   @@ -0,0 +1,19 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle\PanelsStyleDefault.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleInterface.php
   @@ -0,0 +1,44 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle\PanelsStyleInterface.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStyleList.php
   @@ -0,0 +1,35 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle\PanelsStyleList.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStylePluginManager.php
   @@ -0,0 +1,39 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle\PanelsStylePluginManager.
   
   +++ b/src/Plugin/PanelsStyle/PanelsStylePluginManagerInterface.php
   @@ -0,0 +1,16 @@
   + * @file
   + * Contains \Drupal\panels\Plugin\PanelsStyle;\PanelsStylePluginManagerInterface.
   

   needs to be removed
   https://www.drupal.org/docs/develop/standards/coding-standards#indenting

Thanks for your review @andypost! I fixed the CS issues.

I think "base" should be moved to "default"

I'm not convinced, that we should really do this. While I just built on-top of what's already been implemented by our fellow community members, it seems to be a common pattern to have a base plugin for all plugins to extend and a default class that extends it as the default plugin, even if it means, that the default plugin class is basically empty. Since there might be a point in the future, where we might want to add code to the default plugin without changing the base plugin, this doesn't seem as weird to me as it might look at first. But let's just wait a bit and gather a few more opinions. If at the end the consensus is to move, it's fine with me.

@FeyP I know that's a common pattern but I still not sure it's good idea and #2296423: Implement layout plugin type in core takes different approach

Should this still be tagged as needs review? Tests are passing and looks like it was reviewed by a few people including myself. Perhaps we should switch this to RBTC or is there still something missing?

When I try to apply this patch, I get a hunk failed message when it tries to patch the StandardDisplayBuilder.php file. I only kinda know what I'm doing so I'm not sure what that means.

There was a new release 2 days ago so the patch probably doesn't apply any more and needs a reroll. I'll try to prepare that asap, thanks for the heads up.

Ok, obviously there was no new release 2 days ago, I was looking at the wrong branch ;). But the patch needed a reroll none the less. So here we go...

1. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -293,6 +304,128 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   +      $this->configuration['regions'][$region]['style']['plugin'] = $style->getPluginId();
   +      $this->configuration['regions'][$region]['style']['configuration'] = $style_settings;
   

   this needs config schema for this properties

2. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -293,6 +304,128 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   +    elseif (is_string($style)) {
   +      $this->configuration['regions'][$region]['style']['plugin'] = $layout;
   +      $this->configuration['regions'][$region]['style']['configuration'] = $style_settings;
   

   $layout var is undefined

3. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -293,6 +304,128 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   +    else {
   +      throw new \Exception('Style must be a string or PanelsStyleInterface object');
   

   better define proper interface expecting style as PanelsStyleInterface as function argument

4. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -480,7 +613,7 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   -  protected function renderPageTitle($page_title) {
   +  public function renderPageTitle($page_title) {
   

   public probably should be defined in interface

Lets consider this for the next beta.

What about D8.3 & Panels v4 support. these patches are actually against the old layout_plugin module.

We probably need to change all layout_plugin references to D8.3 & V4 of the module.

Hi, I've re-rolled last patch for 8.x-4.x-dev

Patch is working great, but 1 test are failing, because we have http://joxi.net/vAWl0Y5ikg3ydr.jpg this issue https://www.drupal.org/node/2867795

New re-roll against latest dev (commit dd6e30d).

Leaving this at "Needs work", because of the tests fail, as mentioned in #51

Fixing tests.

Thought I would take a stab at fixing the tests on this one, but also made a few tweaks:

• Added 'block' and 'region' annotation values so plugins can inform when they're meant to build blocks, regions, or both (set to both by default)
• Set the list style plugin for regions only
• Added an option for the List style plugin to select an ordered or unordered list. Figured it made sense and provides a simple example of a plugin with some settings.

Patch is attached with an interdiff. I may have fixed the outstanding test fail as well, but we'll see if testbot agrees.

Something doesn't feel right about having the StylePlugin responsible for rendering regions and blocks. As we can see with the List style plugin, sometimes we'll want them to just render one or the other. But the interface forces defining both build methods. In the very least I think we need to break up the interface into one for each type (block, region) which plugins can then choose which ones to implement based on where they want to be available. Or one further would be to just have separate plugin types for each (although from reading the comments above this may introduce some kind of regression / backwards compatibility issues?)... Anyway, for now I haven't made any major changes like that. I figured it would be worth some discussion at least before pursuing any major rewrites.

Updated config schema and added config to support setting the default panels styles (was previously hardcoded to "panels_default"). No settings page yet so it has to be done through settings.php. I can do a settings page up, but was hoping to get a patch review before proceeding too much further :)

+++ b/src/Plugin/PanelsStyle/PanelsStyleInterface.php
@@ -4,13 +4,14 @@ namespace Drupal\panels\Plugin\PanelsStyle;
-interface PanelsStyleInterface extends ConfigurablePluginInterface, PluginInspectionInterface {
+interface PanelsStyleInterface extends ConfigurablePluginInterface, PluginInspectionInterface, PluginFormInterface {

Not sure this change needed, it's a implementation details for plugin to implement this methods

1. +++ b/config/install/panels.settings.yml
   @@ -0,0 +1,2 @@
   +default_style_region: panels_default
   +default_style_block: panels_default
   

   Instead of this settings better to add fallback plugin

2. +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -18,7 +18,7 @@ use Drupal\panels\Plugin\PanelsStyle\PanelsStyleInterface;
   -class PanelsStyleBase extends PluginBase implements PanelsStyleInterface {
   +abstract class PanelsStyleBase extends PluginBase implements PanelsStyleInterface {
   

   this makes every plugin to implement configuration form but some styles could have no settings & forms

3. +++ b/tests/src/Unit/StandardDisplayBuilderTest.php
   @@ -134,12 +136,27 @@ class StandardDisplayBuilderTest extends UnitTestCase {
   +    $this->configFactory = $this->getConfigFactoryStub([
   +      'panels.settings' => [
   +        'default_style_block' => 'panels_default',
   +        'default_style_region' => 'panels_default',
   

   they also could be a container params but I think fallback plugin more predictable

Example of fallback plugin #2249303: Implement fallback plugin for Block plugins

@andypost Thanks for the review!

RE: PluginFormInterface & Item #2 in your comments

I think it is needed since buildConfigurationForm, validateConfigurationForm, and submitConfigurationForm are all being called in \Drupal\panels\Form\PanelsBlockConfigureFormBase. Whether or not the plugin has a form or settings doesn't really matter, they still need to define these methods since they'll always be called when the plugin is selected.

It's possible there's a different pattern we can use here, but I feel like this is pretty similar to how other plugins handle optional forms. Field formatters for example do pretty much the same thing (@see Drupal\Core\Field\FormatterBase). Thoughts?

RE: Fallback Plugin

I quite like the idea of a fallback, but I think it's different than my intention with the default settings. The default plugin settings are so you can set which plugin is selected by default when adding new blocks / editing regions with no settings yet. The panels_default plugin doesn't really do anything so I could see people wanting to set their own default (myself included).

However, a fallback plugin for when a previously configured plugin no longer exists I think is a good idea. I'll get working on that one this week.

New patch with fallback plugin so we don't WSOD when a style plugin goes missing. Currently falling back to rendering the region/block using the base class. Not sure if that's the best way to go but it seems better to me to render something rather than nothing? Missing plugin message is logged.

That looks like some nitpicks left but overall great!

1. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -91,77 +91,37 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   +    $build['#title'] = $panels_display->renderPageTitle($panels_display->configuration['page_title']);
   
   +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -479,7 +625,7 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   -  protected function renderPageTitle($page_title) {
   +  public function renderPageTitle($page_title) {
   

   not sure that related to scope of the issue

2. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -294,6 +316,130 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   +  public function getRegionStyle($region, $style_id = '') {
   +    $region_config = !empty($this->configuration['regions'][$region]) ? $this->configuration['regions'][$region] : [];
   +    $default = $this->configFactory->get('panels.settings')->get('default_style_region');
   +
   +    $plugin_id = !empty($region_config['style']['plugin']) ? $region_config['style']['plugin'] : $default;
   +    $plugin_configuration = !empty($region_config['style']['configuration']) ? $region_config['style']['configuration'] : [];
   +
   +    if (!empty($style_id) && $style_id !== $plugin_id) {
   +      $plugin_id = $style_id;
   +      $plugin_configuration = [];
   +    }
   +
   +    return $this->styleManager->createInstance($plugin_id, $plugin_configuration);
   

   This logic could be simplified if style manager can accept empty plugin ID and resolve it to default/fallback inside manager to hide implementation

3. +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -0,0 +1,110 @@
   +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
   +    $this->configuration = $form_state->getValues();
   

   not sure that useful, other plugins can't call parent with this defaults

4. +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -0,0 +1,110 @@
   +  public function getConfiguration() {
   +    return array_merge($this->defaultConfiguration(), $this->configuration);
   

   is there useful trait in core for that or maybe base class

5. +++ b/src/Plugin/PanelsStyle/PanelsStyleBroken.php
   @@ -0,0 +1,52 @@
   +  public function buildRegion(PanelsDisplayVariant $display, array $build, $region, array $blocks) {
   +    \Drupal::logger('panels')->alert('THe panels style plugin named "@style" is missing for region "@region"', [
   ...
   +  public function buildBlock(PanelsDisplayVariant $display, BlockPluginInterface $block) {
   +    \Drupal::logger('panels')->alert('The panels style plugin named "@style" is missing for block "@block"', [
   

   makes sense to add variant name somehow to help debug it

6. +++ b/src/Plugin/PanelsStyle/PanelsStyleBroken.php
   @@ -0,0 +1,52 @@
   +      '#markup' => t('<em>The panels style plugin is missing for this block or region.</em>'),
   

   $this->t()

7. +++ b/src/Plugin/PanelsStyle/PanelsStyleList.php
   @@ -0,0 +1,64 @@
   +        'ul' => 'Unordered',
   +        'ol' => 'Ordered',
   

   needs $this->t()

8. +++ b/src/Plugin/PanelsStyle/PanelsStylePluginManager.php
   @@ -0,0 +1,66 @@
   +    // Don't return the fallback plugin definition
   +    if (isset($definitions['panels_broken'])) {
   +      unset($definitions['panels_broken']);
   

   no reason for isset()

Thanks again for the review!

1. Agreed, there may be reason for this but I don't think it falls in the scope of this issue (OP correct me if I'm wrong)
2. I think since the defaults are block/region specific that that logic should remain here.
3. I'm not sure I understand this one? Can you explain further?
4. You know this always felt a little odd to me merging on get. I looked at some other core configurable plugins and a more common pattern is to merge on set using Drupal\Component\Utility\NestedArray::mergeDeep()
5. This is a weird one. I've added the variant label and id to the error message, but if you use panels with page_manager the label gets saved on the page manager page variant and not the panel so this value is just NULL. For now I've overridden the label() method on the PanelsDisplayVariant class to support this... it feels a bit overreaching but isn't necessarily outside the realm of modules supporting each other without explicit dependencies so I can live with it I think...
6. Yep.
7. Yep.
8. Done.

The last patch works fine for me. Thank you Chris

I'm wondering now if the settings for default block/region styles should actually be set on the variant rather than a global setting? Could be more flexible for more complex editing interfaces that make use of the IPE...

@drclaw i would think it makes more sense to apply block block/region styles to the variant rather than adding it globally. Even better would be if you could set a default in global but would be able to override it on the block instance level.

In my specific use case, using the IPE, users never touch the global panelizer configuration but only place blocks on predefined templates. There are some defaults but further from that users are free to place custom block types (text, image, video) and even place multiple instances per block.

I'm afraid i won't be of much help programming this but am willing to review, test and document this feature.

Looking through this.

1. In regards to #66 and #67, I'd agree, that this needs to be applied to the variant not the global settings.

2. +++ b/src/Plugin/PanelsStyle/PanelsStyleBase.php
   @@ -0,0 +1,122 @@
   +  public function buildBlock(PanelsDisplayVariant $display, BlockPluginInterface $block) {
   +    $build = [
   +        '#theme' => 'block',
   +        '#attributes' => ['class' => ['container']],
   +        '#weight' => $display->getWeight(),
   +        '#configuration' => $block->getConfiguration(),
   +        '#plugin_id' => $block->getPluginId(),
   +        '#base_plugin_id' => $block->getBaseId(),
   +        '#derivative_plugin_id' => $block->getDerivativeId(),
   +      ];
   +    if (!empty($build['#configuration']['label'])) {
   +      $build['#configuration']['label'] = SafeMarkup::checkPlain($build['#configuration']['label']);
   +    }
   +    $build['content'] = $block->build();
   +
   +    return $build;
   

   Does the base style (and by extension, the default style) really need the "container" class added? This could potentially cause problems for anyone using a bootstrap-based theme. I think expected behavior would be that the default style does nothing, and then other style plugins would actually make modifications.

3. I saw @japerry's original recommendation in #15 about placing this functionality in ctools rather than panels, and I think that does make a lot of sense. But with all the progress made on this issue since, has that thought process been abandoned? I definitely don't think it hurts for it to live in panels but also would request an official decision be made from panels/ctools maintainers before additional work is done on this issue.

I am getting this error after applying #64

error: patch failed: src/Form/PanelsBlockConfigureFormBase.php:137
error: src/Form/PanelsBlockConfigureFormBase.php: patch does not apply
error: patch failed: src/Plugin/DisplayBuilder/StandardDisplayBuilder.php:48
error: src/Plugin/DisplayBuilder/StandardDisplayBuilder.php: patch does not apply
error: patch failed: tests/src/Unit/StandardDisplayBuilderTest.php:2
error: tests/src/Unit/StandardDisplayBuilderTest.php: patch does not apply

I'm sure #68 contains copy/pasted code, but it reminds me of #2934223: layout_builder module has a hidden dependency on block module:
'#theme' => 'block' only works if block.module is installed, and Panels doesn't depend on it.

I brought my question in #68.3 to the weekly #layouts meeting on slack. Based on the discussion, I thought it prudent to post the full conversation here, as the general opinion seems to be around getting better support for styles in core itself.

widgetsburritos [12:29 PM]
Hi guys. I like to request panels maintainer feedback on #68.3 here: https://www.drupal.org/project/panels/issues/2296437#comment-12406364

eclipsegc [12:34 PM]
sighs
so... implementing styles is a thing we've avoided even discussing for a couple years at this point
lol
and with all the core work going on...
maybe we should have a larger discussion about that?

widgetsburritos [12:36 PM]
Yeah. I ask because we’re in the middle of our D7 to D8 migration and we heavily rely on styles. I’ve found a temporary and hacky solution but would prefer it being something officially supported.

samuel.mortenson [12:36 PM]
On my D8 Panels sites I’ve been basically hard coding styles - i.e. having Block Plugins with style-like configuration, and Custom Block fields that represent styles. All get “applied” in hooks
It’s a mess but I started doing it around a year ago

widgetsburritos [12:39 PM]
My issue involves a little bit more than that as it also has to integrate with the entity block module. We’re applying styles to embedded nodes. That module is actually what I’ve hacked to accomplish what I’m trying to do. But it leaves a lot to be desired.
Either way. I just wanted to get this discussion on the radar as I do think it’s an important feature that is currently missing in D8

eclipsegc [12:40 PM]
so...

samuel.mortenson [12:40 PM]
So thinking about Layout Builder - a huge issue right now is that there’s no way to generically alter a Block Plugin form in _all places it appears_, and also alter its configuration.

eclipsegc [12:41 PM]
with the new Layout Builder, we're currently dependent on block.module for the theme output. I wonder if a long-term fix for that might be introducing a style plugin with a sane/unstyled default

samuel.mortenson [12:41 PM]
Without that, you have to write panels styles things for every parent form the Block Plugin form is displayed in (IPE, backend, Layout Builder, etc)

eclipsegc [12:41 PM]
our schema would certain support it
I guess. I don't want to put up blockers for LB, but we can discuss adding this either as a stable feature, or figuring out what contrib might look like to make it happen.

samuel.mortenson [12:42 PM]
If there was a way to generically alter Block Plugin forms, you could have a block_styles module that would “just work” for a lot of use cases.

eclipsegc [12:42 PM]
I worry about supporting all the Panels things going forward as we largely replace much of it in core

tim.plunkett [12:42 PM]
@samuel.mortenson i'm surprised that hasn't come up elsewhere for any other PluginFormInterface

samuel.mortenson [12:43 PM]
@tim.plunkett Yeah I think I’ve only thought about it with regards to blocks, but it’s actually a big issue. There is already a ton of duplicated form_alter hooks out there for blocks, since their forms show up in so many places (edited)
“How do I add extra form elements and configuration to a plugin?”

eclipsegc [12:46 PM]
yeah, actually years ago when we discussed this in core, my proposal was a form element in the block form that allowed style to be chosen
however
in LB, I think it'd be better to treat it as a contextual link

tim.plunkett [12:47 PM]
sure
but in general embedded forms are kinda screwed

samuel.mortenson [12:47 PM]
I’ve wanted to add a class selector for Layout Plugins too - I think it’s a larger plugin issue

tim.plunkett [12:48 PM]
it is

eclipsegc [12:49 PM]
form altering plugin forms universally?
w/o knowing what they're embedded in?

tim.plunkett [12:49 PM]
right
because you shouldn't always need to care

eclipsegc [12:49 PM]
I agree... that seems like a bit of an over sight on our part

tim.plunkett [12:50 PM]
I think we could use PluginFormFactory and decorators

eclipsegc [12:50 PM]
but also, out of scope for block styles
we can solve it a different way
@tim.plunkett fwiw, I'd really like to explore replacing our block theme dependency with something around this idea.
for stable

tim.plunkett [12:54 PM]
:thumbsup:

eclipsegc [12:55 PM]
cool

I'm not entirely sure what the next course of action is here, but I'm just documenting this for maintaining a record of that original conversation.

Thanks for your points in comment #68 and for posting this @WidgetBurritos!

So it kind of sounds like the maintainers are leaning towards a higher-level solution, probably with some core changes, which means it could be a little while before we see it. Also if the final solution for Panels ends up being related to those changes, it makes for an awkward migration for anyone using this patch to whatever that looks like.

So, if we're going to have to wait for upstream changes before being able to do styles in the panels module, I wonder if what we're doing in this patch is better suited for contrib for the time being? I _think_ we can do everything for panels block styles through alter hooks. The only thing we can't alter is the panels regions output, but maybe we can get a small hook put in for that for now... The advantages I see in doing this in contrib are:

• We can use panels styles in the interim of waiting on the whatever core decisions/changes need to be made
• Since we know the solution is temporary it's better to be in contrib rather than having to apply a patch that will never be committed
• Also migrating should be easier since it can live in said contrib module

I'd be willing to give it a shot in a separate module but would like to hear what people think before diving in too deep... Also I may be overlooking some major thing that makes doing this in contrib impossible/illogical/insane :)

@drclaw I definitely think most of this could live as a separate contrib module, although I'd have to reevaluate everything that's happening in the patch. I'd be willing to do some work with you on this. Perhaps a sandbox module for now?

has anyone started working on a contrib solution?

@mohammed76 Unfortunately I have no not had a chance. I can't speak for the others.

Here's a re-roll of the patch with added block hooks to better support quickedit, contextual links etc.

On another note I'm planning on tackling the contrib solution in a the next month as I have a project that's going live in July that needs it (they don't want a patch this big looming on their live site). Will keep this thread updated with progress!

Tested #77 - everything seems to apply and work fine. See the repo - https://github.com/drupov/d8-panels and check out
branch "2296437-implement-style-plugin" there.

A question: how can a custom style plugin be created then (in a custom module)?

@drclaw @mohammed76 FYI: It does appear someone has already taken this patch and created a module out of it:
https://www.drupal.org/project/panels_extra_styles_d8

I'm not exactly sure how up-to-date it is with the patches in this issue, but might be easier to patch that module, than maintain a massive panels patch.

@WidgetsBurritos Looks like that module is actually just a StylePlugin that depends on the patch in this issue. It doesn't add the style plugin functionality itself.

Haha at first I was like "phew, that's gonna save me some time!" :P Oh well, starting on this tonight so hopefully I'll have something working soon!

@drclaw, my bad. I misread that description, and didn't investigate it too closely. Sorry about that.

@drclaw - you're the best!

Hi, @drclaw do you need some help according to your contribution solution?

This seems to works pretty neat. I've updated the repo https://github.com/drupov/d8-panels - check out branch "2296437-implement-style-plugin" git clone -b 2296437-implement-style-plugin git@github.com:drupov/d8-panels.git and follow the installation steps to see it in action.

There is a custom module d8_panels which demos the creation of a custom style plugin ("wrapper"). The code is inspired from panels_extra_styles_d8, but has some improvements and the idea is to show how to make a plugin standalone, without an additional module. There are custom configurations on it and it uses a twig template for the output. Of course based on the latest patch form #77. See the "/test" path in the installation for a demo of a wrapped region and pane (even with title wrappers) on a custom panelized page manager page:

https://ibb.co/kbAM9d

One this is a bit buggy: when I open the pane configuration form for a pane the custom style plugin is not recognized. Switching to default and back to wrapper then shows all the configuration settings:

https://ibb.co/jERiNy

Should we set it to "Needs review"?

For anyone wondering about the contrib solution, I've got a sandbox going with a mostly working version https://www.drupal.org/sandbox/ceastwood/2981033. It's not quite ready yet for testing as I'm still ironing out some kinks, but the bulk of the patch port is done. Will post again when we're ready for testing, just wanted to update in case anyone was wondering if I actually was doing anything :)

I think the contrib solution is ready for testing. https://www.drupal.org/sandbox/ceastwood/2981033

For anyone that has been using the patch in this issue, there are notes on how to migrate in the readme as well as on the sandbox project page. The tl;dr is that your config should be migrated automatically, but you'll need to refactor your style plugins.

Thanks!

Thanks for taking the time to create the sandbox module. I've installed it on one of our sites to replace the patch and so far it's working great.

Thank you everyone for your work on this! I managed to get it all working after making a few patches

Currently Panels Extra Styles D8 depends on the patch, rather than the contrib module, and neither module is compatible with D9 (while Panels Style isn't compatible with anything over D8.5).

Here is a compilation of everything I am using in order to get both modules working together and for D9 compatibility.

Panels Style
#3031437: Error when region style set to default
#3062867: D8.5^/D9 compatibility - User temp store moved to core
#3179208: D8.7^/D9 compatibility - ConfigurablePluginInterface is deprecated
#3179124: D9 Version Requirements

Panels Extra Styles D8
#3179234: Use Panels Style contrib module as a dependency rather than Panels patch
#3179230: D9 Compatibility

I hope this helps others as well!

Edit: I may have spoken too soon. I cannot save block configurations currently due to the errors mentioned here: #3062867: D8.5^/D9 compatibility - User temp store moved to core.

Hi,
I have a Drupal 8 site using the patch in #63. However, when converting to Drupal 9 I get an error from drush:

In YamlDiscovery.php line 24:

/var/www/rory2.bigmallet.bigmallet.co.uk/web/modules/contrib/panels/panels.routing.yml: Duplicate key "panels.region_edit_style" detected at line 36 (near " _c
tools_access: 'machine_name'").

In YamlSymfony.php line 40:

Duplicate key "panels.region_edit_style" detected at line 36 (near " _ctools_access: 'machine_name'").

In Parser.php line 335:

Duplicate key "panels.region_edit_style" detected at line 36 (near " _ctools_access: 'machine_name'").

I find that the patch in #63 adds three duplicate sections for panels.region_edit_style. I am presuming that this is an error and the Symfony upgrade included in Drupal 9 is rejecting this. To this end I have re-rolled that patch with just one added and attached in case it helps anyone else.

The same could be done for the patch in #77.

Regards Rory

Sorry, but the patch file I just submitted in #89 was corrupted. The replacement does work.

Regards Rory