The widget form code for "Validate closed paragraphs and expand if needed." checks all fields on the paragraph and does not limit validation to the fields in the form display.

When the paragraphs widget validates a "closing" form, then it does not highlight any field and shows errors messages that are confusing to editors "This value should not be null.".

Proposed:
1. Allow the widget elementValidate() to flag the errors on the widget forms as it normally would. This provides human understandable error messages and highlights the field.
FIX to implement: Update collapse_button to limit validation to the field item.

2. Keep on form build validation for "" in case there is some edge case that is it still needed.
FIX to implement: Update code to check for a field widget similar to EntityFormDisplay::flagWidgetsErrorsFromViolations().

CommentFileSizeAuthor
#65 respect-display-on-validation-2890086-65.patch9.2 KBjoevagyok
#64 respect-display-on-validation-2890086-64.patch13.98 KBjoevagyok
#63 2890086-63.patch9.65 KBaporie
#55 respect-display-on-validation-2890086-55.patch9.26 KBsergiur
#52 2890086-52.patch9.08 KBkarishmaamin
#47 2890086-47.patch9.38 KBmbovan
#46 2890086-46.patch9.37 KBsuresh prabhu parkala
#42 interdiff_41-42.txt686 bytessardara
#42 paragraphs-validation-to-respect-form-display-2890086-42.patch9.27 KBsardara
#41 interdiff-2890086-36-41.txt1.47 KBademarco
#41 paragraphs-validation-to-respect-form-display-2890086-41.patch9.3 KBademarco
#36 paragraphs-validation-to-respect-form-display-2890086-36.patch8.13 KBb_sharpe
#33 paragraphs-validation-to-respect-form-display-2890086-32.patch8.28 KBb_sharpe
#30 paragraphs-validation-to-respect-form-display-2890086-30-2.patch8.16 KBrecrit
#30 interdiff-2890086-28-30.txt2.26 KBrecrit
#28 paragraphs-validation-to-respect-form-display-2890086-28.patch8.17 KBrecrit
#28 interdiff-2890086-26-28.txt4.32 KBrecrit
#26 paragraphs-validation-to-respect-form-display-2890086-26.patch7.7 KBrecrit
#18 paragraphs-validation-to-respect-form-display-2890086-18.patch8.61 KBrecrit
#16 Screenshot from 2017-11-14 12:36:52.png103.7 KBbrahmjeet789
#15 interdiff-2890086-12-15.txt2.05 KBrecrit
#15 paragraphs-validation-to-respect-form-display-2890086-15.patch9.15 KBrecrit
#13 interdiff-2890086-10-12.txt912 bytesrecrit
#13 paragraphs-validation-to-respect-form-display-2890086-12.patch9.1 KBrecrit
#10 interdiff-2890086-7-10.txt6.81 KBrecrit
#10 paragraphs-validation-to-respect-form-display-2890086-10.patch8.96 KBrecrit
#7 interdiff-2890086-4-7.txt1.63 KBrecrit
#7 paragraphs-validation-to-respect-form-display-2890086-7.patch7.18 KBrecrit
#5 paragraphs--with-patch-2890086-4--error-display-when-collapsing-20170628.png108.59 KBrecrit
#5 paragraphs--original-no-patches-289008--error-display-when-collapsing-20170628.png149.98 KBrecrit
#4 interdiff-2890086-2-4.txt1.59 KBrecrit
#4 paragraphs-validation-to-respect-form-display-2890086-4.patch5.56 KBrecrit
#2 paragraphs-validation-to-respect-form-display-2890086-2.patch5.57 KBrecrit

Issue fork paragraphs-2890086

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

recrit created an issue. See original summary.

recrit’s picture

Status: Active » Needs review
StatusFileSize
new5.57 KB

The attached patch implements fixes described in the report.

recrit’s picture

Title: Paragraphs widgets validate field not in the current form display » Paragraphs widgets validates fields that are not in the current form display
recrit’s picture

StatusFileSize
new5.56 KB
new1.59 KB

Removed $delta from #limit_validation_errors

recrit’s picture

