Allow default opt-out for single paragraph type

We need to wait this Related to be committed.

Related issue Allow to select a default paragraph type that will be shown for an empty field is almost done. Let's discuss here how we want to handle with single paragraph type.

Then we postpone it for the other issue.

Discussed with @miro_dietiker:
Some thoughts:
- for the case when a paragraph is limited to one type... it's really nice to see that form already present.
- there is only one concerning thing: the paragraph is always stored when saving... even if all the fields remain empty, the problem with the save is, we can't apply isEmpty() to the fields and skip saving. If the paragraph type has special meaning, we might skip saving of something meaningful.
- if a paragraph is truly an optional thing, it's bad to remove it every time

Unpostponing.

Berdir finds that isEmpty() check and skip saving is the wrong approach.

So if we really need to opt out, we need to make a difference between _default and _none. We can then default to _default that will offer the single paragraph type, plus allow a setting to explicitly select none.

Not sure how common this requirement is. We could also postpone until people report real need.

However, as long as we don't have a setting that makes sense, we should hide the setting completely for fields allowing only a single paragraph type.

Having a single-type paragraph form is undesirable behavior. None means none, not default.

@Taran2L that is for sure true, none is none, if the user selected none in the form display settings of that field is obvious that the field will not have a default paragraphs type added by default, what this issue proposed is to when there is just one paragraph type we want to show that as default as long as the user didn't select none. And, when the paragraph is there but the fields are not filled it should be removed.

I just tested and @Taran2L is right, even if none is selected the default paragraph type is still added. Promoting to fix that.

@Taran2L, the none is not working only for single paragraph type, if you check when there are multiple paragraphs type you can see the that is working fine. The bug with single type should fixed in this issue.

Soon after we committed to add the default, we have a user complaining about the "always default" behavior:
#2770507-49: Allow to select a default paragraph type that will be shown for an empty field

So we need to allow opt-out.

I can provide a patch today

