-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ORM] currency field that is set to zero evaluates to TRUE #3473
Comments
In discussion with @simonwelsh on IRC: currency fields (set to 0) from database return 0.00; which PHP sees as as a string and as such it evaluates as true From a coder's perspective this is dangerous, unreliable and counter intuitive. Currency extends Decimal, it does not extend Varchar so I wonder about the following: when we retrieve a Currency value from the database, why not turn it into a proper number? |
I agree that if the magic functions can't be used in calculations, then it will cause issues for developers. I haven't come across this issue myself however. @sunnysideup It would be good to see a unit test that fails because of this finding. |
Hi Jeremy What is wrong with the test above? Do you mean that it would be good to see a real situation where the treatment of a zero currency value, as shown above, would lead to real problems? For example in e-commerce, you could have. /**
* returns true if the product has a price
* @return Boolean
*/
function hasPrice($product){
return $product->Price ? true : false;
} If Price is a currency field then this would always return true... Even if the product had a default (no price) of zero. |
I think the real danger here is that the way it works it so different from how these things usually work in Php. When I think about currency, I think about a number and not a string. A number that equals zero is akin to false in PHP and not true. |
Thanks for the further explanation. I guess it isn't really a bug per se, but more of something that can trip people up. PHP isn't a type-safe language, so there is always the inherent risk - I'd say devs are aware of the risk. The I like your proposal that Currency's |
I’ve encountered this before in templates too: <% if $Price %>
{$Price}
<% end_if %> Will return |
We should override |
It's a bit tricky... a price of $0.00 is still valid (free!) so I kind of think currency fields should allow null values in the database, and a template if test should only fail on a null value. For me the same idea applies to numeric fields - a user entering 0 in a field has a different meaning to me than a user leaving a field blank. That's a can of worms... but to open it for just a moment:
|
I agree with jonom here - no price is different from 0 price. |
exists() isn't a check for "if it's valid", it's a check for if it has a non-empty value. It doesn't make any sense to treat $0.00 as "exists", when we treat "0" as "not exists" for other numeric fields. Changing to use null as a "is empty flag" would need to be treated consistently across the ORM though... do you now want to treat empty strings (non null) as "exists()"? It doesn't make much sense to me sorry. |
I disagree. ;) Free things have no price. |
Fair enough. I don't want these worms to wriggle too far so let me just pick them up and... there. They're back in the can. |
Oops....Jono, you may have missed some worms! There are two senses in which something can have no price and these are getting mixed up in the above conversation. In finance and economics, prices can be negative, zero, or positive. And, all goods and services implicitly have prices since acquiring them involves an exchange of dollars, perhaps zero dollars. A price of zero means you can acquire a good or service without paying any dollars, like air. So, "no price" and "zero price" are really the same idea; the phrase "no price" in this context is just casual, imprecise language to state that the acquisition price is zero. This is the first sense of "no price". A strictly positive price means you have to reach into your pocket and pay dollars to acquire the good or service, like a new car. A strictly negative price means you have to reach into your pocket and pay dollars to get rid of the good or service, like a debt. The second sense in which something can have "no price" is when a price variable in a computer program has not been assigned a value. So, it is probably better to use the phrase "no price" to refer to a price of zero. And, instead, use the phrase "unassigned price" to indicate that a price variable has been declared but no value has yet been assigned. I haven't checked, but I see that Whether you are talking about That's my 2 cents, which is neither "no price" nor "unassigned price" and which definitely "exists()" :P |
I knew those worms would get loose again! |
It is hard to grasp small, slippery things that wriggle...;P |
I think if I got to this point, I'd start splitting my costing across multiple fields, perhaps with currency, value, and an enum for an explicit state. I agree that in db terminology, null closer approximates "not assigned" than "empty value", however it's not a concept that the ORM respects in any way. |
Hey @tractorcow...If you got to what point ? No matter what happens at the db or ORM levels (ie: meanings of null, not assigned, or empty value...), a numerical variable that can take the value zero, such as |
Of wanting to distinguish between "no" value, and an explicit non-empty zero value. |
Solution: keep exists() as a non-falsey check, and add isNull() to DBField to check for nullability? That would be fine for a 3.x change at least. In 4.... you could go all out and distinguish null / non-null across the board, but it would be a breaking change and a significant amount of work. |
Yeah, I was thinking of isSet() or isAssigned()... Maybe define an isNull() and its negation isSet() to avoid using the negation operator ! too often ? |
I think there's |
The pontificating about "is zero a value" is kind of beside the point - we have a situation where Ints treat 0 as empty and Decimal/Currency treats it as not empty. This kind of inconsistency is a bug, plain and simple. The other line of argument would end up with us introducing "nullable" as an option for Ints, Decimals, and Currencies, and then having 0 be "a value" on these. But that has been discussed elsewhere and is a new feature, rather than a bug. This test illustrates the problem, executed on 4.3.
This would be another bug fixed by #7039. |
The nullable-numeric ticket is here #8135 |
Reopening this - it turns out that #8448 only fixed float values, not decimal. |
Fixed in 4.4 |
currency field that is set to zero evaluates to TRUE. This is confusing to say the least - as it goes against all the basics in PHP. To replicate
This returns the following
The text was updated successfully, but these errors were encountered: