Problem/Motivation

Recently there have been a lot of Paragraphs validation fixes. But we also need to validate closed Paragraphs. e.g. When there are invalid references in a entity reference field inside a paragraph.

Proposed resolution

Use $entity->validate(). If there are validation errors, just list them within the closed paragraph as error messages. On submit, fail validation on them, just add the messages on the current form element.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#52 interdiff-2733987-48-52.txt690 bytesModernMantra
#52 validate_closed_paragraphs-2733987-52.patch4.38 KBModernMantra
#48 validate_closed_paragraphs-2733987-48.patch4.38 KBModernMantra
#48 Screenshot from 2016-12-12 13-21-50.png18.5 KBModernMantra
#46 interdiff-2733987-43-46.txt1.63 KBModernMantra
#46 closed_paragraphs-2733987-46.patch4.53 KBModernMantra
#43 interdiff-2733987-41-43.txt669 bytesModernMantra
#43 closed_paragraphs-2733987-43.patch4.27 KBModernMantra
#41 Screenshot from 2016-11-30 12-09-58.png35.65 KBModernMantra
#41 interdiff-2733987-40-41.txt2.09 KBModernMantra
#41 closed_paragraphs-2733987-41.patch4.27 KBModernMantra
#40 closed_paragraphs-2733987-40.patch3.27 KBModernMantra
#36 interdiff-2733987-33-36.txt1.18 KBModernMantra
#36 closed_paragraphs-2733987-36.patch6.94 KBModernMantra
#33 closed_paragraphs-2733987-33.patch5.65 KBModernMantra
#30 closed_paragraphs-2733987-30.patch5.64 KBModernMantra
#26 interdiff-2733987-25-26.txt2.01 KBModernMantra
#26 closed_paragraphs-2733987-26.patch6.31 KBModernMantra
#25 Screenshot from 2016-11-16 17-26-08.png42.02 KBModernMantra
#25 closed_paragraphs-2733987-25.patch6.27 KBModernMantra
#21 closed_paragraphs-2733987-TEST_ONLY.patch4.98 KBModernMantra
#20 interdiff-2733987-14-20.txt3.43 KBModernMantra
#20 closed_paragraphs-2733987-20.patch6.22 KBModernMantra
#14 interdiff-2733987-12-14.txt4.15 KBModernMantra
#14 closed_paragraphs-2733987-14.patch6.58 KBModernMantra
#12 interdiff-2733987-9-12.txt2.32 KBModernMantra
#12 closed_paragraph-2733987-12.patch5.01 KBModernMantra
#9 closed_paragraphs-2733987-9.patch4.41 KBModernMantra
#7 closed_paragraphs-2733987-7.patch2.79 KBModernMantra
#2 term_removed.png38.44 KBpguillard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yongt9412 created an issue. See original summary.

pguillard’s picture

Status: Active » Needs work
FileSize
38.44 KB

Here is what I did to reproduce :

  • I created a paragraph with taxonomy terms
  • Added this test paragraph to basic_page content type
  • Created a node/page with 3 terms, tag1, tag2, tag3
  • Deleted tag2
  • Get back to the node to check validation : The term has been removed, and I get logically no validation errors : Term Removed

@yongt9412, is this what you meant ?

pguillard’s picture

Status: Needs work » Needs review
johnchque’s picture

Status: Needs review » Active

We are talking about Paragraphs that have the mode set as "Closed" and a better way to fix this is adding a $entity->validate()

May I ask why did you change the status? :)

pguillard’s picture

Thanks & Sorry for that. I changed the status because Open issues may stay without response.

miro_dietiker’s picture

because Open issues may stay without response

@pguillard i would still recommend to always first try an update with following the issue status definition:
https://www.drupal.org/node/73178
See also https://www.drupal.org/node/156119

ModernMantra’s picture

Some progress... Created only tests to verify the 'problem'. Still needs some work on test before shifting to making a patch :).

miro_dietiker’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

When testing and adding required fields to closed paragraphs, i even ended up with fatal errors when saving.
And collapsed paragraphs were no more expandable.

