Partially a follow-up of #1174640: Add new HTML5 FAPI element: number: We have a number element - there are more places where it should be used and legacy validation should be replaced. But there are also plain bugs (i.e. missing validation). Those are indicated with a !.

Issue / setting / variable No
validation
Tests
written
user_picture_file_size ! X
filter_url_length ! X
Image effects (dimensions, ...)
image_jpeg_quality
minimum_word_size (Search module) ! X
Image field settings (dimensions, file size, ...)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

Issue summary: View changes

Updating introduction.

Niklas Fiekas’s picture

Issue summary: View changes

Make a table.

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
3.32 KB

First failing tests for user_picture_file_size, filter_url_length and minimum_word_size.

Niklas Fiekas’s picture

Issue summary: View changes

image_jpeg_quality is properly validated.

Niklas Fiekas’s picture

Issue summary: View changes

New tests created.

Status: Needs review » Needs work

The last submitted patch, 1527988-number-validation-1-tests.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

That + attempts to add validation.

Status: Needs review » Needs work

The last submitted patch, 1527988-number-validation-3.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
13.57 KB

Convert the other occurences.

I am not sure why #3 is still failing, that is: The user configuration form doesn't save invalid input, but it says it does and it also doesn't show validation errors.

Status: Needs review » Needs work

The last submitted patch, 1527988-number-validation-5.patch, failed testing.

Niklas Fiekas’s picture

Ok, typo there.

This should do it.

aspilicious’s picture

'#description' => t('The minimum allowed image size expressed as WIDTHxHEIGHT (e.g. 640x480). Leave blank for no restriction. If a smaller image is uploaded, it will be rejected.'),

How can you leave this blank when the minimum value is 1...

Niklas Fiekas’s picture

Just leave it blank ;)

Seriously: The number will only be validated if it is not ''. So either valid or blank are allowed, unless #required is set.


/**
 * Form element validation handler for #type 'number'.
 *
 * Note that #required is validated by _form_validate() already.
 */
function form_validate_number(&$element, &$form_state) {
  $value = $element['#value'];
  if ($value === '') {
    return;
  }
  // ...
aspilicious’s picture

I wrote part of that function

#shame

cosmicdreams’s picture

Read through patch on #7 but I'm still confused: What's left to do after #7's patch

Niklas Fiekas’s picture

If everything that's there is OK, that's it.

ericduran’s picture

This is awesome. :)

Niklas Fiekas’s picture

bleen’s picture

Status: Needs review » Reviewed & tested by the community
  • patch applied
  • tests passed
  • manual checking of input filters, image field settings, image presets passed
  • code standards look good
  • cant find any other fields that should be made into number fields
  • removes crufty code ++++
  • adds tests ++++

RTBC

Niklas Fiekas’s picture

Thank you for the testing, bleen18.

Niklas Fiekas’s picture

Issue tags: +Needs backport to D7

Not to forget: The ones with the ! need to be backported to D7, though we cant use the shiny number element there.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great job. Nice clean-up. Committed to 8.x.

aspilicious’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs work

Back to needs work see #17

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
5.59 KB
3.26 KB

Fortunately only the simple cases were missing validation. That is only integers that must be positive.

The last submitted patch, 1527988-number-validation-20.patch, failed testing.

Niklas Fiekas’s picture

Next try.

Edit: OK. I believe this is ready.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

The backport looks complete to me. Can't think of a reason not to backport it.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, nicely spotted!

Committed and pushed to 7.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Factor out file field to a seperate issue.