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.
Comment | File | Size | Author |
---|---|---|---|
#152 | 1174640-forgot-placeholder-testcoverage.patch | 788 bytes | Niklas Fiekas |
#145 | 1174640-html5-number-144.patch | 19.58 KB | Niklas Fiekas |
#145 | 1174640-html5-number-144-interdiff.txt | 777 bytes | Niklas Fiekas |
#144 | 1463656-drupal-kernel-54.patch | 29.64 KB | Niklas Fiekas |
#144 | 1463656-drupal-kernel-54-interdiff.txt | 4.96 KB | Niklas Fiekas |
Comments
Comment #1
Dave ReidThoughts:
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.
Comment #2
ericduran CreditAttribution: ericduran commentedWe'll also want to add the #step attribute.
Comment #3
ericduran CreditAttribution: ericduran commentedComment #4
Dave ReidAnother concern: we could probably replace core's weight select fields with this new number element.
Comment #5
ericduran CreditAttribution: ericduran commented1st patch,
still need to change all the form field that are numbers to use the new element.
But uploading for simpler test-bot testing. ;)
Comment #6
ericduran CreditAttribution: ericduran commentedNew 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)
Comment #7
ericduran CreditAttribution: ericduran commented- Removing _element_validate_number
- adding min, max support.
Comment #9
ericduran CreditAttribution: ericduran commentedugg, apparently the number field doesn't accept decimals.
Comment #10
ericduran CreditAttribution: ericduran commentedOk, I need to add the step, attribute is if a decimal.
Comment #11
ericduran CreditAttribution: ericduran commentedComment #12
ericduran CreditAttribution: ericduran commentedComment #14
ericduran CreditAttribution: ericduran commented#11: 117464-html5-number-element.patch queued for re-testing.
Comment #15
voxpelli CreditAttribution: voxpelli commentedSome more feedback.
Doesn't include #min, #max (and #placeholder) - also not #id and #name?
According to Mozilla the size, maxlength, placeholder attributes are ignored for the number-type: https://developer.mozilla.org/en/HTML/element/input#attr-maxlength
Same feedback regarding the form-text class here as on #1174646: Add new HTML5 FAPI element: range
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.
Comment #16
yareckon CreditAttribution: yareckon commentedsub.
Comment #17
ericduran CreditAttribution: ericduran commented@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.
Comment #18
ericduran CreditAttribution: ericduran commentedRerolled,
- 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.
Comment #19
ericduran CreditAttribution: ericduran commentedComment #20
Dave ReidI 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().
Comment #21
voxpelli CreditAttribution: voxpelli commentedWhy should "placeholder" be included when it's not usable by this input-type?
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.
Comment #22
ericduran CreditAttribution: ericduran commentedAlso need to add validation for the step attribute. See http://www.w3.org/TR/html5/association-of-controls-and-forms.html#suffer...
Comment #23
aspilicious CreditAttribution: aspilicious commentedIgnore this
Comment #24
aspilicious CreditAttribution: aspilicious commentedHere 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.
Comment #25
ericduran CreditAttribution: ericduran commentedwe can just leave this in the form_validate_number
We can't rely on #title.
This is a helper function for a helper function. We can just do this is number_validate
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.
Comment #26
ericduran CreditAttribution: ericduran commentededit: double post.
Comment #27
aspilicious CreditAttribution: aspilicious commentedOk 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
Comment #28
aspilicious CreditAttribution: aspilicious commentedForgot patch
Comment #29
ericduran CreditAttribution: ericduran commentedLet the test bot review for us 1st ;)
Comment #31
amateescu CreditAttribution: amateescu commentedFixed those form.inc errors.
Comment #32
ericduran CreditAttribution: ericduran commentedThere is no #number property.
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.
Comment #33
aspilicious CreditAttribution: aspilicious commentedPlease review the range patch first because the functions overlap
Comment #34
yched CreditAttribution: yched commentedSubscribe
Comment #35
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #36
ericduran CreditAttribution: ericduran commentedComment #37
JacineRemoving the sprint tag. Also, can we get an issue summary update here? Thanks!
Comment #38
ericduran CreditAttribution: ericduran commentedHmm, this issue is actually pretty close, we probably just need some small changes and a re-roll.
Comment #39
aspilicious CreditAttribution: aspilicious commentedI 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)
And
All tests are passing, I think we handled every edge case.
Comment #40
aspilicious CreditAttribution: aspilicious commentedSmall doc fix. Some comments exceeded 80 chars.
Comment #41
aspilicious CreditAttribution: aspilicious commentedForgot to upload
Comment #43
aspilicious CreditAttribution: aspilicious commentedNo idea why that failed lets try again
Comment #44
tim.plunkettJust some minor nitpicks, leaving cnr for someone to ACTUALLY review it.
missing space
missing space
missing space
no camelCase, missing space
more camelcase
14 days to next Drupal core point release.
Comment #45
aspilicious CreditAttribution: aspilicious commentedOk fixed everything from #44. Waiting for more reviews before I reroll :)
Comment #46
ericduran CreditAttribution: ericduran commentedthis 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.
Comment #47
ericduran CreditAttribution: ericduran commentedHmm, I guess it doesn't have to be number specific since we're also going to use it for the range element.
Comment #48
richthegeek CreditAttribution: richthegeek commented+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:
Other than that, all forseeable issues have been covered and the code is clean and conforms to standards.
Comment #49
aspilicious CreditAttribution: aspilicious commentedHmm lets try this one
Comment #50
aspilicious CreditAttribution: aspilicious commentedor this one
Comment #51
ericduran CreditAttribution: ericduran commentedI'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.
Comment #52
tim.plunkettI 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 :)
Comment #53
ericduran CreditAttribution: ericduran commentedThanks. Not bikesheding just an observation and a simple question.
We'll see what the test said.
Comment #55
tim.plunkettSorry 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
Comment #56
ericduran CreditAttribution: ericduran commented@tim.plunkett ha my reply came of rude. Sorry about that. :-p
Comment #57
ericduran CreditAttribution: ericduran commentedI think this is the line @tim.plunkett was talking about define('SIMPLETEST_COLLECT_ERRORS', FALSE);
Comment #58
tim.plunkettJust trying this out.
Hopefully this appeases ericduran's fury ;)
Comment #59
aspilicious CreditAttribution: aspilicious commentedI 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.
Comment #60
aspilicious CreditAttribution: aspilicious commentedAnd 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)
Comment #61
aspilicious CreditAttribution: aspilicious commentedOk 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.
Comment #62
aspilicious CreditAttribution: aspilicious commentedAdded 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... :(
Comment #64
JacineYay! Awesome to see activity here. Tagging this for the current sprint. :D
Can anyone help @aspilicious with the test failures?
Comment #65
ericduran CreditAttribution: ericduran commentedFixing text now. Patch soon to come.
Comment #66
ericduran CreditAttribution: ericduran commentedx-post
Comment #67
Dave ReidHrm, 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.
Comment #68
ericduran CreditAttribution: ericduran commentedThis 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.
Comment #69
ericduran CreditAttribution: ericduran commentedfor the test bot.
Comment #70
Dave ReidI 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.
Comment #71
ericduran CreditAttribution: ericduran commentedAnd here it is with the css.
Comment #72
ericduran CreditAttribution: ericduran commentedActually 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.
Comment #73
aspilicious CreditAttribution: aspilicious commentedI don't think we need this line than.
Otherwise this looks kinda rtbc
-24 days to next Drupal core point release.
Comment #74
aspilicious CreditAttribution: aspilicious commentedComment #75
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedA few issues and/or remarks.
Superflous space.
Repeating the same error message here. Maybe there is a nicer way.
Trailing dots missing. Also, I usually expect validation functions to work the other way around.
Do we have line breaks at 80 characters or can we have them earlier in favor of having "is greater" together?
Wierd quotes here.
No trailing dot.
Not sure if we should touch it: Don't translate assertion messages.
Don't translate the title.
Yep. Probably remove this.
Not sure if this should be one line or more.
Comment #76
Dave ReidFew thoughts
We should probably still support #size for browsers that fallback to default text behavior?
Default size might be a little big? Maybe use 30 as a default?
22 days to next Drupal core point release.
Comment #77
ericduran CreditAttribution: ericduran commentedUpdate, fix all the comments mention above.
Comment #79
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, thanks. Next round.
Since
0
might be a legitimate number, we probably need to check$value === ''
here. Alsoif_(
and always use{ }
.Too much leading space, here.
And here.
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.
Usually t() on a $variable isn't ok, but yeah ... it's a testcase and the error message is defined in another place already.
If we touch the line, remove the t().
Add a trailing
,
?Superflous empty line.
See http://drupal.org/node/1354#forms for doxygen on form constructors.
Comment #80
cosmicdreams CreditAttribution: cosmicdreams commentedFixed most of the formatting issues called out in 79.
Comment #81
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank 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?
Comment #83
cosmicdreams CreditAttribution: cosmicdreams commenteddang, 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.
Comment #84
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMuch better. Thanks.
Doxygen is supposed to start with
/**
.Comment #85
cosmicdreams CreditAttribution: cosmicdreams commentedFixed that comment with this patch:
Comment #86
aspilicious CreditAttribution: aspilicious commentedThe 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!
Comment #87
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch with that small change.
Comment #88
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#87: FAPI-number-1174640-87.patch queued for re-testing.
Comment #90
Dave ReidValidation 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).
Comment #91
Dave ReidAlso can we merge or get rid of element_validate_number() now that we have form_validate_number()?
Comment #92
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRebased + #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.
Comment #93
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedFor 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:
Comment #95
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk. 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)
Note that were using 'Float test hard' as the title here.
And the same #title here.
Comment #96
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedInterdiff:
Use the same constant values as C++ does, comments still need to be adjustedForgot to include thatComment #97
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNow changed those values. Removed references to IEEE 754 from the comment, because that is probably no longer valid.
(Edit: Lol at the filename.)
Comment #98
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... 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.
Comment #100
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#98: 1174640-html5-number-98.patch queued for re-testing.
Comment #101
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks 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).
Comment #103
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMhh ... typo when converting !($a && $b) to !$a || $!b. And using $mantissa where $digits should be used.
Comment #104
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay! #1174630: Add new HTML5 FAPI element: url got in :)
#103: 1174640-html5-number-103.patch queued for re-testing.
Comment #106
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRebased + added a comment that the other elements have, too.
Comment #107
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedEww ... did a mistake when rebasing and added the _element_number_validate_step_mismatch number function back.
Comment #108
Dave ReidI would still really like to see this patch remove element_validate_number() since it's completely duplicated by form_validate_number()? Thoughts?
Comment #109
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWe 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.
Comment #110
Dave Reidxjm is working on a more detailed review but here's a few things:
Missing a space after the if statement.
We should make #step optional here (with an isset()) so that we could replace element_validate_number() with this function.
Comment #111
xjmI dig the nice thorough testing. Docs review:
We could probably start adding datatypes here, no? Seems particularly relevant here. :)
http://drupal.org/node/1354#param-return-data-type
I'd put these in one (wrapping) line; either
TRUE if blah, or FALSE if blah.
or
TRUE if blah, or FALSE otherwise.
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. :)Huh?
Typo: erros.
We can use
format_string()
rather thant()
on the assertion message strings. Reference: http://drupal.org/simpletest-tutorial-drupal7#tCan we get some inline comments in these so that they're not just a bunch of numbers? :)
This should use // on each line (even though it's long).
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.
Comment #112
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedReroll 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.Comment #113
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAs @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:
Comment #114
yched CreditAttribution: yched commentedWe 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.
Comment #115
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh, good point.
Comment #117
aspilicious CreditAttribution: aspilicious commented#115: 1174640-html5-number-115.patch queued for re-testing.
Comment #119
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks like git.d.o is up, again.
#115: 1174640-html5-number-115.patch queued for re-testing.
Comment #120
cosmicdreams CreditAttribution: cosmicdreams commentedIn 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.
Comment #121
xjmtestNumber()
is way easier to understand now. Yay.Three other bits:
Missed this one:
$mantissa - $bit
. :)I still can't quite make sense of this comment.
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)?
Comment #122
aspilicious CreditAttribution: aspilicious commentedSo 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
Comment #124
cosmicdreams CreditAttribution: cosmicdreams commentedIn 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.
Comment #125
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@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.
Comment #126
cosmicdreams CreditAttribution: cosmicdreams commented@Niklas Fiekas : Awesome! if you don't beat me to the patch I'll do it later today.
Comment #127
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk. 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.
Comment #129
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSo ... anything else? ;)
Comment #130
xjmYeah @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:
Missing second asterisk at the top of this docblock.
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 thet()
instead for all of them?"ID" should be in caps.
Missing second asterisk at the top of this docblock.
Comment #131
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for the review.
As for (2) I don't think we should be doing that in the first place.
Comment #132
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@aspilicious pointed out that we do indeed need that fallback, so adding it back.
Comment #133
xjmLooked through the patch again and everything looks good to me. The test coverage is very thorough.
Comment #134
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you.
I think this could get in soon, which is awesome, because this blocks a many things.
Comment #135
aspilicious CreditAttribution: aspilicious commentedOk, let's do this!
Comment #136
aspilicious CreditAttribution: aspilicious commented#132: 1174640-html5-number-132.patch queued for re-testing.
Comment #138
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay! #1174628: Add new HTML5 FAPI element: search got in :)
Rebased.
Comment #139
aspilicious CreditAttribution: aspilicious commentedLet's get this in!
Comment #140
chx CreditAttribution: chx commented+ * 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!
Comment #141
chx CreditAttribution: chx commentedI 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.
Comment #142
cosmicdreams CreditAttribution: cosmicdreams commented#140 sounds like a follow up issue. moving patch's status back to RBTC given the non-blocker status of the comment.
Comment #143
cosmicdreams CreditAttribution: cosmicdreams commented#140 sounds like a follow up issue. moving patch's status back to RBTC given the non-blocker status of the comment.
Comment #144
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedIgnore those. Selected the wrong patch files.
Comment #145
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSorry. Wrong patches.
About this one: Basically agreed with the follow up, if at all. But let's not say something wrong.
Comment #146
aspilicious CreditAttribution: aspilicious commentedOk lets get this in now...
Comment #147
chx CreditAttribution: chx commentedI am OK with this.
Comment #148
catchThanks all. Committed/pushed to 8.x.
Comment #149
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay! This unblocks range, date, datetime, ... ... awesome :)
Extended the change record.
Comment #150
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSpinned of first follow up: #1519720: Remove element_validate_integer() and element_validate_integer_positive() replace with number element type.
Comment #151
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAnother follow up: #1519852: Use the new number FAPI element for the number field textfield widget.
Comment #152
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI knew I'd forget something (again).
Comment #153
xjmNo-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()
?Comment #154
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYep, this should be the only thing that's missing.
Comment #155
catchThanks! Commmitted/pushed the follow-up.
Comment #156
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNext follow-up: #1527988: Missing or legacy number validation.
Comment #158
XanoNote that #148 introduced a bug: #1964192: form_validate_number() excludes countries that use commas.
Comment #158.0
XanoEdit
Comment #159
Alex Bukach CreditAttribution: Alex Bukach commentedShould't number type be added to https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...?