Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff-2733987-48-52.txt | 690 bytes | ModernMantra |
#52 | validate_closed_paragraphs-2733987-52.patch | 4.38 KB | ModernMantra |
#48 | validate_closed_paragraphs-2733987-48.patch | 4.38 KB | ModernMantra |
#48 | Screenshot from 2016-12-12 13-21-50.png | 18.5 KB | ModernMantra |
#46 | interdiff-2733987-43-46.txt | 1.63 KB | ModernMantra |
Comments
Comment #2
pguillard CreditAttribution: pguillard commentedHere is what I did to reproduce :
@yongt9412, is this what you meant ?
Comment #3
pguillard CreditAttribution: pguillard commentedComment #4
johnchqueWe 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? :)
Comment #5
pguillard CreditAttribution: pguillard commentedThanks & Sorry for that. I changed the status because Open issues may stay without response.
Comment #6
miro_dietiker@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
Comment #7
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedSome progress... Created only tests to verify the 'problem'. Still needs some work on test before shifting to making a patch :).
Comment #8
miro_dietikerWhen 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.
Comment #9
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedSome 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' :)
Comment #10
miro_dietikerStill mr. Testbot, what's the status?
Comment #11
miro_dietikerHm #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. :-)
Comment #12
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedTest 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...
Comment #13
BerdirMy 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?
Comment #14
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedProviding the patch that offers validation of closed paragraph, changed tests by adding some assertions.
Comment #15
miro_dietikerHm, 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?
Comment #16
Berdirthat'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.
Comment #17
Berdir@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.
Comment #18
miro_dietikerI'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?
Comment #19
BerdirThat 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.
Comment #20
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedMade some improvements in validation and small fix in tests. Now patch should look better for review :-)
Comment #21
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedForgot to add test only patch. Here it is...
Comment #24
johnchqueLooks 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. :)
Comment #25
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedRefactored 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).
Comment #26
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedAfter some discussion with @berdir, after validation failed there will be no error message but rather opened paragraphs instead of closed.
Comment #29
miro_dietikerMissing space.
I don't see why this should change.
Why not based on ParagraphsTestBase? I guess it will simplify the test.
Comment #30
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedMade big refactoring considering previous comment...
Comment #33
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedFixing the test failures. On local machine seems ok, lets see test bot.
Comment #36
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedFailure 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 :-)
Comment #37
johnchqueI was writing a really big review when I realized that:
We can remove this test class.
Instead of adding more tests. Please do:
This has to be assertFieldByName
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.
Comment #38
johnchqueBtw 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.
Comment #39
johnchqueBtw just before this, set the reference field as required.
Comment #40
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedSo 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.
Comment #41
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedAs 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.
Comment #42
Berdira comma separated list of those messages is IMHO going to look pretty weird. But we can always improve that later.
Looks like we lost the assert on the validatin message here?
Comment #43
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedNow 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
Comment #44
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedAs discussed with @miro_dietiker, opening issue to change appearance of error message.
Comment #45
miro_dietikerLinking to 7.x issue about the same thing.
Comment #46
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedExtending little bit test coverage as suggested by @miro_dietiker.
Comment #48
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedRerolling the patch and some refactoring, now we are displaying 'validation warning message' in bit different way.
Comment #49
johnchque[].
Looks good, but when I add a new Paragraph without having the reference field as required the message is gone.
Comment #50
BerdirYes 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.
Comment #51
BerdirNeeds work for the array()
Comment #52
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedfixed array notation :-)
Comment #53
johnchqueRetested, still applies. Good!
Comment #55
miro_dietikerYeah, committing. More integrity! :-)
Created the follow-up to validate nested paragraphs for completeness.
Comment #56
johnchque#2838192: Validate nested paragraphs when collapsed in classic widget the issue about nested. :)
Comment #58
mogio_hh CreditAttribution: mogio_hh commentedWith 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.
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.
Comment #59
lukasss CreditAttribution: lukasss commentedI also can't verify $form_state->getValue() in the custom entity when paragraph is closed