So as a result, the user/editor can't resolve the situation anymore, making it a critical bug.

Identified two very common cases:
A required field that is in a collapsed paragraph.
A reference to a target (term, node, ..) that is no more existing.
We should test cover both - and the implementation should cover all validation errors for sure.

ModernMantra’s picture

Some more progress and improvement. I am leaving this issue currently to someone else to make improvements/progress...
Keeping the status to needs work despite new patch, since it needs more improvements to achieve goal of setting to 'needs review' :)

miro_dietiker’s picture

Status: Needs work » Needs review

Still mr. Testbot, what's the status?

miro_dietiker’s picture

Status: Needs review » Needs work

Hm #10 should be a failing test, but you commented out the failing line.
The second case is incomplete.

This issue should not only validate, but also open the (potentially closed) paragraphs on validation error. This also needs to work with nesting, so the tests should also contain a nested situation, or we should simply extend the nested test with a validation problem.
Fixing this problem is pretty challenging. :-)

ModernMantra’s picture

Test that was incomplete (as stated in #11) should be fine now. Some small improvements. I have explored the issue and seems we need some big revision here, since tests are not failing on validating closed paragraph, and also manually when done in UI no fails. Either tests are wrong, or issue is interpreted wrong...

Berdir’s picture

Status: Needs review » Needs work

My understanding was that a validation should show validation errors in close paragraphs. If you already want to show it when we look at it, then sure, that is possible too. We just need to move the validate code as we discussed to where we show the closed mode and list the errors there.

We did discuss that, so I'm not sure why that is not part of the patch in some way ? (calling validate(), displaying the validation messages at least as a drupal_set_message().

As to your tests, the first doesn't actually have any assertions for the validation messages and the second doesn't close the paragraphs, that's why it does work there?

ModernMantra’s picture

Providing the patch that offers validation of closed paragraph, changed tests by adding some assertions.

miro_dietiker’s picture

Status: Needs review » Needs work

Hm, we should at least show the paragraphs open when they have a validation error.

How about collapsing a paragraph, should/can we call validation there too?

Unsure if we should force open paragraphs with required fields. That should IMHO be a separate issue discussion.

Are the tests also containing a nested example or do we need to add that?

Berdir’s picture

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -571,6 +570,21 @@ class InlineParagraphsWidget extends WidgetBase {
+          if (!empty($violation)) {
+            $render_array = [
+              [
+                '#theme' => 'item_list',
+                '#items' => $violation->getMessage(),
+              ],
+            ];
+            $message = \Drupal::service('renderer')->renderPlain($render_array);
+            drupal_set_message($message, 'error');

that's not what I meant.

drupal_set_message() was for a first tes. to display a list, you want to add it to $element subform as initialized below.

Also, this builds a list for each message. you want to have a single list with multiple messages.

Berdir’s picture

@Miro: Showing messages *and* open doesn't make sense to me. Either we show messages or we open them. But if we open, then we wouldn't show validation errors upfront, that's not how forms otherwise work.

I find opening them by default a bit tricky as it might not be clear to the user, especially if you have multiple/many with errors.

miro_dietiker’s picture

I'm confused. :-)

I didn't intend to display a message upfront.
I was only talking about validating on save. In that case, we would display the error message and try to make the element in question visible.

We might want to discuss and define the exact user stories for the different resulting states?

Berdir’s picture

Use $entity->validate(). If there are validation errors, just list them within the closed paragraph as error messages. On submit, fail validation on them, just add the messages on the current form element.

That seems like a pretty clear "validate upfront" to me?

That was always my goal and what I've been working towards, although possibly not always directly visible in the patches, either due to misunderstandings of what I wanted and partially getting first patches out that do a bit of validation and show it somewhere.

ModernMantra’s picture

Made some improvements in validation and small fix in tests. Now patch should look better for review :-)

ModernMantra’s picture

Status: Needs review » Needs work

The last submitted patch, 21: closed_paragraphs-2733987-TEST_ONLY.patch, failed testing.

The last submitted patch, 21: closed_paragraphs-2733987-TEST_ONLY.patch, failed testing.

johnchque’s picture

Looks nice, but the access test is a bit different, can we move the tests to new class please? something like ParagraphsValidationTest :)
Also please add an screenshot about the changes and how the messages are shown. :)

