Problem/Motivation

Saving a paragraphs type creates configuration for all behavior plugins - even disabled ones.
Like so:

behavior_plugins:
  style:
    group: ''
  grid_layout:
    paragraph_reference_field: ''
    available_grid_layouts: {  }
  lockable: {  }
  primer_background:
    background_image_field: ''

This empty dummy configuration can be invalid. We should get rid of this behavior.

Proposed resolution

Change the saving behavior of paragraphs types. Disabled plugins should have not configuration stored.

Remaining tasks

User interface changes

API changes

Let /** var \Drupal\paragraphs\ParagraphsBehaviorCollection $plugin_collection */.
$plugin_collection->getConfiguration() now only contains keys for enabled plugins, while $plugin_collection->get('<diabled_plugin_id>')->getConfiguration() should still return (the default) valid plugin configuration.

The output arrays of ParagraphsBehaviorCollection::getAll, ParagraphsBehaviorCollection::getEnabled and ParagraphsTypeInterface::getEnabledBehaviorPlugins are now required to be sorted by behavior plugin weight.

The instanceIDs array of a ParagraphsBehaviorCollection is now kept sorted by plugin weight, while the pluginInstances array remains unsorted.

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

VladimirMarko created an issue. See original summary.

VladimirMarko’s picture

Assigned: Unassigned » VladimirMarko
VladimirMarko’s picture

Status: Active » Needs work
FileSize
2.26 KB

This causes test failures, because it reorders the behavior plugins in the behavior form and also in the view, for some reason.

VladimirMarko’s picture

Assigned: VladimirMarko » Unassigned

I do not like the issues this creates. Just filtering the output of ParagraphsBehaviorCollection::getConfiguration should not have caused them. As we cannot restrict when this function gets called, I do not think that there is a nice way of fixing this.

I also do not like that $plugin_collection->getConfiguration()['<diabled_plugin_id>'] and $plugin_collection->get('<diabled_plugin_id>')->getConfiguration() can now return two different results.

Unassigned.

miro_dietiker’s picture

Stop saving is about saving and submitting the paragraph type form.
We should never implement workarounds in getters to filter out garbage data, instead clean before / on save.

Let's look at \Drupal\paragraphs\Form\ParagraphsTypeForm::save

      foreach ($behavior_plugin_definitions as $id => $behavior_plugin_definition) {
        $behavior_plugin = $paragraphs_type->getBehaviorPlugin($id);

        if ($form_state->getValue(['behavior_plugins', $id, 'enabled'])) {

        else {
          // The plugin is not enabled, reset to default configuration.
          $behavior_plugin->setConfiguration([]);
        }

So it looks like here we are setting an empty array instead of unsetting the key.

VladimirMarko’s picture

We are setting the plugin configuration to default, in that code snippet. This is because of

  /**
   * {@inheritdoc}
   */
  public function setConfiguration(array $configuration) {
    $this->configuration = $configuration + $this->defaultConfiguration();
  }

in ParagraphsBehaviorBase.

The defaults are apparently needed for our code to function, so we cannot actually unset the configuration. So the only option would be to filter the configuration somewhere along the way to the export, as I did in my patch above.

miro_dietiker’s picture

Priority: Normal » Major

Plugins should not be called if they are disabled. Thus there should be no dependency in code to anything like default configuration.
If anything, we should make sure default configuration is only applied and required if it is enabled.

If a new plugin is registered, all paragraph types are changed on next export. That's really bad.

miro_dietiker’s picture

Ah.. i mixed this again with #2884236: ParagraphsBehaviorBase::submitBehaviorForm() results in in a lot of empty settings being saved
Will edit the previous comment.

It's still annoying with config export and maintenance of something like a distribution. Thus keeping major.

VladimirMarko’s picture

Assigned: Unassigned » VladimirMarko

I'll give this another attempt.

VladimirMarko’s picture

I needed to adjust multiple functions which return arrays of behavior plugin to return these ordered by plugin weight.

The last submitted patch, 10: stop_saving_of_empty-2884131-10-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

VladimirMarko’s picture

Primsi’s picture

Status: Needs review » Needs work
  1. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -18,6 +19,18 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
       /**
        * {@inheritdoc}
    +   */
    +  public function __construct(PluginManagerInterface $manager, array $configurations = []) {
    

    This one is missing the docbloc.

  2. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -26,14 +39,18 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
       public function getEnabled() {
         $this->getAll();
         $enabled = [];
    

    I guess we can remove this getAll()?

  3. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -57,7 +77,13 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
    +    $ordered_instances = [];
    +    foreach ($this->instanceIDs as $plugin_id) {
    +      $ordered_instances[$plugin_id] = $this->pluginInstances[$plugin_id];
    +    }
    

    I am not really familiar with this, so asking: can we just return $this->pluginInstances?

  4. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -68,4 +94,46 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
    +   */
    +  public function sortHelper($aID, $bID) {
    +    $aWeight = $this->manager->getDefinition($aID)['weight'];
    

    Wandering if we could use something like SortArray::sortByKeyInt here

VladimirMarko’s picture

3.
As discussed, the order of keys in instanceIDs and pluginInstances is not necessarily the same and sort (inherited form DefaultLazyPluginCollection) only sorts the former. As I did not want to change this, each time an array of plugin instances is returned (getAll, getEnabled), I need to reorder it.

I added a comment to help clarify the purpose of the loop.

Implemented the rest.

mbovan’s picture

Title: Stop saving of empty pararaphs type configuration for disabled plugins » Sort behavior plugins by weight

I think this issue does more now than what was the original scope...

Since we could use the previous contributions I retitled this issue to #2884131: Sort behavior plugins by weight and addressing the original problem in #3121776: Filter out disabled behavior plugins on the paragraphs type configuration form.

mbovan’s picture

Status: Needs review » Needs work
  1. +++ b/config/schema/paragraphs_type.schema.yml
    @@ -17,6 +17,7 @@ paragraphs.paragraphs_type.*:
    +      orderby: key
    

    This was committed in #3119488: Sort list of enabled behaviors in paragraphs.paragraphs_type configs alphabetically

  2. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -17,6 +19,24 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
    +   * Constructs a new PluginManagerInterface object.
    +   *
    +   * @param \Drupal\Component\Plugin\PluginManagerInterface $manager
    +   *   The manager to be used for instantiating plugins.
    +   * @param array $configurations
    +   *   (optional) An associative array containing the initial configuration for
    +   *   each plugin in the collection, keyed by plugin instance ID.
    +   */
    +  public function __construct(PluginManagerInterface $manager, array $configurations = []) {
    +    parent::__construct($manager, $configurations);
    +
    +    if (!empty($configurations)) {
    +      // Sort the collection to ensure the plugins are always ordered by weight.
    +      $this->sort();
    +    }
    +  }
    

    Can't we do the sort when needed, in ParagraphsBehaviorCollection::getAll()?

  3. +++ b/src/ParagraphsBehaviorCollection.php
    @@ -68,4 +101,39 @@ class ParagraphsBehaviorCollection extends DefaultLazyPluginCollection {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getConfiguration() {
    +    $configuration = parent::getConfiguration();
    +
    +    // Do not give configuration for disabled plugins.
    +    foreach ($configuration as $plugin_id => $plugin) {
    +      if (!isset($plugin['enabled']) || !$plugin['enabled']) {
    +        unset($configuration[$plugin_id]);
    +      }
    +    }
    +
    +    return $configuration;
    +  }
    

    This should be avoided as per #5.