Patch attached. Please review

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1310,10 +1310,6 @@ class InlineParagraphsWidget extends WidgetBase {
-    if (count($allowed_types) === 1) {
-      return key($allowed_types);

This should still happen by default. But what we need is a setting such as _none and that will trigger to return NULL.

With all the respect, I totally disagree with the proposed workflow. It's just a bad UX, both for site builders/developers and content managers.

You are creating an exception in behavior. Which is not a good thing in any case. When there are multiple paragraph types - nothing is being shown and someone must specify a default paragraph type. Single paragraph type works in the opposite manner. Which is confusing.

UX wise, this is just a default behavior. If you look at the settings, they look the same for the user / site builder.
With this design, we just want to make the default behavior better.
And as we learned from Field Collections users, this is important.

If we don't do this, configuring a default paragraph type needs settings at two different locations:
First configure / enable the single paragraph type (the ERR field)
Then, edit the form display widget settings to select the very same type as default.
We thought that's cumbersome.
We can't put those settings to the paragraph type selection list as we think it is a widget thing.

Note that the single paragraph type already received and will receive more special attention and slightly behave differently.

While this is not yet fully set into stone, i'm open to hear more opinions to revert this proposed behavior.

That means that you now differ between NULL and an empty string. I don't like that, lets use an explicit _none or so instead. Try #empty_option on the form element.

Also test that before setting it to _none explicitly, that the single type is picked by default.

If the user doesn't want a default paragraph type, 'default_paragraph_type' now gets the value '_none'.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -86,7 +86,7 @@ class InlineParagraphsWidget extends WidgetBase {
          'add_mode' => 'dropdown',
          'form_display_mode' => 'default',
   -      'default_paragraph_type' => '',
   +      'default_paragraph_type' => '#empty_option',
        );
   

   The default is still empty string. or NULL. But not this :)

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1360,9 +1360,14 @@ class InlineParagraphsWidget extends WidgetBase {
        if (count($allowed_types) === 1) {
   -      return key($allowed_types);
   +        return key($allowed_types);
        }
   

   incorrect indendation.

3. +++ b/src/Tests/ParagraphsAddModesTest.php
   @@ -193,4 +193,30 @@ class ParagraphsAddModesTest extends ParagraphsTestBase {
   +  /**
   +   * Tests if settings for default paragraph type is working properly for single
   +   * type paragraphs.
   +   */
   +  public function testSettingDefaultParagraphTypeWithSingleType() {
   

   The first line should be < 80 characters.

   Tests the default paragraph type behavior for a field with a single type.

   Also, in this test, lets ensure the default behavior by going to node/add/... before setting it to _none and make sure there is a default.

I implemented the suggested changes.

Looks good, just two small things.

1. +++ b/src/Tests/ParagraphsAddModesTest.php
   @@ -193,4 +193,34 @@ class ParagraphsAddModesTest extends ParagraphsTestBase {
   +   * Tests the default paragraph type behavior for a field with a single type.
   

   oops, this is not related with the behavior. Remove that word only.

2. +++ b/src/Tests/ParagraphsAddModesTest.php
   @@ -193,4 +193,34 @@ class ParagraphsAddModesTest extends ParagraphsTestBase {
   +  public function testSettingDefaultParagraphTypeWithSingleType() {
   

   Change this test name to testDefaultParagraphTypeWithSingleType.

1. Then that sentence wouldn't make sense anymore. It has nothing to with behavior plugins, but it still has to do with how it/the widget *behaves* for a single paragraph. Maybe "Tests the widget default paragraph logic type with a single type" ?

Changed testSettingDefaultParagraphTypeWithSingleType to testDefaultParagraphTypeWithSingleType.

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1360,7 +1360,12 @@ class InlineParagraphsWidget extends WidgetBase {
+    // Check if the user explicitly selected not to have any default Paragraph
+    // type. Othewise, if there is only one type available, that one is the
+    // default.
+    if ($default_type === '_none') {
+      return NULL;
+    }

IMHO I think we don't need this comment here, it is clear enough what we are doing.

+++ b/src/Tests/ParagraphsAddModesTest.php
@@ -193,4 +193,34 @@ class ParagraphsAddModesTest extends ParagraphsTestBase {
+    // Check that when only one paragraph type is allowed in a content type,
+    // one instance is automatically added in the 'Add content' dialogue.
+    $this->drupalGet('node/add/paragraphed_test');
+    $this->assertNoText('No Paragraph added yet.');

EDIT:This is OK, I was confused.
So, when user chose only one paragraph type, that type should set as default, and after that if user doesn't want to use that type as default he can go to settings and chose none.

The comment is fine, in my opinion. Better to have a few comments too many than a few too few.

The test works as intended - that assert differentiates between the two cases being tested.

The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures. We must have them because we cannot always figure out how to express ourselves without them, but their use is not a cause for celebration.So when you find yourself in a position where you need to write a comment, think it through and see whether there isn’t some way to turn the tables and express yourself in code. Every time you express yourself in code, you should pat yourself on the back. Every time you write a comment, you should grimace and feel the failure of your ability of
expression.Why am I so down on comments? Because they lie. Not always, and not intentionally, but too often. The older a comment is, and the farther away it is from the code it describes, the more likely it is to be just plain wrong. The reason is simple. Programmers can’t realistically maintain them.Code changes and evolves. Chunks of it move from here to there. Those chunks bifurcate and reproduce and come together again to form chimeras. Unfortunately the comments don’t always follow them—can’t always follow them. And all too often the comments get separated from the code they describe and become orphaned blurbs of everdecreasing accuracy.

Explain yourself in code.

There are certainly times when code makes a poor vehicle for explanation. Unfortunately,many programmers have taken this to mean that code is seldom, if ever, a good means forexplanation. This is patently false. Which would you rather see? This:

// Check to see if the employee is eligible for full benefits
if ((employee.flags & HOURLY_FLAG) && (employee.age > 65))
Or this?
if (employee.isEligibleForFullBenefits())

It takes only a few seconds of thought to explain most of your intent in code. In many cases it’s simply a matter of creating a function that says the same thing as the comment you want to write.

http://www.kyleblaney.com/software-blog/2012/6/29/comments-are-a-failure...

I think a comment there makes sense, it is not that obvious, the whole method is fairly complex. We needed quite a few iterations to get it right.

This looks good to me.

I like. :-)

Hm, maybe it's better to introduce a constant versus string '_none'?

Oops, changed a status by mistake.