Form validation messages appear in incorrect order

Patch works and still applies.

This does not completely solve the issue. The errors are displayed in the order form_set_error() is called. This is controlled by the hook invocation order and what order the validate hooks look at the fields. This patch does of course solve the issue for empty required fields and any modules which walk through the fields in form order.

I would like to see this sort of change be written for the development version and then backported. The code in Drupal 6 looks like it has the same behavior as 5.

If this is to be fixed at all for D7, I think this patch is the way to do it. As per #3, it's not a 100% solution, but it probably gets us most of the way there, and remaining edge cases would need a deeper FAPI refactoring (D8 material). But to be honest, I think even this patch should be bumped to D8. Leaving the version alone for now, so that others can make that call.

Well, why not. Just moves sorting from drupal_render() to form_builder(), so nothing lost.

With a proof that we're fixing a bug here, RTBC.

#4: validate_in_order-177124-4.patch queued for re-testing.

#4: validate_in_order-177124-4.patch queued for re-testing.

Using the patch in #4 I've created an updated patch for Drupal 7 using version 7.12.

Needs to be fixed in HEAD first.

Here's the updated patch for 8.x-dev based on comment #4

Here's the D7.x-dev patch, renamed as suggested in http://drupal.org/node/332678

Hmm. Interestingly, this patch actually messes up the weighting of field_groups, which always display right at the bottom of a node/add form, below the vertical tabs and form actions with this patch applied (D7).

Here is a slightly different version of the patch, which does the validation in order, but doesn't mess with the form so much aside from that.

Aah, that makes much more sense! :)

Now we should take over the inline comment from #13 to explain why we're enforcing elements to be ordered there.

Also, we still need a test.

There is still a problem with this. Once you submit a form, if it has validation errors, then when that form is rebuilt and rerendered, a lof of the fields show up under the submit/action buttons - similar to my post in #15 but now only after a validation failure.

I know I should just click the follow button but am so happy this issue is being worked on. Always bugged me that the error messages were out of order but never succeeded in fixing this. It's a big usabillity issue IMO so kuddos to all of you!

Cheers

I just tested this patch on a clean Drupal 8 and Drupal 7 installation, and it actually seems to work properly. The problem I'm experiencing only seems to happen when the field_group module is used, which makes me think the issue is with field_group and no with this patch. Setting back to needs review so that others can test and confirm.

I have confirmed that the problem with the fields reordering on submit, is indeed a problem with field_group. I have a patch that solves the field_group issue, a feature module to test with, and a screenshot that shows the problem at #1657306: Field group form elements are placed at the wrong location within the form array structure..

Works for me, but sun suggests we need a comment and a test. Marked #1843520: Field Validation Order is Wrong a duplicate of this.

I've just tried the patch, but there is a problem with it. Try this:

- Make a fresh install of Drupal 7.17 (or Drupal 7.x-dev).
- Apply the patch.
- Edit the "Basic Page" content type, go to the "Manage fields" tab and make the "Body" field required. Then, reorder the fields making the "Body" field the field with the lowest weight and the "Title" field the field with the highest weight.
- Then, try to create a new "Basic Page" node. Leave all the fields blank and click on the "Save" button.
- You'll see that the validation errors are shown in the wrong order. In addition, the fields on the node creation form are now rendered in the wrong order too!.

You're right! What a strange bug. This works OK if you call element_children() on a copy of $elements, but that's pretty inefficient. Still digging.

Yeah, it's really weird. I don't know the code very well, so I won't be able to help very much. I've looking for a chance to retrieve the fields from the database in the right order, but that's not easy, because the weight of each field is stored in the 'data' column, which is a BLOB column.

No no, definitely don't want to go to the DB, all the data needed is in $form/$elements around the place. I'm just poking around to understand how the field stuff moves "title" to the correct place, quite late in the game I think. I can't look at it now, will look again after dayjob work.

Interesting. I think this issue is related specifically to the Title field, because it's still not quite like the other ones - it gets a weight of -5 from the node_content_form() regardless of it's weight from the UI. I'm still trying to figure out where the weight from the field gets applied (not an expert).

OK, the reason this doesn't work quite right for "title" is that the weight of non-fields are only fixed up in _field_extra_fields_pre_render() called during drupal_render(). At the time we're validating the form, this hasn't been done yet.

Besides the fact that the weight isn't set yet, I *think* the reason that sorting $elements in _form_validate() breaks the field ordering is that $elements['#sorted'] is set to true as a side-effect, which means that when attempting to sort *after* pre_render has changed the weights, it thinks the $elements are already sorted, so doesn't re-sort.

So... a combination of issues:
- form needs to be pre_rendered to get the right weights?? (this might be bad)
- _field_extra_fields_pre_render() should unset #sorted if #weight is modified

That has a lot of sense. Nice job!.

I don't think we should call pre_render from validate - that's just too evil. So, we can't really fix this completely. We can get pretty close by sorting a *copy* of $elements (a copy so we don't modify $elements, which causes another bug later).

Hi again, lyricnz,

I've just tested the patch. Doing the same as in #23, the order of the field validation errors are still wrong, but the order of the fields after validation is the right one in the node create/edit form.

Going further with the test case explained in #23, adding another required field to the "Basic Page" content type and trying to create a new node of that content type leaving all the three (required) fields blank results in:

- The order of the fields after the validation is right in the node create/edit form.
- The order of the field validation errors shown is right except for the "Title" field, which is not really a regular field.