Screenshots of error messages when attempting to collapse a paragraph that has a validation error:
1. Original without any patches: paragraphs--original-no-patches-289008--error-display-when-collapsing-20170628.png

2. With patch #4: paragraphs--with-patch-2890086-4--error-display-when-collapsing-20170628.png

johnchque’s picture

Status: Needs review » Needs work

Indeed, messages can be more clear.
Tests are not passing yet, we need tests passing. :)

recrit’s picture

@yongt9412 I suspect that the form display is not configured for the node reference in the failing test. First attempt at fixing is attached.

recrit’s picture

Status: Needs work » Needs review
johnchque’s picture

Status: Needs review » Needs work

Doesn't seem to be that, there are other tests that assert the error messages displayed, maybe better to check them.

recrit’s picture

StatusFileSize
new8.96 KB
new6.81 KB

The display used for the check was using the widget state which was not set yet. This updates the code to set the display after the paragraph entity is resolved.

recrit’s picture

Status: Needs work » Needs review
recrit’s picture

Status: Needs review » Needs work
recrit’s picture

StatusFileSize
new9.1 KB
new912 bytes

Fixing classic widget that does not have createMessage() method.

recrit’s picture

Status: Needs work » Needs review
recrit’s picture

StatusFileSize
new9.15 KB
new2.05 KB

Fixed the $field_name variable collision.

brahmjeet789’s picture

StatusFileSize
new103.7 KB

@recrit: i am unable to apply ur patch on paragraph ,please find attachment it and fix it

brahmjeet789’s picture

Status: Needs review » Needs work
recrit’s picture

Status: Needs work » Needs review
StatusFileSize
new8.61 KB

re-rolled for latest dev #a5919ee.

Status: Needs review » Needs work

The last submitted patch, 18: paragraphs-validation-to-respect-form-display-2890086-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

recrit’s picture

I believe this issue has been addressed by #2939718: Remove validation of closed paragraphs, fix validation and display of validation message for collapse which removes the validation of the closed forms, eliminating the code that was initially not using the form mode to validate.

recrit’s picture

^ I spoke too soon. The commit on #2939718: Remove validation of closed paragraphs, fix validation and display of validation message for collapse still results in the useless error of "This value should not be null.".

berdir’s picture

Priority: Normal » Major

Yes, in the end, we still validate.

We've also seen a somewhat related issue in our paragraphs_collection tests when you save collapsed paragraph with an inaccessible text format, that also results in a validation error now and didn't before. Before it uncollapsed and then saved without error.

I suspect that this is something that the text widget handles and then ignores those errors. Which means that this will for example also be a problem with with REST save, which has no concept of form display or only validating visible fields.

miro_dietiker’s picture

Polling again...

This was reported in a dupe, caused by Paragraphs type permission module, when permission was not given.

berdir’s picture

Issue tags: +Needs tests

As a first step, I've added a workaround for the text format validation in #3041808: Validation error when user saves paragraph using filter without its permission.

I assume that will get committed soon, then this needs a rebase and we'll need to get all tests to pass and also cover the changes with explicit tests.

miro_dietiker’s picture

Reminder: We need to be careful to check access for such an expansion and skip the validation for Paragraphs without access.

recrit’s picture

Status: Needs work » Needs review
StatusFileSize
new7.7 KB

rolled against the latest 8.x-1.x.

Status: Needs review » Needs work

The last submitted patch, 26: paragraphs-validation-to-respect-form-display-2890086-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

recrit’s picture

Status: Needs work » Needs review
StatusFileSize
new4.32 KB
new8.17 KB

For the ParagraphsWidget, validating in the formElement no longer worked. Moved this code to the massageFormValues() where the entity validate occurs.

Status: Needs review » Needs work

The last submitted patch, 28: paragraphs-validation-to-respect-form-display-2890086-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

recrit’s picture

Status: Needs work » Needs review
StatusFileSize
new2.26 KB
new8.16 KB

Patch updates to fix the classic widget validation.

Status: Needs review » Needs work

