Empty required fields not providing meaningful error messages

Sounds reasonable and nice, let's see what test bot has to say :)

Let's see test bot again, please set the issue as needs review when upload a new patch. And also an interdiff would be useful.

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1051,6 +1052,20 @@ class InlineParagraphsWidget extends WidgetBase {
+    ¶

wrong indentation here. Besides that looks nice.

can we get some example screenshots?

I tested manually a bit and seems to work nicely, the only problem I found so far is that when a paragraph with a required field is added and we have paragraphs in the same level with display mode as 'closed', we don't fill the required field of the new paragraph and we open and close the previously created paragraph it doesn't allow the user to close the old paragraph pointing to the required field of the new paragraph.

And yeah, we need the tests. :-)

Providing test coverage...

Patch works fine.
There is one more update needed.
When viewing the edit form for Paragraph. It doesn't show it as required. It should show "*" red asterisk for required paragraph field.

Updating class "form-require" changes for require paragraph fields.

re #15, there is a patch for that here: #2820502: Add form-required to required paragraph fields. If that will be included in this patch, then that one can be marked dupe.

#17 Looks good for me

Does not seem to get the #17 patch applied, I did a manual patch using #17 and here goes the patch as attachment.

@arpitr it seems your dev branch is way below the current dev. Let's retest #17 and see if it passes now.

I realised after uploading the patch and when I saw the diff, please ignore #21

#17 works for me against latest dev. (2/3 Community Reviews).

However, some coding standards need to be addressed (will roll a patch if I get time).

• File mode should not be set (100644 I think is default for this repo) Could be wrong on this one
  
  old mode 100644 / new mode 100755
• 17 - Drupal Whitespace Operator Before & After
  
  + '#max_delta' => $max-1, should be + '#max_delta' => $max - 1,
• 53 - Either use @inheritdoc or completely document the function.
  + * Validate multiple element items. should be + * {@inheritdoc}
• 80 - Either use @inheritdoc or completely document the function.
  + * Tests displaying error message on empty paragraph required field. should be + * {@inheritdoc}
• 112 - Should have a blank line after function.
  + } should be + }\n

• Re-rolled against latest dev.
• Fixed coding style issues against d.o standards.

• Fixed PHP Error with Empty Required field class setting.

We need this fix and test in the experimental widget too.

Rerolled the patch because of recent commit, as well extended the patch to experimental widget (also added test coverage). Hope it is ok :)

