Problem/Motivation

We want the html5 number element in core.
See http://www.html5tutorial.info/html5-number.php to see how it looks like.

Proposed resolution

Create a form api number element.

Remaining tasks

- Create the number element: done

- Create validation handler: done
==> the number element requires a step validator, we used the method used in webkit converted to php.

- Create tests: done

User interface changes

People will be able to create html5 form api elements

API changes

No real api changes. Only additions.

Original report by davereid

Split from #675348: META: Support HTML5 form input elements to add a new 'numberfield' Form API element.

Questions:
1. Do we add validation?
2. This field would be *widely* used in core.

CommentFileSizeAuthor
#152 1174640-forgot-placeholder-testcoverage.patch788 bytesNiklas Fiekas
#145 1174640-html5-number-144.patch19.58 KBNiklas Fiekas
#145 1174640-html5-number-144-interdiff.txt777 bytesNiklas Fiekas
#144 1463656-drupal-kernel-54.patch29.64 KBNiklas Fiekas
#144 1463656-drupal-kernel-54-interdiff.txt4.96 KBNiklas Fiekas
#138 1174640-html5-number-138.patch19.7 KBNiklas Fiekas
#132 1174640-html5-number-132.patch19.62 KBNiklas Fiekas
#132 1174640-interdiff-132.txt1.85 KBNiklas Fiekas
#131 1174640-html5-number-131.patch19.59 KBNiklas Fiekas
#131 1174640-interdiff-131.txt3.44 KBNiklas Fiekas
#127 1174640-html5-number-127.patch19.81 KBNiklas Fiekas
#127 1174640-interdiff-127.patch3.66 KBNiklas Fiekas
#115 1174640-html5-number-115.patch18.91 KBNiklas Fiekas
#115 1174640-interdiff-115.txt533 bytesNiklas Fiekas
#113 1174640-html5-number-113.patch18.9 KBNiklas Fiekas
#113 1174640-interdiff-113.txt1.64 KBNiklas Fiekas
#112 1174640-html5-number-112.patch17.41 KBNiklas Fiekas
#112 1174640-interdiff-112.txt9.9 KBNiklas Fiekas
#107 1174640-html5-number-107.patch18.04 KBNiklas Fiekas
#107 1174640-interdiff-107.txt2.19 KBNiklas Fiekas
#106 1174640-html5-number-106.patch19.92 KBNiklas Fiekas
#106 1174640-interdiff-106.txt457 bytesNiklas Fiekas
#103 1174640-html5-number-103.patch18.01 KBNiklas Fiekas
#103 1174640-interdiff-103.txt812 bytesNiklas Fiekas
#101 1174640-html5-number-101.patch18.02 KBNiklas Fiekas
#101 1174640-interdiff-101.txt567 bytesNiklas Fiekas
#98 1174640-html5-number-98.patch18 KBNiklas Fiekas
#98 1174640-interdiff-98.txt533 bytesNiklas Fiekas
#97 1174628-html5-search-97.patch18.28 KBNiklas Fiekas
#97 1174640-interdiff-97.txt1.92 KBNiklas Fiekas
#96 1174640-html5-number-96.patch18.59 KBNiklas Fiekas
#96 1174640-interdiff-96.txt7.5 KBNiklas Fiekas
#93 1174640-html5-number-93.cpp_.txt843 bytesNiklas Fiekas
#92 1174640-html5-number-92.patch18.45 KBNiklas Fiekas
#92 1174640-interdiff-92.txt6.42 KBNiklas Fiekas
#87 FAPI-number-1174640-87.patch16.23 KBcosmicdreams
#85 FAPI-number-1174640-84.patch16.22 KBcosmicdreams
#83 FAPI-number-1174640-83.patch16.23 KBcosmicdreams
#80 FAPI-number-1174640-80.patch935.96 KBcosmicdreams
#77 1174640-html5-number-77.patch16.09 KBericduran
#72 1174640-72.patch16.08 KBericduran
#71 1174640-71.patch16.1 KBericduran
#68 1174640-68.patch14.89 KBericduran
#62 1174640-number-element-62.patch16.77 KBaspilicious
#61 1174640-number-element-61.patch13.64 KBaspilicious
#60 internetExplorerErrors.png45 KBaspilicious
#59 1174640-number-element-59.patch13.7 KBaspilicious
#58 drupal-1174640-58.patch13.74 KBtim.plunkett
#50 1174640-number-element-50.patch13.59 KBaspilicious
#49 1174640-number-element-49.patch13.59 KBaspilicious
#43 1174640-number-element-42.patch12.89 KBaspilicious
#41 1174640-number-element-40.patch12.9 KBaspilicious
#39 1174640-number-element.patch12.9 KBaspilicious
#31 117464-html5-number-element-31.patch9.58 KBamateescu
#28 117464-html5-number-element-27.patch9.01 KBaspilicious
#24 117464-html5-number-element-24.patch9.58 KBaspilicious
#18 117464-html5-number-element_2.patch7.5 KBericduran
#11 117464-html5-number-element.patch7.01 KBericduran
#7 117464-html5-number-element.patch7.28 KBericduran
#6 117464-html5-number-element.patch5.88 KBericduran
#5 117464-html5-number-element.patch2.43 KBericduran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Thoughts:
1. Yes. We should support a #min and #max property that is used for automatic validation. We can get rid of the functions in #1174444: Make the _element_validate_* functions in field.module available for all contrib modules to use.
2. Likely we'll want a follow-up issue to convert core usage.

ericduran’s picture

We'll also want to add the #step attribute.

ericduran’s picture

Assigned: Unassigned » ericduran
Dave Reid’s picture

Another concern: we could probably replace core's weight select fields with this new number element.

ericduran’s picture

Status: Active » Needs review
FileSize
2.43 KB

1st patch,

still need to change all the form field that are numbers to use the new element.

But uploading for simpler test-bot testing. ;)

ericduran’s picture

New Patch
- Change field api number module from Text Field to Number Field.
- Change drupal weight select field from textfield to number field.