The last submitted patch, 30: paragraphs-validation-to-respect-form-display-2890086-30-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

siavash’s picture

#30 is working fine for me, thanks!

b_sharpe’s picture

Re-roll for latest dev/1.10

berdir’s picture

Status: Needs work » Needs review
berdir’s picture

Status: Needs review » Needs work

Still has one test fail.

b_sharpe’s picture

Just re-roll, still needs tests fixed.

miro_dietiker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: paragraphs-validation-to-respect-form-display-2890086-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

miro_dietiker’s picture

I see only a classic widget fail.

We could fix this in the new widget as a first step and then decide how critical backporting is.

ademarco’s picture

It seems that, when visiting the edit form node after $this->clickLink(t('Edit'));, the autocomplete input fields do not show anymore the deleted node label and ID.

ademarco’s picture

sardara’s picture

The classic widget remove paragraph button has a non empty #limit_validation_errors, compared to the experimental widget.

ademarco’s picture

Status: Needs work » Reviewed & tested by the community

Patch #42 works well for us.

chewie’s picture

I also could confirm. Patch #43 solves the problem.

berdir’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll. I'm unsure about the test coverage, not sure if it's just fixing incorrect configuration in our test or if it's actually testing the fix. The changed ParagraphsExperimentalAdministrationTest passes on HEAD too.

suresh prabhu parkala’s picture

Status: Needs work » Needs review
StatusFileSize
new9.37 KB

Re-rolled patch please review!

mbovan’s picture

StatusFileSize
new9.38 KB

Another reroll.

Status: Needs review » Needs work

The last submitted patch, 47: 2890086-47.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

capysara’s picture

I'm on Drupal 9.1.0 using the paragraphs 8.x-1.12 version. I updated to the dev version of paragraphs and added the patch in #47. Problem solved! I'm no longer getting errors on fields that are not in the form display.

My problem only occurred when I was using the Paragraphs Experimental widget with Edit mode: Closed. As a workaround (without the patch), changing Edit mode to Open allowed me to save without errors.

sergiur’s picture

Issue tags: +Needs reroll
sergiur’s picture

tried a quick reroll but there's a conflict in tests/src/Functional/WidgetLegacy/ParagraphsAdministrationTest.php and didn't have the time to figure it out

karishmaamin’s picture

StatusFileSize
new9.08 KB

Re-rolled patch #47

karishmaamin’s picture

Status: Needs work » Needs review
sergiur’s picture

Status: Needs review » Needs work

there's an elseif in the wrong place in ParagraphsWidget.php line 2337

sergiur’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new9.26 KB

found some time, so re-rolled patch at #47. Mostly auto-merged apart from WidgetLegacy/ParagraphsAdministrationTest.php which had a conflict due to the changes in drupalPostForm() must be changed for submitForm(). Also fixed the instances of drupalPostForm() added by this patch.

NormySan’s picture

The latest patch does not solve the issue on the latest 8.x-1.13 release, maybe it works on dev, have not tried.

recrit’s picture

Patch #55 applies cleanly to 8.x-1.14

cweiske’s picture

Patch #55 works on paragraphs 8.x-1.15.0 and Drupal 9.4.4.

The error message is gone:

Validation error on collapsed paragraph field_view: This value should not be null.

joevagyok’s picture

Status: Needs review » Reviewed & tested by the community

Patch #55 works well for us!

berdir’s picture

Status: Reviewed & tested by the community » Needs work

Doesn't apply anymore and needs to be converted to a merge request.

Julien Tekrane made their first commit to this issue’s fork.

aporie’s picture

StatusFileSize
new9.65 KB

Just in need of a fixed version of the current diff which applies on 1.18.

joevagyok’s picture

Status: Needs work » Needs review
StatusFileSize
new13.98 KB

Stabilized the MR and fixed a wrong use statement that went in there. Tests are passing now.
I attached the patch file version of the MR for composer patching and removed the patches from visibility to focus on the MR further on within this issue.

joevagyok’s picture

Reuploading the patch to use diff format instead of the gitlab patch format to avoid issues for those who want to use composer to patch the module.