We save all form values, even if they're empty, which is quite common for optional behavior plugins.

We should probably at least put an array_filter() around it.

CommentFileSizeAuthor
#17 2884236-17.patch6.02 KBVladimirMarko
#17 interdiff-2884236-12-17.txt737 bytesVladimirMarko
#17 2884236-17-test-only.patch3.74 KBVladimirMarko
#12 2884236-12.patch6.01 KBVladimirMarko
#12 interdiff-2884236-10-12.txt2.09 KBVladimirMarko
#12 2884236-12-test-only.patch3.72 KBVladimirMarko
#10 2884236-10.patch6.01 KBVladimirMarko
#10 interdiff-2884236-8-10.txt6.31 KBVladimirMarko
#10 2884236-10-test-only.patch3.72 KBVladimirMarko
#8 2884236-8.patch2.22 KBVladimirMarko
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

Is this duplicate?
#2884131: Sort behavior plugins by weight

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Closed (duplicate)
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Closed (duplicate) » Active

Actually it is not. That is about paragraph type settings, this about paragraph settings.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Priority: Normal » Major

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

This is polluting the content storage and making our life harder. We should really try to clean this.

tbonomelli’s picture

tbonomelli CreditAttribution: tbonomelli at MD Systems GmbH commented
Assigned: Unassigned » tbonomelli
tbonomelli’s picture

tbonomelli CreditAttribution: tbonomelli at MD Systems GmbH commented
Assigned: tbonomelli » Unassigned
VladimirMarko’s picture

VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented
Assigned: Unassigned » VladimirMarko
Status: Active » Needs review
FileSize
2884236-8.patch2.22 KB

As we cannot know what a plugin considers to be a default value, ParagraphsBehaviorBase now considers all empty values to be default and won't save them.

If a behavior plugin wants to distinguish between different empty values, it will have to override either the submitBehaviorForm method or at least the new filterBehaviorFormSubmitValues method.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
  1. +++ b/src/ParagraphsBehaviorBase.php
@@ -130,7 +131,43 @@ abstract class ParagraphsBehaviorBase extends PluginBase implements ParagraphsBe
+   * Default implementation considers a value to be default iff it is an empty
+   * value. Behavior plugins that do not consider all empty values to be default

    typo: iff 

  2. +++ b/src/ParagraphsBehaviorBase.php
@@ -130,7 +131,43 @@ abstract class ParagraphsBehaviorBase extends PluginBase implements ParagraphsBe
+   * @return array
+   *   An associative array of values submitted to the form with empty leaves
+   *   recursively removed.

    recursively removed is an implementation detail, that's not part of the API and shouldn't be mentioned here. 

  3. +++ b/src/ParagraphsBehaviorBase.php
@@ -130,7 +131,43 @@ abstract class ParagraphsBehaviorBase extends PluginBase implements ParagraphsBe
+    do {
+      $old_array = $new_array;
+      $new_array = NestedArray::filter($old_array);
+    }
+    while ($new_array !== $old_array);

    This needs a comment. Also not convinced that we need to pass in $values *and* $form_state, if you pass in $form_state it is easy enough to call getValues() on it.

    strange that the parent doesn't have the nesting and filtering itself reversed, then this loop wouldn't be necessary.

We should be able to test this by checking what gets saved by our test plugins and maybe add a nested example if we don't have that yet.

VladimirMarko’s picture

VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2884236-10-test-only.patch3.72 KB
interdiff-2884236-8-10.txt6.31 KB
2884236-10.patch6.01 KB

Here you go.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Needs work

That test-only was supposed to fail i guess? :-)

VladimirMarko’s picture

VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2884236-12-test-only.patch3.72 KB
interdiff-2884236-10-12.txt2.09 KB
2884236-12.patch6.01 KB

Yeah. The test class was wrongly namespaced.

I also fixed some coding standards violations.

The last submitted patch, 12: 2884236-12-test-only.patch, failed testing. View results

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Needs work

So we are removing empty values only, and recursively.
It also seems that keys with assigned values FALSE (array_filter()) are removed.

But it also seems that if a plugin provides default values, those are stored.

Not sure if this is what we exactly want and what are our options. Thinking about it:

Say, for instance, if a plugin proposes that a setting is enabled by default, that setting is present in all paragraph types by default value even if the plugin is not enabled. This seems odd, right?

VladimirMarko’s picture

VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented

@miro_dietiker:
There's not much we can do about that. Plugins define their settings themselves and we cannot know what any given plugin considers to be its default settings without manually inspecting its buildBehaviorForm method.
(Or doing something really dodgy like cloning its Paragraph, removing its current plugin settings and building, validating and submitting a new settings form for it.)

The problem is that we have provided a default implementation of submitBehaviorForm in ParagraphsBehaviorBase that stores all the settings from the form and most plugin authors did not see the need to override that method to reduce the amount of junk settings their specific plugin stores.

So to do anything at all, this patch needs to be opinionated about what constitutes default settings. If a behavior plugin author disagrees with this opinion, he should either override ParagraphsBehaviorBase::submitBehaviorForm, override ParagraphsBehaviorBase::filterBehaviorFormSubmitValues, or not use ParagraphsBehaviorBase at all.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/tests/src/Functional/ParagraphsExperimentalBehaviorsTest.php
@@ -0,0 +1,95 @@
+    // Add a note that uses the behavior plugin give it an empty setting.
+    $this->drupalGet('node/add/');

strange trailing slash, also this relies on the feature that it redirects to the single node type if there is only one. lets save a redirect and go to the form directly.

VladimirMarko’s picture

VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2884236-17-test-only.patch3.74 KB
interdiff-2884236-12-17.txt737 bytes
2884236-17.patch6.02 KB

The last submitted patch, 17: 2884236-17-test-only.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

@Miro: I think you might have tested the wrong thing here. This is about the per-paragraph behavior settings, there is no such thing as disabled plugins here.

What you are looking for is #2884131: Sort behavior plugins by weight, which should solve the problem of disabled plugin configuration.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Oh :-) committed.

Status: Fixed » Closed (fixed)

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