ModernMantra’s picture

Refactored test to new class, no changes in the code. Providing the image to verify how the fix looks (nothing special, still needs some discussion and work).

ModernMantra’s picture

After some discussion with @berdir, after validation failed there will be no error message but rather opened paragraphs instead of closed.

Status: Needs review » Needs work

The last submitted patch, 26: closed_paragraphs-2733987-26.patch, failed testing.

The last submitted patch, 26: closed_paragraphs-2733987-26.patch, failed testing.

miro_dietiker’s picture

  1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -214,6 +213,14 @@ class InlineParagraphsWidget extends WidgetBase {
    +      if($violations) {
    

    Missing space.

  2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -570,9 +577,6 @@ class InlineParagraphsWidget extends WidgetBase {
    -      elseif ($item_mode == 'closed') {
    -        $element['subform'] = array();
    

    I don't see why this should change.

  3. +++ b/src/Tests/ParagraphsValidationTest.php
    @@ -0,0 +1,138 @@
    +class ParagraphsValidationTest extends WebTestBase {
    

    Why not based on ParagraphsTestBase? I guess it will simplify the test.

ModernMantra’s picture

Made big refactoring considering previous comment...

Status: Needs review » Needs work

The last submitted patch, 30: closed_paragraphs-2733987-30.patch, failed testing.

The last submitted patch, 30: closed_paragraphs-2733987-30.patch, failed testing.

ModernMantra’s picture

Fixing the test failures. On local machine seems ok, lets see test bot.

Status: Needs review » Needs work

The last submitted patch, 33: closed_paragraphs-2733987-33.patch, failed testing.

The last submitted patch, 33: closed_paragraphs-2733987-33.patch, failed testing.

ModernMantra’s picture

Failure was related to existing test who tries to edit paragraph which is already in edit mode (because of patch). So added small test support to verify that we are in edit mode, as well removed some lines of tests. Hope test bot will turn to green :-)

johnchque’s picture

Status: Needs review » Needs work

I was writing a really big review when I realized that:

+++ b/src/Tests/ParagraphsValidationTest.php
@@ -0,0 +1,145 @@
+class ParagraphsValidationTest extends ParagraphsTestBase {

We can remove this test class.

Instead of adding more tests. Please do:

  1. +++ b/src/Tests/ParagraphsAdministrationTest.php
    @@ -533,11 +533,13 @@ class ParagraphsAdministrationTest extends WebTestBase {
    +    $this->assertRaw('field_paragraphs[0][subform][field_entity_reference][0][target_id]');
    +    $this->assertRaw('field_paragraphs[0][subform][field_entity_reference][1][target_id]');
    

    This has to be assertFieldByName

  2. +++ b/src/Tests/ParagraphsAdministrationTest.php
    @@ -533,11 +533,13 @@ class ParagraphsAdministrationTest extends WebTestBase {
         // Try to collapse with an invalid reference.
         $this->drupalPostAjaxForm(NULL, ['field_paragraphs[0][subform][field_entity_reference][0][target_id]' => 'foo'], 'field_paragraphs_0_collapse');
    

    This part is important, when trying to collapse, just below this line, assert that the error message is shown. What the tests is doing in there is trying to collapse with an invalid reference, that is what your second test was doing. So assert in there the error message.

johnchque’s picture

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -214,6 +213,14 @@ class InlineParagraphsWidget extends WidgetBase {
+      $violations = $paragraphs_entity->validate();
+      $violations->filterByFieldAccess();
+      if (count($violations) > 0) {
+        $item_mode = 'edit';
+      }

Btw after this you should add a $form_state->setErrorByName($violations, 'This field is needed');
or something similar.
Or loop over all the field in the violations variable to add a message when trying to collapse.

johnchque’s picture

+++ b/src/Tests/ParagraphsAdministrationTest.php
@@ -533,11 +533,13 @@ class ParagraphsAdministrationTest extends WebTestBase {
     // Try to collapse with an invalid reference.
     $this->drupalPostAjaxForm(NULL, ['field_paragraphs[0][subform][field_entity_reference][0][target_id]' => 'foo'], 'field_paragraphs_0_collapse');

Btw just before this, set the reference field as required.

ModernMantra’s picture

So dropped custom test class, added some asserts and improvements as suggested in comments #37,38,39. As discussed with @berdir, thus keeping paragraphs opened without error message.

ModernMantra’s picture

As again discussed with @berdir, fixes consists of showing small error message and opening paragraphs with validation error (check image). Discussed and would be nice top open issue to change warning message formatting. So far colors and style seems not so 'cool' and eye catching.

Berdir’s picture

  1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -214,6 +214,22 @@ class InlineParagraphsWidget extends WidgetBase {
    +        $info['validation_error'] = array(
    +          '#type' => 'markup',
    +          '#markup' => '<em class="color-warning">' . $this->t(implode(', ', $messages)) . '</em>',
    +        );
    

    a comma separated list of those messages is IMHO going to look pretty weird. But we can always improve that later.

  2. +++ b/src/Tests/ParagraphsAdministrationTest.php
    @@ -533,11 +536,19 @@ class ParagraphsAdministrationTest extends WebTestBase {
         $this->drupalPostAjaxForm(NULL, ['field_paragraphs[0][subform][field_entity_reference][0][target_id]' => 'foo'], 'field_paragraphs_0_collapse');
    -    $this->assertText('There are no entities matching "foo".');
    +    // Paragraph should be still in edit mode.
    +    $this->assertFieldByName('field_paragraphs[0][subform][field_entity_reference][0][target_id]');
    +    $this->assertFieldByName('field_paragraphs[0][subform][field_entity_reference][1][target_id]');
         // Attempt to remove the Paragraph.
         $this->drupalPostAjaxForm(NULL, [], 'field_paragraphs_0_remove');
    

    Looks like we lost the assert on the validatin message here?

ModernMantra’s picture

Now it is '\n' separated list (it was really bad with comma as said). Yes regarding #42.2, this message is lost since we are keeping paragraph in edit mode

ModernMantra’s picture

As discussed with @miro_dietiker, opening issue to change appearance of error message.

miro_dietiker’s picture

ModernMantra’s picture

Extending little bit test coverage as suggested by @miro_dietiker.

Status: Needs review » Needs work

The last submitted patch, 46: closed_paragraphs-2733987-46.patch, failed testing.

ModernMantra’s picture

Rerolling the patch and some refactoring, now we are displaying 'validation warning message' in bit different way.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -189,6 +189,7 @@ class InlineParagraphsWidget extends WidgetBase {
+    $info = array();

[].

Looks good, but when I add a new Paragraph without having the reference field as required the message is gone.

Berdir’s picture

Status: Needs work » Needs review

Yes because we already switched the type to open and don't validate anymore. But the form *is* open and you will get the validation errors when you try to submit. IMHO, that is good enough, anything else gets complicated.

Berdir’s picture

Status: Needs review » Needs work

Needs work for the array()

ModernMantra’s picture

fixed array notation :-)

johnchque’s picture

Status: Needs review » Reviewed & tested by the community

Retested, still applies. Good!

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, committing. More integrity! :-)
Created the follow-up to validate nested paragraphs for completeness.

johnchque’s picture

Status: Fixed » Closed (fixed)

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

mogio_hh’s picture

With me this is not working as the validate function of my custom field is not able to check the value of the field when the paragraph is collapsed.
The returned value is all the time null.

  public function validate($element, FormStateInterface $form_state) {
    $value = $element['#value'];
}

Do I have to handle validations in a different way when they are used in preview displays?
It seems as if the form values are not accessible by the fieldWidget when validating.

lukasss’s picture

I also can't verify $form_state->getValue() in the custom entity when paragraph is closed