Steps to reproduce:

  1. Create a minimal form with any #type of form element which is #required but has no #title.
  2. Submit the form without providing any value.

Expected result: because the #required form element has no #title, there should be no error message, but the element itself should get the error class (ie. the red border).

Actual result: There is no error message, and the form element does not even get the error class.

Patches with test will follow soon.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Boobaa’s picture

Boobaa’s picture

Status: Active » Needs review

Meh, forgot to NR.

chx’s picture

Form validation does not fail, which means a form with a #required element which has no #title nor value can be submitted.

Nope. Form validation fails and the submit handler does not run. This can easily be seen by adding an + $this->assertNoRaw("The form_test_validate_required_form_no_title form was submitted successfully.", 'Validation form submitted successfully.');
to the test only patch after the first drupalPost.

Boobaa’s picture

Updated patches according to @chx's instructions.

Boobaa’s picture

The bug is present in 8.x as well; here are the patches.

Status: Needs review » Needs work

The last submitted patch, core-1785436-5-required-no-title-test-and-fix-D8.patch, failed testing.

Boobaa’s picture

Status: Needs work » Needs review

That failing test seems to be totally unrelated for me. Does anybody have a clue about that?

Boobaa’s picture

Boobaa’s picture

chx’s picture

Status: Needs review » Needs work
+    $errors = $this->xpath('//input[contains(@class, "error")]');
+    $this->assertFieldByXPath('//input[contains(@class, "error")]', NULL, 'Error input form element class found.');

you do not use this $errors variable and also assertFieldByXPath() is the exact same as just xpath() if you call it with a NULL, but a little later in the code there's the call with FALSE -- so let's do that here as well. And let's get rid of $errors as it is not used.

Other than that, this is ready. Typically we would verify the nullity of a value with isset() but as it is not a variable the !== NULL is fine here.

Boobaa’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
4.07 KB
3.57 KB

Updated the patches according to #10, 7.x comes first.

Boobaa’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go IMO.

chx’s picture

Title: Submission of #required elements without #title and empty value is allowed » Submission of #required elements without #title and empty value does not display any error
catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Thanks. Committed/pushed to 8.x.

Looks like this needs a 7.x backport?

Boobaa’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

I do know it's not a good idea to mark my own issue as RTBC, but this time it's the proper way, I think: D7 patches are available in #11 already, and @chx marked them RTBC in #13. I have even checked now that the D7 patch can still be applied cleanly.

webchick’s picture

Assigned: Unassigned » David_Rothstein

Hm. This represents a behaviour change. I'd like to run this past David first. I could see it cause seemingly random issues to crop up in contrib/custom modules, perhaps a valid use case of which might be programmatically setting #required fields behind the scenes. Then again, it seems like it's probably just exposing what ought to be happening anyway. So... yeah. Would love to get David's thoughts before commit. :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.17 release notes

As far as I can see, this should be safe; it makes the HTML output correctly reflect the form validation that the form API already did.

Now, watch us be totally wrong and this turn out to be the patch that breaks all of Drupal :)

In the hopes that that's not the case, I committed this to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/01f18eb (But to be safe, I'm adding this to CHANGELOG.txt and to the release notes also.)

Actually, it seems pretty odd to me that we don't display some kind of error message in this case also (in addition to the red border around the form element) but I guess that's for some other issue to solve.

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
David_Rothstein’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

In theory this could be backported, although I'm not sure it's really worth it at this point.

MrHaroldA’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

I found a regression with views and required exposed filters caused by this patch. In #1877678: Exposed filter gets error class without submitting it comment #6 there's a simple view to reproduce this.

Changing the patched line

if (isset($element['#parents']) && form_get_error($element) !== NULL) {
  $element['#attributes']['class'][] = 'error';
}

to the Drupal 7.16 version (pre 01f18eb540c968d1bfae7def22de558bdc208c17)

if (isset($element['#parents']) && form_get_error($element)) {
  $element['#attributes']['class'][] = 'error';
}

doesn't add the error class on this unsubmitted and thus "errorless" form.

A var_dump() of form_get_error($element) results in string(0) "", and not NULL.

MrHaroldA’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Patch (to be ported)

Hmmm... Further investigation points to this piece of code in views/plugins/views_plugin_exposed_form.inc->render_exposed_form()

    $form_state = array(
      'view' => &$this->view,
      'display' => &$this->display,
      'method' => 'get',
      'rerender' => TRUE,
      'no_redirect' => TRUE,
      'always_process' => TRUE,
    );

...

    $form_state['exposed_form_plugin'] = $this;
    $form = drupal_build_form('views_exposed_form', $form_state);
    $output = drupal_render($form);

In this case 'always_process' => TRUE triggers form validation, which < Drupal 7.17 did throw an error, but without the error class. I now believe the patch from this issue reveiled the error in the views exposed filter plugin instead of causing it ...

Marking it as 6.x/patch again, sorry for the noise!

MrHaroldA’s picture

Issue summary: View changes

validation has no problems

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.