added null check, so that zeros don’t turn up unexpectantly #7103
Conversation
There was a problem hiding this comment.
It seems ok, but it would be good to have a test. @pitchandtone could you add a check for empty strings and nulls being converted to null?
pitchandtone
force-pushed
the
fix-numericfield-null
branch
from
1f60f04
to
fe0e6b3
Compare
|
Tests added. |
tests/php/Forms/NumericFieldTest.php
Outdated
| public function testNullSet() { | ||
| $field = new NumericField('Number'); | ||
| $field->setValue(''); | ||
| $this->assertEquals($field->Value(), null); |
There was a problem hiding this comment.
Sorry to be pedantic, but the expected value is on the left, expression on the right. :)
src/Forms/NumericField.php
Outdated
| @@ -154,6 +154,9 @@ public function setValue($value, $data = null) | |||
| */ | |||
| protected function cast($value) | |||
| { | |||
| if (strlen($value) === 0) { | |||
| return false; | |||
There was a problem hiding this comment.
Why is false being cast for a numeric field? I would expect null to be used here.
There was a problem hiding this comment.
It's set to false, so that in the Value function it doesn't end up being pushed into a formatter.
I can update the Value function to check is null and false if you'd prefer.
There was a problem hiding this comment.
Let's try null and see if we get better value for money :) It feels more semantically correct than false for empty numeric values.
tests/php/Forms/NumericFieldTest.php
Outdated
| $field->setValue(null); | ||
| $this->assertEquals($field->Value(), null); | ||
| $field->setValue(0); | ||
| $this->assertEquals($field->Value(), 0); |
There was a problem hiding this comment.
Can you also please test dataValue()
pitchandtone
force-pushed
the
fix-numericfield-null
branch
from
f89936f
to
aebe4ef
Compare
|
Reworked now with null check. |
tests/php/Forms/NumericFieldTest.php
Outdated
| public function testNullSet() { | ||
| $field = new NumericField('Number'); | ||
| $field->setValue(''); | ||
| $this->assertEquals(null, $field->Value()); |
There was a problem hiding this comment.
assertNull would be a more appropriate assertion here - but I won't block merge over it
There was a problem hiding this comment.
OK - I fixed this as part of the rebase
dhensby
force-pushed
the
fix-numericfield-null
branch
from
aebe4ef
to
cb6b488
Compare
|
I've rebased this onto 4.0 so that it makes the 4.0 release |
dhensby
force-pushed
the
fix-numericfield-null
branch
from
cb6b488
to
ada32fb
Compare
|
It appears the tests aren't passing :/ |
tests/php/Forms/NumericFieldTest.php
Outdated
| public function testNullSet() { | ||
| $field = new NumericField('Number'); | ||
| $field->setValue(''); | ||
| $this->assertNull($field->Value()); |
There was a problem hiding this comment.
It's coming up as '', which is probably should be the expected value for this test. Only the datavalue() should be null, although it emits an empty string to the user through the front-end Value() method
I suggest:
- changing setValue() to setSubmittedValue() (to simulate a user submitting an empty string)
- expect these invalid strings to be returned as-is ($this->originalValue).
There was a problem hiding this comment.
I'll leave this for @pitchandtone to tidy up. Remember to pull the commit locally that I've pushed up...
pitchandtone
force-pushed
the
fix-numericfield-null
branch
from
ada32fb
to
64547ba
Compare
pitchandtone
force-pushed
the
fix-numericfield-null
branch
from
64547ba
to
f14e6ba
Compare
|
@dhensby I've done the rebase on 4.0 here, and fixed that text, let me know if there's anything missing. |
|
There's a code style issue, but I'll merge by hand and fix it. |
|
Good job @pitchandtone and thanks for picking up the extra cleanup @dhensby |
No description provided.