Allow selecting multiple behavior styles

+++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
@@ -142,17 +143,37 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
+          '#title' => $this->t('Default style'),
...
+        '#title' => $this->t('Default style'),

Let's shift the titles immediately after the #type line each. That improves readability.

+++ b/config/schema/paragraphs_collection.schema.yml
@@ -2,11 +2,17 @@ paragraphs.behavior.settings.style:
     group:

This needs to change to a new key "groups" as well. We also should still keep the old key for the valid fallback.

All existing config needs updates. Most should use the new notation.
We should make sure we have a test with one old test item to deal with the fallback to guarantee compatibility.

Template suggestions: Let's simply pick the first ones we find. We don't need to combine them at all.

Yes, quite a few. Specifically also for transitions between one and multiple style groups. if you have group A,B,C test going from A to A,B and from A,B to B and from A,B to C or so. All while having existing content.

+++ b/config/schema/paragraphs_collection.schema.yml
@@ -4,9 +4,21 @@ paragraphs.behavior.settings.style:
+      label: 'Test'
...
+        label: 'Test'

:-)

1. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -81,16 +81,39 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +        $form['style_wrapper'][$group_id] = [
   

   maybe we should have another key here so we have styles[$group_id] = $style?

   Also unhappy about the fact that we don't have machine names for our groups but just the labels and then some magically converted version of it for the key..

2. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -81,16 +81,39 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +          '#title' => !empty($this->configuration['groups'][$group]) ? t('%group Style', ['%group' => $this->configuration['groups'][$group]]) : t('Style'),
   

   $this->configuration['groups'][$group] doesn't exist actually, as group is already the value of the group, so just use that.

   Can you include a screenshot of this form with one and two selected groups?

3. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +152,57 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
       */
      public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
   -    $form['group'] = [
   +    $form['groups'] = [
          '#type' => 'select',
   +      '#title' => $this->t('Style groups'),
   +      '#multiple' => TRUE,
          '#empty_option' => $this->t('- None -'),
          '#options' => $this->yamlStyleDiscovery->getStyleGroups(),
   -      '#title' => $this->t('Style group'),
          '#description' => $this->t('Restrict available styles to a certain style group. Select "- None -" to allow all styles.'),
   -      '#default_value' => $this->configuration['group'],
   +      '#default_value' => $this->configuration['groups'],
          '#ajax' => [
   

   unsure about keeping both keys, actually. might be easier to write an update function than maintain all that code duplication forever?

4. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +152,57 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +      foreach($groups as $group) {
   

   code style.

5. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +152,57 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +        $group_name = strtolower(str_replace(' ', '_', $group));
   

   yep, this again. We should at least have a method for this, to make sure we do this consistently.

   Or we add separate definitions of groups, with an id and label, for example in a $module.paragraphs.style_group.yml file or by introducing a sub-key for styles and groups.

   That would need to be a separate issue that we do first, need to discuss with Miro.

   also here it is $group_name, above $group_id.

not sure, but considering that we have the default setting, we could indeed do something like this:

groups:
  group_a:
    default: foobar
  group_b:
    default: 

?

Discussed.

So yes, lets unify on that structure, write an update function that converts existing paragraph types. We only keep the fallback in one place, and that's for the selected style on paragraphs. add a getStyles($paragraph) method, check the styles behavior setting key first, if empty, check style and then map that to its group.

I think we also need to do the separate group discovery. Probably easier here, then we only need one update function that can do both things at once. That will need to map the label from before back to the new id.

Right, that problem again, because we always show the form elements. You should be able to implement that yourself in the validate configuration form method, that should only be called if enabled.

1. +++ b/config/install/paragraphs.paragraphs_type.container.yml
   @@ -12,3 +12,6 @@ description: 'Use <em>Container</em> to group paragraphs to apply a common style
        enabled: true
   +    groups:
   +      general_group:
   +        default: ''
   

   Hm, isn't general_group something that's only defined in demo? That means hook_install() of the demo module should set that value.

2. +++ b/modules/paragraphs_collection_demo/paragraphs_collection_demo.install
   @@ -82,11 +82,10 @@ function _paragraphs_collection_demo_add_background_plugin_field() {
        ],
        'style' => [
          'enabled' => TRUE,
   -      'group' => '',
   +      'groups' => ['general_group' => ['default' => '']],
        ],
      ]);
   

   ah, you're actually already doing that here, so should be able to just remove it above. And I guess not have the plugin enabled by default then.

3. +++ b/modules/paragraphs_collection_demo/paragraphs_collection_demo.paragraphs.style.yml
   @@ -2,27 +2,27 @@ paragraphs-green:
      groups:
   -    - 'General Group'
   +    - 'general_group'
   

   we can remove the quotes in that case.

4. +++ b/paragraphs_collection.install
   @@ -13,3 +15,29 @@ function paragraphs_collection_update_8001() {
   +function paragraphs_collection_update_8002() {
   +  $paragraph_types = ParagraphsType::loadMultiple();
   +  foreach ($paragraph_types as $id => $paragraph_type) {
   +    /** @var ParagraphsType $paragraph_type */
   +    if ($paragraph_type->hasEnabledBehaviorPlugin('style')) {
   +      $default = '';
   +      // Get the old configuration.
   +      if ($group = $paragraph_type->getBehaviorPlugin('style')->getConfiguration()['group']) {
   +        $default_style = $paragraph_type->getBehaviorPlugin('style')->getConfiguration()['default'];
   +        $group_id = \Drupal::service('paragraphs_collection.style_group_discovery')->getGroupId($group);
   +        if ($style = \Drupal::service('paragraphs_collection.style_discovery')->getStyle($default_style)) {
   +          if (in_array($group_id, $style['groups'])) {
   +            $default = $default_style;
   +          }
   +        }
   +        $config = ['enabled' => TRUE, 'groups' => [$group_id => ['default' => $default]]];
   +        $paragraph_type->getBehaviorPlugin('style')->setConfiguration($config);
   +        $paragraph_type->save();
   +      }
   +    }
   

   Might be better to use raw config here, using (config) entity api in update functions is always tricky, especially through the plugins. Default settings or so might get in the way.

   You can use $config_factory->listAll('paragraphs.paragraph_type.', then load each with getEditable($id)

5. +++ b/paragraphs_collection.module
   @@ -68,7 +68,7 @@ function paragraphs_collection_theme() {
    function paragraphs_collection_modules_installed($modules) {
   -  \Drupal::cache('discovery')->deleteMultiple(['paragraphs_collection_grid_layouts', 'paragraphs_collection_style']);
   +  \Drupal::cache('discovery')->deleteMultiple(['paragraphs_collection_grid_layouts', 'paragraphs_collection_style', 'paragraphs_collection_style_group']);
    }
   

   Lets try to put that in a single cache entry, will comment on that later.

6. +++ b/paragraphs_collection.services.yml
   @@ -2,6 +2,9 @@ services:
   +  paragraphs_collection.style_group_discovery:
   +    class: \Drupal\paragraphs_collection\StyleGroupDiscovery
   +    arguments: ['@module_handler', '@string_translation', '@controller_resolver', '@cache.discovery', '@theme_handler', '@config.factory']
   

   Ah, you made a separate service. I think I would put it in the same service.

7. +++ b/src/StyleGroupDiscoveryInterface.php
   @@ -0,0 +1,42 @@
   +interface StyleGroupDiscoveryInterface {
   

   just move those methods on the existing discovery and then when you do the discovery have two methods and just store things in two different properties.

8. +++ b/src/StyleGroupDiscoveryInterface.php
   @@ -0,0 +1,42 @@
   +   *
   +   * @param string $group_label
   +   *   The group label.
   +   *
   +   * @return string
   +   *   The group id.
   +   */
   +  public function getGroupId($group_label);
   

   I think this only ever makes sense during the upgrade path and then never again. I'd just implement that logic in the update function and loop over the group list.

   Especially with translatable labels and things like that we just don't want to support this as an offical API

Maybe a follow-up, but realised while reviewing here a bit:
If a style group only has one or zero styles, it shouldn't be displayed at all in the form.

+++ b/paragraphs_collection.install
@@ -20,15 +20,16 @@
-        $group_id = \Drupal::service('paragraphs_collection.style_group_discovery')->getGroupId($group);
+        $group_id = strtolower(str_replace(' ', '_', $group));

Not what I meant. We do need to do a reverse lookup, I just meant you should do it inside this function. get them all and loop over it.

Also translation is fun, if we have a label that for some reason already has a translation then it woudn't match. If you have a translatable object, you can get the untranslated string from it.

Also, above you use $paragraph_type->getBehaviorPlugin(), that won't work anymore, now you need to access it all as raw config.

I don't care too much about breaking the API, this project doesn't even have an alpha release. But I do care about an update path.

Also, those bugs you are finding should also be covered by test coverage here if possible.

1. +++ b/paragraphs_collection.install
   @@ -42,10 +42,25 @@
   +        $paragraphs = \Drupal::entityTypeManager()->getStorage('paragraph')->loadByProperties(['type' => $paragraph_type->get('id')]);
   

   Not sure if we can perform this without a chunk based process.

   We also would need to update previous revisions so reverts would work...

2. +++ b/paragraphs_collection.install
   @@ -42,10 +42,25 @@
   +          $style_name = $paragraph->getBehaviorSetting('style', 'style');
   

   Might not be present on some Paragraphs.

+++ b/paragraphs_collection.install
@@ -42,10 +42,25 @@
+
+        $paragraphs = \Drupal::entityTypeManager()->getStorage('paragraph')->loadByProperties(['type' => $paragraph_type->get('id')]);
+        foreach ($paragraphs as $id => $paragraph) {
+          $style_name = $paragraph->getBehaviorSetting('style', 'style');
+          $style_discovery = \Drupal::service('paragraphs_collection.style_discovery');
+          $style = $style_discovery->getStyle($style_name);
+          if (!isset($style['groups'])) {
+            continue;
+          }
+          $group_styles = $style_discovery->getStyleOptions($group_ids[$group]);
+          if (in_array($style_name, array_keys($group_styles))) {
+            $paragraph->setBehaviorSettings('style', ['styles' => [reset($style['groups']) => $style_name]]);
+            $paragraph->save();
+          }
+        }

Yes, this definitely doesn't scale. That's why I said we should do this at runtime, not in an update function that updates everything.

Getting there, lots of minor things and possible cleanup/simplification. Didn't review the tests closely.

1. +++ b/paragraphs_collection.install
   @@ -13,3 +15,50 @@ function paragraphs_collection_update_8001() {
   +  $group_ids = [];
   +  foreach ($groups as $group) {
   +    $group_ids[$group->getUntranslatedString()] = strtolower(str_replace(' ', '_', $group->getUntranslatedString()));
   +  }
   

   Hm, what I expected is that you do this transformation on the group definition on each style. This works too.

   However, there is no need for this awkward back-conversion of the label into the style. Those are the group definitions, the group ID is already available to you as they of the $groups array, just use it. That has the advantage that the group ids do *not* need to match the values as long as the group labels match.

2. +++ b/paragraphs_collection.install
   @@ -13,3 +15,50 @@ function paragraphs_collection_update_8001() {
   +      // Get the old configuration if the style plugin was enabled.
   +      if (!empty($group = $paragraph_type->get('behavior_plugins.style.group')) && isset($group_ids[$paragraph_type->get('behavior_plugins.style.group')])) {
   +        $default_style = $paragraph_type->get('behavior_plugins.style.default');
   

   In the first condition you set $group, but then you actually still get again.

   I would actually suggest that you move the $group assignment out of the condition on a separate line, just above it. Then the condition just becomes if ($group && isset($group_ids[$group]) which is 3x easier to read. Maybe also name it $group_label, to make it clear that we are working with the group label here.

3. +++ b/paragraphs_collection.install
   @@ -13,3 +15,50 @@ function paragraphs_collection_update_8001() {
   +        if ($style = \Drupal::service('paragraphs_collection.style_discovery')->getStyle($default_style)) {
   +          if (in_array(reset($style['groups']), array_values($group_ids))) {
   

   you still have $groups from above, keyed by group id, so you can simplify this to isset($groups[reset($style['groups'])]). might also make sense to check that $style['groups'] is !empty() first, just in case there still is a style defined that does not have a group.

4. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -81,16 +80,23 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +      $plugin_default = '';
   

   $plugin_default is a strange variable name. $default_style or $group_default would make more sense to me.

5. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -81,16 +80,23 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +        '#default_value' => $paragraph_styles[$group_id] ?: $plugin_default,
   

   Are we sure that the $group_id array key is always set or does this need a !empty()? what happens if you enable a new group and then edit existing content?

6. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +135,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
          '#options' => $this->yamlStyleDiscovery->getStyleGroups(),
   -      '#title' => $this->t('Style group'),
   -      '#description' => $this->t('Restrict available styles to a certain style group. Select "- None -" to allow all styles.'),
   -      '#default_value' => $this->configuration['group'],
   +      '#description' => $this->t('Restrict available styles to a certain style group. Select none to allow all styles.'),
   +      '#default_value' => array_keys($this->configuration['groups']),
          '#ajax' => [
            'callback' => [$this, 'updateDefaultStyle'],
   

   the description doesn't make sense anymore. you can select more than one, and you must select at least one, so the second part can go away.

7. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +135,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +
   +    foreach ($groups as $group_id => $group_default) {
   +      $default = '';
   +      if (!empty($this->configuration['groups'][$group_id]['default'])) {
   +        $default = $this->configuration['groups'][$group_id]['default'];
   +      }
   

   $group_default doesn't seem to be used but a new $default is created below which is quite confusing?

   Maybe loop over foreach (array_keys($groups) as $group_id) when you don't need the value?

8. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +135,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +        '#title' => $this->t($group_label . ' default style'),
   

   you must not pass dynamic values to t(), this must use something like @group_label

9. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -129,31 +135,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
   +        '#description' => $this->t('This style will be default option on a behavior form.'),
   

   not sure the description here is really useful as it is duplicated for each group. IMHO, either remove or include the group label again.

10. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -269,8 +301,8 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
         if ($paragraph->getParagraphType()->hasEnabledBehaviorPlugin('style')) {
    -      $default = $paragraph->getParagraphType()->getBehaviorPlugin('style')->getConfiguration()['default'];
    -      if ($paragraph_style = $paragraph->getBehaviorSetting('style', 'style', $default)) {
    +      $defaults = $paragraph->getParagraphType()->getBehaviorPlugin('style')->getConfiguration()['groups'];
    +      if ($paragraph_style = $paragraph->getBehaviorSetting('style', ['styles', key($defaults)], reset($defaults)['default'])) {
    

    ok, so we only support the first group for style templates. That's tricky, especially because we can't control which group is first.

    I think we should either support multile (which means changing the return value, but we'recompletely changing the structure of the config here as well, so I'm OK with that) or at least loop until you find the first template.

11. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -280,4 +312,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
    +   * Gets the current style config of a paragraph.
    

    ... current styles for each enabled group .. or so.

12. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -280,4 +312,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
    +   * @return array
    +   *   The list of all enabled groups with their configured style.
    

    string[] would be a better return type for this.

13. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -280,4 +312,43 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
    +  public function getStyles(ParagraphInterface $paragraph) {
    +    $paragraph_styles = [];
    +    if ($styles = $paragraph->getBehaviorSetting('style', 'styles')) {
    +      // Loop over all groups to get the behavior setting.
    +      foreach ($this->configuration['groups'] as $key => $value) {
    +        $paragraph_styles[$key] = $paragraph->getBehaviorSetting($this->getPluginId(), ['styles', $key]);
    +      }
    

    I'm missing the default fallback logic here, which would likely simplify code that is using this quite a bit.

    My recommendation would be to change the order a bit and do something like this:

    1. Create $paragraph_styles (or just $styles, not sure why we need two variables) and loop over the groups, initialize with the default value for each group.

    2. Check if you have styles configured, if yes, overwrite the defaults with the configuration that you have.

    3. If not, check if you have a single style, match that to the correct group (as you have it now).

1. +++ b/paragraphs_collection.module
   @@ -23,8 +23,10 @@
   +  if ($templates = ParagraphsStylePlugin::getStylesTemplate($paragraph)) {
   

   Hm.. geStyleTemplates() would make more sense to me as a method name.

2. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
   @@ -296,37 +289,43 @@
   -  public static function getStyleTemplate(ParagraphInterface $paragraph) {
   +  public static function getStylesTemplate(ParagraphInterface $paragraph) {
        if ($paragraph->getParagraphType()->hasEnabledBehaviorPlugin('style')) {
   -      $defaults = $paragraph->getParagraphType()->getBehaviorPlugin('style')->getConfiguration()['groups'];
   -      if ($paragraph_style = $paragraph->getBehaviorSetting('style', ['styles', key($defaults)], reset($defaults)['default'])) {
   

   question here is also if we want to keep it static or not, but I guess that's fine..

Improved variable names a bit in getStyles() but we just have too many different things in there.

Committed.