I like it how this is combining the two cases of the form elements, although only very few lines are won.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -799,30 +799,33 @@ class InlineParagraphsWidget extends WidgetBase {
   +      '#title' => $title,
   ...
            'title' => [
   ...
              '#value' => $title,
   

   The #title should not be set on this level in the second case, or at least wasn't before.
   Applies to both widgets.

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -799,30 +799,33 @@ class InlineParagraphsWidget extends WidgetBase {
   +      $require_class = $this->fieldDefinition->isRequired() ? 'form-required' : '';
   ...
   +          '#attributes' => ['class' => [$require_class]],
   

   I don't think we should create an empty string item here. Make the variable an array with empty default.

Fixed that.

Disregard the above. It's a patch for an entirely different issue.

This should fix it.

#37 works fine - tested with both classic and experimental widget

#37 is now working, instead the test-only should fail.

As discussed let's move the test to a new class for the UI. :)

Moved the tests to a new class, as discussed.

Converted to use only short array syntax.

Next time be careful with interdiff name. Besides that looks good. Any complaint for RTBC it?

There is still a case where this meaningless error message appears:

Steps to reproduce:
1. Create a content type
2. Add a required paragraph field to the content type
3. Create a content item of that content type
4. Create a paragraph type item and remove it but don't approve the removal
5. Try to save the content

Expected result:
Error message: "{Field name} field is required."

Actual result:
Error message: "This value should not be null."

Good catch, @ssibal! I will fix it.

I fixed that case, but did not extend the tests to cover it yet.

Fixed failing test.

Extended the test coverage.

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1239,6 +1242,31 @@ class InlineParagraphsWidget extends WidgetBase {
   /**
    * {@inheritdoc}
    */
+  public function multipleElementValidate($element, FormStateInterface $form_state, $form) {

This function should have its own documentation, there is nothing from {@inheritdoc}
Same for the one in ParagraphsWidget

Added the missing method documentation for multipleElementValidate.

The patch #70 forks for me. +1 for RTBC

1. +++ b/src/Tests/Classic/ParagraphsUiTest.php
   @@ -0,0 +1,75 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   
   +++ b/src/Tests/Experimental/ParagraphsExperimentalUiTest.php
   @@ -0,0 +1,75 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   

   We don't need this.

2. +++ b/src/Tests/Classic/ParagraphsUiTest.php
   @@ -0,0 +1,75 @@
   +    $this->drupalPostForm('admin/structure/types/manage/paragraphed_content_demo/fields/add-field', [
   +      'new_storage_type' => 'field_ui:entity_reference_revisions:paragraph',
   +      'label' => $field_title,
   +      'field_name' => 'content',
   +    ], t('Save and continue'));
   
   +++ b/src/Tests/Experimental/ParagraphsExperimentalUiTest.php
   @@ -0,0 +1,75 @@
   +    $this->drupalPostForm('admin/structure/types/manage/paragraphed_content_demo/fields/add-field', [
   +      'new_storage_type' => 'field_ui:entity_reference_revisions:paragraph',
   +      'label' => $field_title,
   +      'field_name' => 'content',
   +    ], t('Save and continue'));
   

   Hmm, this is OK but maybe we can change this a little bit and do on the way which is more common for us with $edit, the same as you did in next few lines. I think will be more readable.

Implemented the changes.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -850,30 +850,33 @@ class InlineParagraphsWidget extends WidgetBase {
   +      $classes = $this->fieldDefinition->isRequired() ? ['form-required'] : [];
   

   I would say "class" instead of "classes".

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -881,7 +884,7 @@ class InlineParagraphsWidget extends WidgetBase {
   -          ]
   

   Unrelated change.

3. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1237,6 +1240,39 @@ class InlineParagraphsWidget extends WidgetBase {
   +    $remove_mode_item_count = 0;
   +    if (isset($widget_state['paragraphs'])) {
   +      foreach ($widget_state['paragraphs'] as $paragraph) {
   +        if ($paragraph['mode'] == 'remove') {
   +          $remove_mode_item_count++;
   +        }
   +      }
   +    }
   

   Maybe to create helper method and put these code there. Would be nice if we can send mode as a parameter and make it to works for all modes.

4. +++ b/src/Tests/Classic/ParagraphsUiTest.php
   @@ -0,0 +1,69 @@
   +    $this->drupalPostForm('admin/structure/types/manage/paragraphed_content_demo/fields/add-field', $edit, t('Save and continue'));
   

   The line is so long. Split it in multi lines, drupalGet('.....') before $edit.

1. As discussed: It's an array that contains classes. (One or none, in this case.) We commonly just use plural for those, in Drupal. As in: $file = current($files);, even if we know that there is just one file in $files.

Implemented the rest.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1237,6 +1240,31 @@ class InlineParagraphsWidget extends WidgetBase {
   +    $non_remove_mode_item_count = $widget_state['real_item_count'] - $remove_mode_item_count ;
   
   +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -1290,6 +1293,31 @@ class ParagraphsWidget extends WidgetBase {
   +    $non_remove_mode_item_count = $widget_state['real_item_count'] - $remove_mode_item_count ;
   +
   

   Extra space before ';'.

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1401,6 +1429,32 @@ class InlineParagraphsWidget extends WidgetBase {
   +   * @param string $mode
   +   *   The mode to look for.
   +   * @return int
   
   +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -1469,6 +1497,32 @@ class ParagraphsWidget extends WidgetBase {
   +   * @param string $mode
   +   *   The mode to look for.
   +   * @return int
   +   *   The number of paragraphs is the given mode.
   

   Separate the @param and @return sections by a blank line.

3. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1401,6 +1429,32 @@ class InlineParagraphsWidget extends WidgetBase {
   +  protected function getNumberOfParagraphsInMode($widget_state, $mode) {
   
   +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -1469,6 +1497,32 @@ class ParagraphsWidget extends WidgetBase {
   +  protected function getNumberOfParagraphsInMode($widget_state, $mode) {
   

   Type hint "array" missing for $widget_state.

4. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -925,7 +928,7 @@ class ParagraphsWidget extends WidgetBase {
   -          ]
   

   Still unrelated change.

5. +++ b/src/Tests/Classic/ParagraphsUiTest.php
   @@ -0,0 +1,70 @@
   +    $field_title = 'Content Test';
   +    $edit = [
   +      'new_storage_type' => 'field_ui:entity_reference_revisions:paragraph',
   +      'label' => $field_title,
   +      'field_name' => 'content',
   +    ];
   +    $this->drupalGet('admin/structure/types/manage/paragraphed_content_demo/fields/add-field');
   +    $this->drupalPostForm(NULL, $edit, t('Save and continue'));
   +    $this->drupalPostForm(NULL, [], t('Save field settings'));
   +    $edit = [
   +      'required' => TRUE,
   +    ];
   +    $this->drupalPostForm(NULL, $edit, 'Save settings');
   +    $this->assertText('Saved ' . $field_title . ' configuration.');
   
   +++ b/src/Tests/Experimental/ParagraphsExperimentalUiTest.php
   @@ -0,0 +1,70 @@
   +    $edit = [
   +      'new_storage_type' => 'field_ui:entity_reference_revisions:paragraph',
   +      'label' => $field_title,
   +      'field_name' => 'content',
   +    ];
   +    $this->drupalGet('admin/structure/types/manage/paragraphed_content_demo/fields/add-field');
   +    $this->drupalPostForm(NULL, $edit, t('Save and continue'));
   +    $this->drupalPostForm(NULL, [], t('Save field settings'));
   +    $edit = [
   +      'required' => TRUE,
   +    ];
   +    $this->drupalPostForm(NULL, $edit, 'Save settings');
   

   We have static function ''fieldUIAddNewField'', maybe you can use it.

6. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1401,6 +1429,32 @@ class InlineParagraphsWidget extends WidgetBase {
   +   * @param string $mode
   +   *   The mode to look for.
   +   * @return int
   +   *   The number of paragraphs is the given mode.
   

   Separate the @param and @return selections by a blank line.

4. Removed from that class as well.

5. Great suggestion! Implemented.

Implemented everything.

Looks fine now.

Quick test: the error message now indeed shows the field label. But the field itself is not marked as an error.

re-rolled patch against the latest dev 31fe6df since it was not applying.

Committed, thx, lots of attributions. :-)

Still authoring @andywhale as the original fixing logic came from him.

For those who are locked in paragraphs to v1.1 and want to use the patch i had to adapt the 86 patch a bit, just removed the assert in ParagraphsConfigTest.php since the method is missing.

Hi, the bug is still reproducible on current -dev version. If you have a required field on a paragraph and you save the node the message error is:

This value should not be null

I have recently faced this issue with the media library widget used in the media field of the paragraph (v1.12), got resolved after changing the widget to entity browser.