Is there anywhere else where the number field should be swap? There's not that many used of number in core that I can find (I didn't really spend that much time looking)

ericduran’s picture

- Removing _element_validate_number
- adding min, max support.

Status: Needs review » Needs work

The last submitted patch, 117464-html5-number-element.patch, failed testing.

ericduran’s picture

ugg, apparently the number field doesn't accept decimals.

ericduran’s picture

Ok, I need to add the step, attribute is if a decimal.

ericduran’s picture

ericduran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -html5

The last submitted patch, 117464-html5-number-element.patch, failed testing.

ericduran’s picture

Status: Needs work » Needs review
Issue tags: +html5

#11: 117464-html5-number-element.patch queued for re-testing.

voxpelli’s picture

Status: Needs review » Needs work

Some more feedback.

+++ b/includes/form.inc
@@ -3655,6 +3655,40 @@ function theme_textfield($variables) {
+ * @param $variables
+ *   An associative array containing:
+ *   - element: An associative array containing the properties of the element.
+ *     Properties used: #title, #value, #description, #size, #maxlength,
+ *     #required, #attributes, #step.

Doesn't include #min, #max (and #placeholder) - also not #id and #name?

+++ b/includes/form.inc
@@ -3655,6 +3655,40 @@ function theme_textfield($variables) {
+  element_set_attributes($element, array('id', 'name', 'value', 'size', 'maxlength', 'placeholder', 'step', 'min', 'max'));

According to Mozilla the size, maxlength, placeholder attributes are ignored for the number-type: https://developer.mozilla.org/en/HTML/element/input#attr-maxlength

+++ b/includes/form.inc
@@ -3655,6 +3655,40 @@ function theme_textfield($variables) {
+  _form_set_class($element, array('form-text', 'form-number'));

Same feedback regarding the form-text class here as on #1174646: Add new HTML5 FAPI element: range

+++ b/includes/form.inc
@@ -3655,6 +3655,40 @@ function theme_textfield($variables) {
+function form_validate_number(&$element, &$form_state) {

Should check #min and #max here as well.

Would also be nice if form_set_value() or something was used to save a typecasted (floatval()?) representation of the number so that it has the correct data type when used by the rest of the code.

Powered by Dreditor.

yareckon’s picture

sub.

ericduran’s picture

@voxpelli:
- Yea, the docs are off, patch should be re-rolled.
- We're using form-text on purposed see http://drupal.org/node/1174628#comment-4544246
- I left the maxlength for the same reason the form-text is there, but I guess we can remove it.
- Yea, makes sense to also validate #min, #max.
- Not sure about the floatval, but it can't hurt so yeah, make sense.

ericduran’s picture

Rerolled,
- I fix the docs, but I didn't add the id property, does it belong here? I don't see it anywhere.
- Removed the maxlenght.
- Added validation for #min, and #max
- Didn't change the val to floatval, I think we should wait on this change.

ericduran’s picture

Status: Needs work » Needs review
Dave Reid’s picture

Status: Needs review » Needs work

I don't believe we can rely on the #title attribute always being present when calling form_error(). See _form_validate() and its calls to form_error().

voxpelli’s picture

+++ b/includes/form.inc
@@ -3655,6 +3655,49 @@ function theme_textfield($variables) {
+  element_set_attributes($element, array('id', 'name', 'value', 'size', 'placeholder', 'step', 'min', 'max'));

Why should "placeholder" be included when it's not usable by this input-type?

+++ b/includes/form.inc
@@ -3655,6 +3655,49 @@ function theme_textfield($variables) {
+  if ($value != '' && !is_numeric($value)) {
+    form_error($element, t('%name must be a number.', array('%name' => $element['#title'])));
+  }
+  // Validate the #min property.
+  if ($value != '' && isset($element['#min']) && $value < $element['#min']) {
+    form_error($element, t('%name must be above %min.', array('%name' => $element['#title'], '%min' => $element['#min'])));
+  }
+  // Validate the #max property.
+  if ($value != '' && isset($element['#max']) && $value > $element['#max']) {
+    form_error($element, t('%name must be below %max.', array('%name' => $element['#title'], '%max' => $element['#max'])));
+  }

Either wrap these checks within one "if ($value != '')" or add a check in the beginning of the validate-function that returns early if $value is empty. No need to check the same thing three times.

Powered by Dreditor.

ericduran’s picture

Also need to add validation for the step attribute. See http://www.w3.org/TR/html5/association-of-controls-and-forms.html#suffer...

aspilicious’s picture

Ignore this

aspilicious’s picture

Here is a new try. It does not include some stuff davereid and voxpelli noticed. I made some smaller functions because we also need them for the range validation. I prefer many small functions above one bigger. But feel free to merge some of them if you think its needed. In fact is range validation the same as number validation.

I didn't test this patch so chances are it doesn't work.

ps: Should we test if min < max and stuff? I don't tink so.

EDIT:
Srry for the tabs and trailing white spaces. I won't reroll because it needs some reviews first.

ericduran’s picture

+++ b/includes/form.incundefined
@@ -3653,6 +3654,132 @@
+
+/**
+ * Helper form element validator: number.
+ */
+function _element_validate_number($element, &$form_state) {
+  if (!is_numeric($element['#value'])) {
+    form_error($element, t('%name must be a number.', array('%name' => $element['#title'])));
+  }
+}

we can just leave this in the form_validate_number

+++ b/includes/form.incundefined
@@ -3653,6 +3654,132 @@
+/**
+ * Helper form element validator: min.
+ */
+function _element_validate_min($element, &$form_state) {
+  if(isset($element['#min'] && $element['#value'] < $element['#min']) {
+      form_error($element, t('%name must be below or equal to %min.', array('%name' => $element['#title'], '%min' => $element['#min'])));
+  }
+}

We can't rely on #title.

+++ b/includes/form.incundefined
@@ -3653,6 +3654,132 @@
+/**
+ * Helper form element validator: step.
+ */
+function _element_validate_step($element, &$form_state) {
+  if (isset($element['#step'])) {
+    if (isset($element['#min']) && _element_validate_step_mismatch($element['#value'], $element['#step'], $element['#min'])
+       // Message that this is not a valid number
+    }
+    elseif (_element_validate_step_mismatch($element['#value'], $element['#step']) {
+       // Message that this is not a valid number
+    }
+  }

This is a helper function for a helper function. We can just do this is number_validate

+++ b/includes/form.incundefined
@@ -3653,6 +3654,132 @@
+ */
+function _element_validate_step_mismatch($value, $step, $min = 0.0) {
+  $doubleValue = (double)$value;
+  $doubleValue = abs($doubleValue - $min);
+  $digits = 16; // DBL_MANT_DIG
+
+  // double's fractional part size is DBL_MANT_DIG-bit. If the current value
+  // is greater than step*2^DBL_MANT_DIG, the following computation for
+  // remainder makes no sense.
+  if ($doubleValue / pow(2.0, $digits) > $step) {
+    return false;      ¶
+  }
+  // The computation follows HTML5 4.10.7.2.10 `The step attribute' :
+  // ... that number subtracted from the step base is not an integral multiple
+  // of the allowed value step, the element is suffering from a step mismatch.
+  (double) $remainder = abs($doubleValue - $step * round($doubleValue / $step));
+  // Accepts erros in lower fractional part which IEEE 754 single-precision
+  // can't represent.
+  $computedAcceptableError = (double)($step / pow(2.0, $digits));
+
+  return $computedAcceptableError < $remainder && $remainder < ($step - $computedAcceptableError);

Lets not add this here, keep this on the other issue, because is going to loose all the context and the long conversation and then they'll be two threads about the same exact thing. We'll just make this patch a dependent on the range patch.

Powered by Dreditor.

ericduran’s picture

edit: double post.

aspilicious’s picture

Ok this one is better. I'm ok with postponing but I used this issue because I'm just going to use the number validate function for range. As it is exactly the same. The only difference between number and range is the slider.

http://diveintohtml5.org/forms.html#type-range

aspilicious’s picture

Forgot patch

ericduran’s picture

Status: Needs work » Needs review

Let the test bot review for us 1st ;)

Status: Needs review » Needs work

The last submitted patch, 117464-html5-number-element-27.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.58 KB

Fixed those form.inc errors.

ericduran’s picture

Status: Needs review » Needs work
+++ b/includes/form.incundefined
@@ -3655,6 +3656,106 @@ function theme_textfield($variables) {
+ */
+function form_validate_number(&$element, &$form_state) {
...
+  // Validate the #number property.
+  if (!is_numeric($value)) {
+    form_error($element, t('%name must be a number.', array('%name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'])));

+++ b/modules/field/modules/number/number.moduleundefined
@@ -94,18 +94,16 @@ function number_field_instance_settings_form($field, $instance) {
-    '#element_validate' => array('_element_validate_number'),

There is no #number property.

+++ b/modules/field/modules/number/number.moduleundefined
@@ -94,18 +94,16 @@ function number_field_instance_settings_form($field, $instance) {
-    '#type' => 'textfield',

We shouldn't do this. If anything we should add a new fieldapi widget call number field but that can be a separate patch. Will leave the field api for another set of patches. These are just the actual element field.

Powered by Dreditor.

aspilicious’s picture

Please review the range patch first because the functions overlap

yched’s picture

Subscribe

attiks’s picture

ericduran’s picture

Issue tags: +HTML5 Sprint: August 2011 - 2
Jacine’s picture

Issue tags: -HTML5 Sprint: August 2011 - 2

Removing the sprint tag. Also, can we get an issue summary update here? Thanks!

ericduran’s picture

Hmm, this issue is actually pretty close, we probably just need some small changes and a re-roll.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
12.9 KB

I think this is done.

The values of $digits and $mantissa are calculated using the information on the php floating point doc page.
(http://php.net/manual/en/language.types.float.php)

$mantissa_length = 30; // This is max workable length for 32bit representation of mantissa. We not use other (52-$mantissa_length bits)

And

Floating point numbers have limited precision. Although it depends on the system, PHP typically uses the IEEE 754 double precision format, which will give a maximum relative error due to rounding in the order of 1.11e-16. Non elementary arithmetic operations may give larger errors, and, of course, error progragation must be considered when several operations are compounded.

All tests are passing, I think we handled every edge case.

aspilicious’s picture

Small doc fix. Some comments exceeded 80 chars.

aspilicious’s picture

Forgot to upload

Status: Needs review » Needs work

The last submitted patch, 1174640-number-element-40.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
12.89 KB

No idea why that failed lets try again

tim.plunkett’s picture

Just some minor nitpicks, leaving cnr for someone to ACTUALLY review it.

+++ b/core/includes/form.incundefined
@@ -3731,6 +3731,113 @@
+  if(isset($element['#min']) && $value < $element['#min']) {

missing space

+++ b/core/includes/form.incundefined
@@ -3731,6 +3731,113 @@
+  if(isset($element['#max']) && $value > $element['#max']) {

missing space

+++ b/core/includes/form.incundefined
@@ -3731,6 +3731,113 @@
+  elseif(_element_validate_step_mismatch($value, $element['#step'])) {

missing space

+++ b/core/includes/form.incundefined
@@ -3731,6 +3731,113 @@
+  $doubleValue = (double)$value;

no camelCase, missing space

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -216,6 +216,79 @@
+    $errorMessages = array(

more camelcase

14 days to next Drupal core point release.

aspilicious’s picture

Ok fixed everything from #44. Waiting for more reviews before I reroll :)

ericduran’s picture

this looks good to me code wise.

Do we maybe want to rename _element_validate_step_mismatch to _number_element_validate_step_mismatch or _element_number... which makes a little more sense.

Thoughts?

I'll wait for the re-rolled before changing status.

ericduran’s picture

Hmm, I guess it doesn't have to be number specific since we're also going to use it for the range element.

richthegeek’s picture

+1 on #46.

It might be nice to have an is_numeric check on the #min, #step, #max attributes in theme_number() so that the validation checks do not fail on-submit (it'd be a weird error that'd take a while to track down, but it would be rare to occur).

Comment's have a few wording/spelling issues, suggestions as follows:

// Validate the #number property.
// Ensure the input is numeric.
// Validate the #min property.
// Ensure that the input is greater than the #min property, if set.
// Validate the #max property.
// Ensure that the input is less than the #max property, if set.
// Validate the #step property.
// Check that the input is an allowed multiple of #step (offset by #min if #min is set).
// Accepts erros in lower
// Accepts errors in lower

Other than that, all forseeable issues have been covered and the code is clean and conforms to standards.

aspilicious’s picture

Hmm lets try this one

aspilicious’s picture

or this one

ericduran’s picture

I'm not really sure about the is_numeric check

As far as I know drupal doesn't support broken code. So if someone puts text in there and it breaks that's better than the error.

Maybe someone more familiar with these kind issues could comment.

tim.plunkett’s picture

I think that's right, in #1283892: Let Render API fail in a tale-telling way on invalid $element we went back and added trigger_error('foo', E_USER_ERROR); when someone uses the Render API wrong.

This patch is using E_USER_WARNING, you can begin the bikeshed on that :)

ericduran’s picture

Thanks. Not bikesheding just an observation and a simple question.

We'll see what the test said.

Status: Needs review » Needs work

The last submitted patch, 1174640-number-element-50.patch, failed testing.

tim.plunkett’s picture

Sorry I meant bikeshed over the error level, not anything you said.

There is a line in the patch in the other issue about disabling simpletest from catching the errors which this probably needs.
I'd find it myself but I'm on my phone

ericduran’s picture

@tim.plunkett ha my reply came of rude. Sorry about that. :-p

ericduran’s picture

I think this is the line @tim.plunkett was talking about define('SIMPLETEST_COLLECT_ERRORS', FALSE);

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
13.74 KB

Just trying this out.
Hopefully this appeases ericduran's fury ;)

aspilicious’s picture

I read the other issue and now I'm sure we need thsi extra validation. And I'm going to change the level warning to "E_USER_ERROR". Just to have a consistent pattern in core.

aspilicious’s picture

And here is a screenshot with all the elements.
http://www.diigo.com/item/image/1fbht/vqg0?size=o

Tests:
On chrome ==> awesome form/browser validation :D
On IE ==> awesome server side validation

(attached a screenshot of IE)

aspilicious’s picture

Ok I removed define('SIMPLETEST_COLLECT_ERRORS', FALSE); because the tests don't contain any problems that would trigger one of these notices.

The tests only check for correct validation.

aspilicious’s picture

Added some tests for the errors. But they fail, although I can see the correct messages on my screen. (assertraw accepts substring I hope...)

I also added tests for placeholder and disabled and the disabled tests are failing. Again the elements are disabled so something else must be going wrong. Not able to fix it... :(

Status: Needs review » Needs work

The last submitted patch, 1174640-number-element-62.patch, failed testing.

Jacine’s picture

Issue tags: +sprint

Yay! Awesome to see activity here. Tagging this for the current sprint. :D

Can anyone help @aspilicious with the test failures?

ericduran’s picture

Issue tags: -sprint

Fixing text now. Patch soon to come.

ericduran’s picture

Issue tags: +sprint

x-post

Dave Reid’s picture

Hrm, I disagree strongly with the trigger_error() calls in theme_number(). We don't support coders being dumb and misusing the API. Any place where this can be variable input by the user it should be validated prior (e.g. min/max/step fields for a number widget formatter would be validated) to being used as the FAPI properties.

ericduran’s picture

FileSize
14.89 KB

This patch should pass the test.

Changes:
- Remove the trigger_error. As mention by Dave Read this pattern is really not used in core.

For anyone interested in why the old test were failing incase the trigger_error gets added back. One was because simpletest needs a small change to support new elemenets. Lol so lets remember that for the next patch lol. Also the trigger error was failing because the assertRaw was actually looking for the wrong text because the way the error is printed. Anyways not really needed now since that part got removed.

This patch should be good.

ericduran’s picture

Status: Needs work » Needs review

for the test bot.

Dave Reid’s picture

I think this also needs the default CSS changes like were made in the tel patch, otherwise the default rendering of this field in non-HTML5 supported browsers looks bad in the Seven theme.

ericduran’s picture

FileSize
16.1 KB

And here it is with the css.

ericduran’s picture

FileSize
16.08 KB

Actually that last patch had both form-text and form-number added to the element. And on the preview tel element we decided to only use the form-{element-type}

So this patch removed form-text from the number field, which makes the css actually needed now.

Sorry for all the noise.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1023,11 +1029,120 @@ function form_test_select_submit($form, &$form_state) {
+  define('SIMPLETEST_COLLECT_ERRORS', FALSE);

I don't think we need this line than.

Otherwise this looks kinda rtbc

-24 days to next Drupal core point release.

aspilicious’s picture

Status: Needs work » Needs review
Niklas Fiekas’s picture

Status: Needs review » Needs work

A few issues and/or remarks.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+  //  Ensure the input is numeric.

Superflous space.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+  // Check that the input is an allowed multiple of #step (offset by #min if
+  // #min is set).
+  if (isset($element['#min'])) {
+    if (_element_number_validate_step_mismatch($value, $element['#step'], $element['#min'])) {
+      form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
+    }
+  }
+  elseif (_element_number_validate_step_mismatch($value, $element['#step'])) {
+     form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));

Repeating the same error message here. Maybe there is a nicer way.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+ * @return
+ *   TRUE if the input value is invalid
+ *   FALSE if the input value is valid

Trailing dots missing. Also, I usually expect validation functions to work the other way around.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+  // Double's fractional part size is $mantissa-bit. If the current value
+  // is greater than step*2^$mantissa, the following computation for

Do we have line breaks at 80 characters or can we have them earlier in favor of having "is greater" together?

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+  // The computation follows HTML5 4.10.7.2.10 `The step attribute' :

Wierd quotes here.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -219,6 +219,79 @@ class FormsTestCase extends DrupalWebTestCase {
+        // Create placeholder array

No trailing dot.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -261,7 +334,7 @@ class FormsTestCase extends DrupalWebTestCase {
+    $this->assertEqual(count($disabled_elements), 34, t('The correct elements have the disabled property in the HTML code.'));

Not sure if we should touch it: Don't translate assertion messages.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -119,6 +119,12 @@ function form_test_menu() {
+    'title' => t('Number'),

Don't translate the title.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1023,11 +1029,120 @@ function form_test_select_submit($form, &$form_state) {
+  define('SIMPLETEST_COLLECT_ERRORS', FALSE);

Yep. Probably remove this.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1023,11 +1029,120 @@ function form_test_select_submit($form, &$form_state) {
+  $form['submit'] = array('#type' => 'submit', '#value' => 'Submit');

Not sure if this should be one line or more.

Dave Reid’s picture

Few thoughts

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,115 @@ function theme_tel($variables) {
+  element_set_attributes($element, array('id', 'name', 'value', 'step', 'min', 'max', 'maxlength', 'placeholder'));

We should probably still support #size for browsers that fallback to default text behavior?

+++ b/core/modules/system/system.moduleundefined
@@ -374,6 +374,16 @@ function system_element_info() {
+    '#size' => 60,

Default size might be a little big? Maybe use 30 as a default?

22 days to next Drupal core point release.

ericduran’s picture

Status: Needs work » Needs review
FileSize
16.09 KB

Update, fix all the comments mention above.

Status: Needs review » Needs work

The last submitted patch, 1174640-html5-number-77.patch, failed testing.

Niklas Fiekas’s picture

Ok, thanks. Next round.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,112 @@ function theme_tel($variables) {
+  if(!$value)
+    return;

Since 0 might be a legitimate number, we probably need to check $value === '' here. Also if_( and always use { }.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,112 @@ function theme_tel($variables) {
+      form_error($element, t('%name must be higher or equal to %min.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'], '%min' => $element['#min'])));

Too much leading space, here.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,112 @@ function theme_tel($variables) {
+      form_error($element, t('%name must be below or equal to %max.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'], '%max' => $element['#max'])));

And here.

+++ b/core/includes/form.incundefined
@@ -3758,6 +3758,112 @@ function theme_tel($variables) {
+ * @return
+ *   TRUE if the input value is invalid.
+ *   FALSE if the input value is valid.
+ */
+function _element_number_validate_step_mismatch($value, $step, $min = 0.0) {

Just in case you didn't deliberately skip this point: TRUE if invalid on a function that is called validate is a bit wierd, to me.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -219,6 +219,79 @@ class FormsTestCase extends DrupalWebTestCase {
+          $this->assertText(t($errorMessages[$id], $placeholders));
+        }
+        else {
+          $this->assertNoText(t($errorMessages[$id], $placeholders));

Usually t() on a $variable isn't ok, but yeah ... it's a testcase and the error message is defined in another place already.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -261,7 +334,7 @@ class FormsTestCase extends DrupalWebTestCase {
-    $this->assertEqual(count($disabled_elements), 33, t('The correct elements have the disabled property in the HTML code.'));
+    $this->assertEqual(count($disabled_elements), 34, t('The correct elements have the disabled property in the HTML code.'));

If we touch the line, remove the t().

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1011,7 +1017,11 @@ function form_test_select($form, &$form_state) {
+    '#value' => 'Submit'

Add a trailing ,?

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1023,11 +1033,119 @@ function form_test_select_submit($form, &$form_state) {
+/*
+ * Builds a form to test #type 'number' validation.
+ */
+function form_test_number($form, &$form_state) {
+
+  $base = array(

Superflous empty line.

See http://drupal.org/node/1354#forms for doxygen on form constructors.

cosmicdreams’s picture

FileSize
935.96 KB

Fixed most of the formatting issues called out in 79.

Niklas Fiekas’s picture

Status: Needs work » Needs review

Thank you. Something seams to be wrong with the patch, however. Maybe the testbot can handle it anway ... so let's try "needs review".

Edit: Mhh ... were you creating the patch with Git?

The last submitted patch, FAPI-number-1174640-80.patch, failed testing.

cosmicdreams’s picture

Status: Needs review » Needs work
FileSize
16.23 KB

dang, I had relied on my IDE (phpStorm) to create the patch. I guess that's not going to be viable path. I'll create a better patch.

Niklas Fiekas’s picture

Status: Needs work » Needs review

Much better. Thanks.

+++ b/core/includes/form.incundefined
@@ -3785,6 +3785,113 @@ function theme_tel($variables) {
+/*

Doxygen is supposed to start with /**.

cosmicdreams’s picture

FileSize
16.22 KB

Fixed that comment with this patch:

aspilicious’s picture

+++ b/core/includes/form.incundefined
@@ -3786,6 +3786,113 @@ function theme_tel($variables) {
+ * @return
+ *   TRUE if the input value is invalid.
+ *   FALSE if the input value is valid.
+ */
+function _element_number_validate_step_mismatch($value, $step, $min = 0.0) {

The function returns TRUE is a step mismatch has occured. If the @return statement is confusing we can possible change it to "TRUE if a step mismatch has occured. False if no step mismatch has occured."

Or something better...

If that's cleared out this is rdy to go!

cosmicdreams’s picture

FileSize
16.23 KB

Here's a patch with that small change.

Niklas Fiekas’s picture

Issue tags: -html5, -sprint

#87: FAPI-number-1174640-87.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5, +sprint

The last submitted patch, FAPI-number-1174640-87.patch, failed testing.

Dave Reid’s picture

Validation functions should typically return TRUE if the conditions pass and FALSE if the value is invalid which seems contrary to the logic of _element_number_validate_step_mismatch() which is why I'd rather it be named valid_number_step() or similar (since it also has nothing to do with having form element arrays as parameters).

Dave Reid’s picture

Also can we merge or get rid of element_validate_number() now that we have form_validate_number()?

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
18.45 KB

Rebased + #90 + unit tests.

Remaining problem: The acceptable error seams to be too high, i.e:
valid_number_step(10.3, 10.3, 0.0001) says valid, but it should say step mismatch.

I tried it with the C++ version and it works as expected. $digits and $mantissa are higher there, though. Using the same values in PHP worked for me, but it probably won't work on all systems.

Niklas Fiekas’s picture

For completeness (and before I do git clean -f again): The taken-from-webkit-slightly-refactored C++ code I used for testing and the $digits and $mantissa values that worked for me there attached.

Output:

DBL_MANT_DIG = 53 != $mantissa = 30
FLT_MANT_DIG = 24 != $digits = 16
valid_number_step(10.3, 10.3, 0): 1
valid_number_step(10.3, 10.3, 0.0001): 0

Status: Needs review » Needs work

The last submitted patch, 1174640-html5-number-92.patch, failed testing.

Niklas Fiekas’s picture

Ok. Forgot to use the new function name. Other findings:

(1) Is there a way to accept arbitrary floating point numbers? What is the browser behaviour when there is no step attribute?
Apparently not.

(2)

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1029,11 +1039,118 @@ function form_test_select_submit($form, &$form_state) {
+  $form['float_step_no_error'] = $base + array(
+    '#title' => 'Float test hard',
+    '#default_value' => 1.2,
+    '#step' => 0.3,

Note that were using 'Float test hard' as the title here.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1029,11 +1039,118 @@ function form_test_select_submit($form, &$form_state) {
+  $form['float_step_hard_no_error'] = $base + array(
+    '#title' => 'Float test hard',
+    '#default_value' => 0.9411764712,
+    '#step' => 0.00392156863712,

And the same #title here.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
7.5 KB
18.59 KB

Interdiff:

  • Fix test failures
  • Use the same constant values as C++ does, comments still need to be adjusted Forgot to include that
  • Add support for step="any", add tests
  • Use the new valid_number_step() function
  • false -> FALSE, true -> TRUE
  • We can't introduce new t() strings in test cases, so !name -> %name
  • Removed baby-kittens-new-line-fixes from the patch
Niklas Fiekas’s picture

Now changed those values. Removed references to IEEE 754 from the comment, because that is probably no longer valid.

(Edit: Lol at the filename.)

Niklas Fiekas’s picture

Oh ... found another baby-kitten hunk.

Edit: We should probably eliminate element_validate_number() in the same follow-up that makes the number field type use the number element type.

Status: Needs review » Needs work
Issue tags: -html5, -sprint

The last submitted patch, 1174640-html5-number-98.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: +html5, +sprint

#98: 1174640-html5-number-98.patch queued for re-testing.

Niklas Fiekas’s picture

Looks like the one in #98 wasn't intented to be a baby-kitten. It was just the wrong function being cleaned up. (<-- Lame excuse to reupload the patch, while one poor testbot still hangs on #98).

Status: Needs review » Needs work

The last submitted patch, 1174640-html5-number-101.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
812 bytes
18.01 KB

Mhh ... typo when converting !($a && $b) to !$a || $!b. And using $mantissa where $digits should be used.

Niklas Fiekas’s picture

Issue tags: -html5, -sprint

Status: Needs review » Needs work
Issue tags: +html5, +sprint

The last submitted patch, 1174640-html5-number-103.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
457 bytes
19.92 KB

Rebased + added a comment that the other elements have, too.

Niklas Fiekas’s picture

Eww ... did a mistake when rebasing and added the _element_number_validate_step_mismatch number function back.

Dave Reid’s picture

I would still really like to see this patch remove element_validate_number() since it's completely duplicated by form_validate_number()? Thoughts?

Niklas Fiekas’s picture

We should do that when making the number field widget an HTML5 widget as a follow-up, because we can't directly replace it. For example we'd have to provide #step, which comes for free once we use the real widget.
#98: Edit: We should probably eliminate element_validate_number() in the same follow-up that makes the number field type use the number element type.

Dave Reid’s picture

xjm is working on a more detailed review but here's a few things:

+++ b/core/includes/form.incundefined
@@ -3836,6 +3836,66 @@ function theme_tel($variables) {
+  if($value === '') {
+    return;

Missing a space after the if statement.

+++ b/core/includes/form.incundefined
@@ -3836,6 +3836,66 @@ function theme_tel($variables) {
+  if (strtolower($element['#step']) != 'any') {
+    // Check that the input is an allowed multiple of #step (offset by #min if
+    // #min is set).
+    $min = isset($element['#min']) ? $element['#min'] : 0.0;
+
+    if (!valid_number_step($value, $element['#step'], $min)) {
+      form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
+    }

We should make #step optional here (with an isset()) so that we could replace element_validate_number() with this function.

xjm’s picture

Status: Needs review » Needs work

I dig the nice thorough testing. Docs review:

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,51 @@ function valid_url($url, $absolute = FALSE) {
+ * @param $value
...
+ * @param $step
...
+ * @param $offset
...
+ * @return

We could probably start adding datatypes here, no? Seems particularly relevant here. :)
http://drupal.org/node/1354#param-return-data-type

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,51 @@ function valid_url($url, $absolute = FALSE) {
+ *   TRUE if no step mismatch has occured
+ *   FALSE if a step mismatch has occured.

I'd put these in one (wrapping) line; either
TRUE if blah, or FALSE if blah.
or
TRUE if blah, or FALSE otherwise.

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,51 @@ function valid_url($url, $absolute = FALSE) {
+  // Double's fractional part size is $mantissa-bit. If the current value
+  // is greater than step*2^$mantissa, the following computation for remainder
+  // makes no sense.

I'd say "A double's..."; I had to read this comment a few times to understand. Also, let's follow our coding standards in comments too (spaces around operators) and maybe add some parens to make step*2^$mantissa less ambiguous. :)

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,51 @@ function valid_url($url, $absolute = FALSE) {
+  // The computation follows HTML5 4.10.7.2.10 'The step attribute':
+  // ... that number subtracted from the step base is not an integral multiple
+  // of the allowed value step, the element is suffering from a step mismatch.

Huh?

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,51 @@ function valid_url($url, $absolute = FALSE) {
+  // Accepts erros in lower fractional part.

Typo: erros.

+++ b/core/includes/form.incundefined
@@ -3836,6 +3836,66 @@ function theme_tel($variables) {
+    form_error($element, t('%name must be a number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
...
+    form_error($element, t('%name must be higher or equal to %min.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'], '%min' => $element['#min'])));
...
+    form_error($element, t('%name must be below or equal to %max.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'], '%max' => $element['#max'])));
...
+      form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -222,6 +222,80 @@ class FormsTestCase extends DrupalWebTestCase {
+          $this->assertRaw(t($errorMessages[$id], $placeholders));
...
+          $this->assertNoRaw(t($errorMessages[$id], $placeholders));

We can use format_string() rather than t() on the assertion message strings. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -2002,6 +2002,53 @@ class CommonValidUrlUnitTestCase extends DrupalUnitTestCase {
+  function testNumberStep() {
...
+  function testNumberStepOffset() {

Can we get some inline comments in these so that they're not just a bunch of numbers? :)

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -222,6 +222,80 @@ class FormsTestCase extends DrupalWebTestCase {
+    /**
+     * The $expected array contains a key for each form title. For each title
+     * there are 4 different booleans, each boolean tells us where we expect a
+     * particular error. Below is the list of the possible errors in the same
+     * order as they would appear in the $errorMessages array.
+     *
+     * 1) %name must be a number.
+     * 2) %name must be higher or equal to %min.
+     * 3) %name must be below or equal to %max.
+     * 4) %name is not a valid number.
+     */

This should use // on each line (even though it's long).

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -222,6 +222,80 @@ class FormsTestCase extends DrupalWebTestCase {
+    $expected = array(
+      'integer_no_number' => array(TRUE, FALSE, FALSE, FALSE),
+      'integer_no_step' => array(FALSE, FALSE, FALSE, FALSE),

Hmm, even with the comment above this is kind of overwhelming and confusing. Maybe some inline comments as well? Anything we can do to help make sense of the BOOLEAN BOOLEAN BOOLEAN BOOLEAN, BOOLEAN BOOLEAN BOOLEAN BOOLEAN, BOOLEAN BOOLEAN BOOLEAN BOOLEAN, BOOLEAN BOOLEAN BOOLEAN BOOLEAN.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
9.9 KB
17.41 KB

Reroll with that. Thank you!

Leaving out the typed @param thing, as discussed in IRC: It isn't clear about float vs. float|int vs. float|int|string, yet.

Made step*2^$mantissa step * 2^$mantissa. I think that makes it clearer without brackets, and that works without spaces around ^. This is more mathematics than PHP code, that doesn't even have ^ for exponents.

Niklas Fiekas’s picture

As @Davereid pointed out:

Replace element_validate_number() with form_validate_number(), since they do (at least without #min, #max and #step) the same thing and can be swapped just like that.

Follow-ups:

  • Replace textfields with number fields where it makes sense.
  • Consider removing element_validate_integer() that would then have no core uses cases. Only the error message differs from form_validate_number() with default step.
  • Consider removing element_validate_integer_positive(). Only the error message differs from form_validate_number() with default step and #min = 0.
yched’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.incundefined
@@ -3836,6 +3836,66 @@ function form_validate_number(&$element, &$form_state) {
+  if (!is_numeric($value)) {
+    form_error($element, t('%name must be a number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
+  }

We should return after this error - the subsequent checks on min, max, step, are only valid if the value is numeric to begin with.

19 days to next Drupal core point release.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
533 bytes
18.91 KB

Oh, good point.

Status: Needs review » Needs work
Issue tags: -html5, -sprint

The last submitted patch, 1174640-html5-number-115.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#115: 1174640-html5-number-115.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1174640-html5-number-115.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: +html5, +sprint

Looks like git.d.o is up, again.

#115: 1174640-html5-number-115.patch queued for re-testing.

cosmicdreams’s picture

In manual testing of 115 I got stuck testing the "Float test with a small step" field. It kept telling me that I entered an incorrect number. It didn't let me click the up or down arrows to the right of the field to change the value. Is that what should have happened?

Is this one supposed to fail? If so, it's failure prevents me from testing the remaining fields.

xjm’s picture

testNumber() is way easier to understand now. Yay.

Three other bits:

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,50 @@ function valid_url($url, $absolute = FALSE) {
+  // A double's fractional part size is $mantissa-bit. If the current value is

Missed this one: $mantissa - $bit. :)

+++ b/core/includes/common.incundefined
@@ -1141,6 +1141,50 @@ function valid_url($url, $absolute = FALSE) {
+  // The computation follows HTML5 4.10.7.2.10 'The step attribute': Check that
+  // the number subtracted from the step base is an integral multiple of the
+  // allowed value step.

I still can't quite make sense of this comment.

+++ b/core/includes/form.incundefined
@@ -3836,6 +3836,67 @@ function theme_tel($variables) {
+ * @param $variables
+ *   An associative array containing:
+ *   - element: An associative array containing the properties of the element.
+ *     Properties used: #title, #value, #description, #size, #min, #max,
+ *     #placeholder, #required, #attributes, #step.

I meant to ask this before but forgot, sorry. This makes it sound like all the properties are required, but I don't think they are?

#120 sounds problematic; could you clarify how precisely you were testing it (and what browser perhaps)?

aspilicious’s picture

So we need to look at #120, #121 and

Chx almost freaked out on the ported c++ code so he wants us to add this line:
"PHP typically use IEEE-754 double. If your doesn't (this should be extremely rare) the results of this function is not reliable."
To the step validation function

cosmicdreams’s picture

In regard to #120 I tested with Chromium (the latest) and I tested by installing the form-test module an manually tried to submit the form. This led to a series of errors, most of which made sense and should have happened based on the initial inputs. However the test I called out in #120 could not be made to have an acceptable value.

Niklas Fiekas’s picture

Status: Needs review » Needs work

@cosmicdreams: Thanks for testing. Looks like you found a webkit bug - output and value are clearly valid, numbers in the format 1e-13 are acceptable as per the standarts and it can be reproduced with a small encapsulated form, not just with Drupal. Filed an issue here: https://bugs.webkit.org/show_bug.cgi?id=81036.

Needs work for the other points.

cosmicdreams’s picture

@Niklas Fiekas : Awesome! if you don't beat me to the patch I'll do it later today.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
19.81 KB

Ok. Given the problem @cosmicdreams found (Thank you, again!) I added some more tests (including the specific case Webkit has problems with).

I also reworded the comments of valid_number_step(), hopefully making clearer what we are doing there and added the disclaimer about IEEE 754 that @chx wanted.

Regarding #121: Having that all other elements say which properties they support in the same way, I do think we should leave it at that and don't make it inconsistent. Changing all of those descriptions at once - keeping consistency - would make a nice follow-up novice issue.

Edit: Ugg ... the interdiff shouldn't be a patch file.

Status: Needs review » Needs work

The last submitted patch, 1174640-interdiff-127.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review

So ... anything else? ;)

xjm’s picture

Status: Needs review » Needs work

Yeah @cosmicdreams, excellent catch. :)

I find the latest iteration of the patch much easier to understand. Did a full review from the bottom up and found a couple of minor things:

  1. +++ b/core/includes/form.incundefined
    @@ -3894,6 +3894,67 @@ function theme_tel($variables) {
    +/*
    + * Returns HTML for a number form element.
    + *
    + * @param $variables
    

    Missing second asterisk at the top of this docblock.

  2. +++ b/core/includes/form.incundefined
    @@ -3894,6 +3894,67 @@ function theme_tel($variables) {
    +    form_error($element, t('%name must be a number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
    ...
    +    form_error($element, t('%name must be below or equal to %max.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'], '%max' => $element['#max'])));
    

    This ternary is a bit painful to read inline, and the check seems to be the same for all of the error messages. Can we possibly define one variable for %name and use that in the t() instead for all of them?

  3. +++ b/core/modules/simpletest/tests/form.testundefined
    @@ -298,6 +298,69 @@ class FormsTestCase extends DrupalWebTestCase {
    +        // Check if the error exists on the page, if the current message id is
    

    "ID" should be in caps.

  4. +++ b/core/modules/simpletest/tests/form_test.moduleundefined
    @@ -1088,11 +1094,126 @@ function form_test_select_submit($form, &$form_state) {
    +/*
    + * Builds a form to test #type 'number' validation.
    + */
    

    Missing second asterisk at the top of this docblock.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
19.59 KB

Thanks for the review.

As for (2) I don't think we should be doing that in the first place.

Niklas Fiekas’s picture

@aspilicious pointed out that we do indeed need that fallback, so adding it back.

xjm’s picture

Looked through the patch again and everything looks good to me. The test coverage is very thorough.

Niklas Fiekas’s picture

Thank you.

I think this could get in soon, which is awesome, because this blocks a many things.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's do this!

aspilicious’s picture

Issue tags: -html5, -sprint

#132: 1174640-html5-number-132.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +html5, +sprint

The last submitted patch, 1174640-html5-number-132.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
19.7 KB
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in!

chx’s picture

Status: Reviewed & tested by the community » Needs work

+ * The implementation assumes it is dealing with IEEE 754 single precision

No, it's double precision.

Also, "Due to the way floating point numbers work there might be a small range of errors when using other representations." no, what you wanted to say is "If your PHP uses something else (again, this should be extremely rare) then the results of this function is not defined." Perhaps we want to add something small to the status report? Note that this whole paragraph is entirely optional, don't let me hold up the patch.

Thanks for the awesome work!

chx’s picture

I did more digging and found http://www.mscs.mu.edu/~georgec/IFAQ/casares1.html . I think linking this would be beneficial because it more clearly states what "extremely rare" means: everything x86 is fine. Power, PowerPC, SPARC, Alpha, all fine. Old IBM mainframes and old Cray mainframes -- uh oh.

One can educate yourself further by reading http://stackoverflow.com/questions/2234468/do-any-real-world-cpus-not-us... . Lots of fun.

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community

#140 sounds like a follow up issue. moving patch's status back to RBTC given the non-blocker status of the comment.

cosmicdreams’s picture

#140 sounds like a follow up issue. moving patch's status back to RBTC given the non-blocker status of the comment.

Niklas Fiekas’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.96 KB
29.64 KB

Ignore those. Selected the wrong patch files.

Niklas Fiekas’s picture

Sorry. Wrong patches.

About this one: Basically agreed with the follow up, if at all. But let's not say something wrong.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok lets get this in now...

chx’s picture

I am OK with this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all. Committed/pushed to 8.x.

Niklas Fiekas’s picture

Yay! This unblocks range, date, datetime, ... ... awesome :)

Extended the change record.

Niklas Fiekas’s picture

Niklas Fiekas’s picture

Niklas Fiekas’s picture

Status: Fixed » Needs review
FileSize
788 bytes

I knew I'd forget something (again).

xjm’s picture

Status: Needs review » Reviewed & tested by the community

No-brainer I think. More test coverage++.

That said, is there any way we can be sure we are not missing other coverage for it? E.g., what about FormTestCase:testRequiredFields()?

Niklas Fiekas’s picture

Yep, this should be the only thing that's missing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Commmitted/pushed the follow-up.

Niklas Fiekas’s picture

Status: Fixed » Closed (fixed)

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

Xano’s picture

Xano’s picture

Issue summary: View changes

Edit

Alex Bukach’s picture