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().
| Comment | File | Size | Author |
|---|
Issue fork paragraphs-2890086
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
Comment #2
recrit commentedThe attached patch implements fixes described in the report.
Comment #3
recrit commentedComment #4
recrit commentedRemoved $delta from #limit_validation_errors
Comment #5
recrit commentedScreenshots 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
Comment #6
johnchqueIndeed, messages can be more clear.
Tests are not passing yet, we need tests passing. :)
Comment #7
recrit commented@yongt9412 I suspect that the form display is not configured for the node reference in the failing test. First attempt at fixing is attached.
Comment #8
recrit commentedComment #9
johnchqueDoesn't seem to be that, there are other tests that assert the error messages displayed, maybe better to check them.
Comment #10
recrit commentedThe 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.
Comment #11
recrit commentedComment #12
recrit commentedComment #13
recrit commentedFixing classic widget that does not have createMessage() method.
Comment #14
recrit commentedComment #15
recrit commentedFixed the $field_name variable collision.
Comment #16
brahmjeet789 commented@recrit: i am unable to apply ur patch on paragraph ,please find attachment it and fix it
Comment #17
brahmjeet789 commentedComment #18
recrit commentedre-rolled for latest dev #a5919ee.
Comment #20
recrit commentedI 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.
Comment #21
recrit commented^ 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.".
Comment #22
berdirYes, 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.
Comment #23
miro_dietikerPolling again...
This was reported in a dupe, caused by Paragraphs type permission module, when permission was not given.
Comment #24
berdirAs 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.
Comment #25
miro_dietikerReminder: We need to be careful to check access for such an expansion and skip the validation for Paragraphs without access.
Comment #26
recrit commentedrolled against the latest 8.x-1.x.
Comment #28
recrit commentedFor the ParagraphsWidget, validating in the formElement no longer worked. Moved this code to the massageFormValues() where the entity validate occurs.
Comment #30
recrit commentedPatch updates to fix the classic widget validation.
Comment #32
siavash commented#30 is working fine for me, thanks!
Comment #33
b_sharpe commentedRe-roll for latest dev/1.10
Comment #34
berdirComment #35
berdirStill has one test fail.
Comment #36
b_sharpe commentedJust re-roll, still needs tests fixed.
Comment #37
miro_dietikerComment #39
miro_dietikerI 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.
Comment #40
ademarco commentedIt 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.
Comment #41
ademarco commentedComment #42
sardara commentedThe classic widget remove paragraph button has a non empty
#limit_validation_errors, compared to the experimental widget.Comment #43
ademarco commentedPatch #42 works well for us.
Comment #44
chewie commentedI also could confirm. Patch #43 solves the problem.
Comment #45
berdirNeeds 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.
Comment #46
suresh prabhu parkala commentedRe-rolled patch please review!
Comment #47
mbovan commentedAnother reroll.
Comment #49
capysara commentedI'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.
Comment #50
sergiurComment #51
sergiurtried a quick reroll but there's a conflict in
tests/src/Functional/WidgetLegacy/ParagraphsAdministrationTest.phpand didn't have the time to figure it outComment #52
karishmaamin commentedRe-rolled patch #47
Comment #53
karishmaamin commentedComment #54
sergiurthere's an elseif in the wrong place in ParagraphsWidget.php line 2337
Comment #55
sergiurfound some time, so re-rolled patch at #47. Mostly auto-merged apart from
WidgetLegacy/ParagraphsAdministrationTest.phpwhich had a conflict due to the changes in drupalPostForm() must be changed for submitForm(). Also fixed the instances of drupalPostForm() added by this patch.Comment #56
NormySan commentedThe latest patch does not solve the issue on the latest 8.x-1.13 release, maybe it works on dev, have not tried.
Comment #57
recrit commentedPatch #55 applies cleanly to 8.x-1.14
Comment #58
cweiske commentedPatch #55 works on paragraphs 8.x-1.15.0 and Drupal 9.4.4.
The error message is gone:
Comment #59
joevagyok commentedPatch #55 works well for us!
Comment #60
berdirDoesn't apply anymore and needs to be converted to a merge request.
Comment #63
aporieJust in need of a fixed version of the current diff which applies on 1.18.
Comment #64
joevagyok commentedStabilized 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.
Comment #65
joevagyok commentedReuploading 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.
Comment #66
joevagyok commented