Yup, the patch in #30 should fix most of the ordering of field validation messages, and not break the form ordering. This probably still needs a test, but since the solution is incomplete, I'd like some buy-in from Real Core Coders before I spend the time on that. Plus a pointer on how to test it.

I manually applied the patch from #30 to Drupal 7's form.inc because I was having the same issue. After a few quick tests it seems to have fixed the problem, but I'd love to know if there are any possible side effects that arise from more thorough testing by those closer to this code. Thanks!

Marking #1922558: "compare two values" validation errors should appear in order as a duplicate of this.

I don't think the solution in #30 fixes all cases of this. For example, the Webform Validation module calls form_set_error() in an order that may not match the order of the form components and it does all of its validation at once, so its errors will be together, not sorted together with validation done by other parts of Drupal.

Right now, form_set_error() calls drupal_set_message() as part of its operation. What we need is for it to instead gather all the errors together. Then, once all errors have been set by all modules, sort the errors by the component to which they apply and then call drupal_set_message().

This may also play into the solution: #569094: Get rid of form_set_error().

Subscribing...

This should be looked at in light of #2131851: Form errors must be specific to a form and not a global.

Needs reroll and tests. This might not even be applicable to 8.x at this point.

Rerolled the patch.

Rerolled the patch

The last two patches look like they're reverting all of the changes to the form system. It's about 200x bigger than the last valid patch.

I tried rerolling #30 and the size of the new patch was just about the same as on #39 or #41. And looks like it is adding back all the parts of code which was removed.

I wonder if this issue is still valid. The patch on #30, changes a loop inside the function _form_validate(), which is no longer present in the file, or any file in the core.

Still needs tests, but here is the patch from #30 rerolled.

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -214,8 +214,12 @@ protected function finalizeValidation(&$form, FormStateInterface &$form_state, $
+    $elements_copy = $elements;

Call this $elements_sorted for readability and this looks good to go :)

Done.

This also needs tests, as mentioned in #45. My apologies for the confusion.

Here's the updated patch for 7.37 based on comment #16

Here's the updated patch for 7.37 based on comment #16

here's the patch for Drupal 7, 7.42. The only difference between the patch #30 and this is the line number.

As far as I understood, the left task for this D8 issue is only adding a test. I added a test which extends KernelTestBase to the patch #47. I expect that this test-only patch fails and another patch with working code and test works.

I'd like someone to review this. Thank you.

The test class name was wrong in the patch #55. I fixed it and try the test again. I'd like someone to review this.

OMG, the bug was reported in 2007, and still not fixed.
#52 worked for me (Drupal 7.50)

This is over nine years old so it would be good to make some progress with it…

- Able to reproduce original problem (8.2.x)
- Fixed by patch #59
- Have rerun testbot (8.2.x) and it passes
- No UI changes
- Seems in scope
- Not seeing any coding standard errors
- Code itself OK to me (it just sorts the element array by weight first before validating it)
- Code is commented
- Tests: I've not used Simpletest myself yet – test looks logical but is line 53 there for a reason or is it just orphaned?

$user_input = $form_state->getUserInput();

Agreed: what is the purpose of $user_input = $form_state->getUserInput()? Let's get that sorted and get this in!

Done.

Thanks @wturrell! In general, you should not mark your own patches RTBC, but in this case it is just a very small change that we approved in #66, so adding my +1 for the RTBC. Good writeup of the review in #63 also.

@cilefen and I went back and forth about whether to commit this issue to 8.4.x only or to backport it. We did come up with some obscure scenarios where fixing this bug and correcting the behavior might change something for some code somewhere (since the fix is in the form subsystem itself), so committing to 8.4.x. only for now on account of the alpha phase.

@Nitesh Sethia, I removed credit for the issue for you since your patches were unfortunately not correct and disrupted the flow of the issue. For your next core issue, I'd look carefully at https://www.drupal.org/contributor-tasks/reroll so we can include your work in the issue! Thanks for contributing.

Similarly for @preksha, your patch update did not take into account the recent comments on the issue. To receive issue credit in the future, you can build off the latest comments on the issue and provide an interdiff: https://www.drupal.org/documentation/git/interdiff

Saving issue credit.

I believe the Drupal 7 maintainers want us to mark issues PTBP until the separate D7 followup issue can be created and referenced on this issue. Once that's done, we mark this issue to fixed in 8.4.x. again. Thanks everyone!

Thank you, @Mikhail K. @wturrell @cilefen @xjm!

Following the comment #70, I opened a new issue for D7 backport.

#2850273: [D7] Form validation messages appear in incorrect order

Can we move this issue to Fixed?

This is a small point. The method name of the test case introduced in #55 is testLimitValidationErrors(). I used this name mistakenly and it's not proper for the content of the test. If possible, I'd like this to be adjusted.

Here is a trial patch. This patch expects that the previous patch was already applied.

I'm including a test case that should show that conditionally required fields are sometimes out of order. (Drupal 8.8)

Please let me know if this should be a separate issue. Thanks.

@Luke.Leber, thanks for the patch - definitely a new issue would be good for this.

I also think this issue should be marked 'Fixed', since it was committed to 8 and there is a separate task for D7, perhaps with a follow up for the small change in #72.

Created #3175401: Update testLimitValidationErrors() to testValidationErrorMessagesSortedWithWeight() as a follow up for #177124-72: Form validation messages appear in incorrect order so now marking this fixed.