Silverstripe-framework: [ORM] currency field that is set to zero evaluates to TRUE

[sunnysideup]

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

• set up dataobject

class MyProduct extends DataObject {
    private static $db = array(
        "MyPrice" => "Currency",
        "MyDouble" => "Double"
    );
}

• setup test

    function test(){
        $MyProduct = MyProduct::get()->last();
        $MyProduct->MyDouble = 0;
        $MyProduct->MyPrice = 0;
        $MyProduct->write();
        if($MyProduct->MyDouble) {
            db::alteration_message("MyDouble with new DataObject: ERROR", "deleted");
        }
        else {
            db::alteration_message("MyDouble with new DataObject: OK", "created");
        }
        $MyProduct = MyProduct::get()->last();
        if($MyProduct->MyDouble) {
            db::alteration_message("MyDouble with saved DataObject: ERROR", "deleted");
        }
        else {
            db::alteration_message("MyDouble with saved DataObject: OK", "created");
        }
        $MyProduct->MyDouble = 0;
        $MyProduct->MyPrice = 0;
        $MyProduct->write();
        if($MyProduct->MyPrice) {
            db::alteration_message("MyCurrency with new DataObject: ERROR", "deleted");
        }
        else {
            db::alteration_message("MyCurrency with new DataObject: OK", "created");
        }
        $MyProduct = MyProduct::get()->last();
        if($MyProduct->MyPrice) {
            db::alteration_message("MyCurrency with saved DataObject: ERROR", "deleted");
        }
        else {
            db::alteration_message("MyCurrency with saved DataObject: OK", "created");
        }
    }

This returns the following

[sunnysideup]

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?

[jedateach]

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.

[sunnysideup]

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.

[sunnysideup]

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.

[jedateach]

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.
However, in this case, yes it is strange behaviour that could catch someone out.

The nullValue function I believe is the value used to write to the database when the value is empty, instead of leaving values NULL in the database.

I like your proposal that Currency's nullValue method returns 0 or "0" instead. These both will evaluate to false correctly. Plus, I don't imagine they will cause any issues saving to the database.

[kinglozzer]

I’ve encountered this before in templates too:

<% if $Price %>
    {$Price}
<% end_if %>

Will return 0.00

[tractorcow]

We should override exists method in DBCurrency, so that at least in templates the truthiness of the value can be corrected.

[jonom]

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:

<% if $Price %> tests that a value exists, even if it is set to 0 (is not null)
<% if $Price > 0 %> tests that a value exists and is positive

[stevie-mayhew]

I agree with jonom here - no price is different from 0 price.

[tractorcow]

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.

[tractorcow]

I agree with jonom here - no price is different from 0 price.

I disagree. ;) Free things have no price.

[jonom]

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.

[SpiritLevel]

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 Currency is a subclass of Decimal so I assume Currency can be negative, zero, or positive. And. I assume Money is the same. If not, then both Currency and Money should definitely be changed to allow negative, zero, and positive values, otherwise no developer wanting to create a serious financial/economic/business intelligence web application would choose SS.

Whether you are talking about Currency, Money or any other numerical field, if a value of zero has been assigned to a variable then this variable should have a value of true for exists(). Zero, is, after all, often a perfectly valid numerical value! Hence, I think there may be a problem when, as @tractorcow says, 'we treat "0" as "not exists" for other numeric fields.' I think that exists() should only return false if a value has not been assigned.

That's my 2 cents, which is neither "no price" nor "unassigned price" and which definitely "exists()" :P

[jonom]

I knew those worms would get loose again!

[SpiritLevel]

It is hard to grasp small, slippery things that wriggle...;P

[tractorcow]

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.

[SpiritLevel]

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 tempInCelsius, should definitely return true for exists() when the variable actually is zero. Anything else just doesn't make sense for a method called exists(). Strings are a strange case though, as you mentioned. Hmm...

[tractorcow]

Hey @tractorcow...If you got to what point ?

Of wanting to distinguish between "no" value, and an explicit non-empty zero value.

[tractorcow]

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.

[SpiritLevel]

Yeah, I was thinking of isSet() or isAssigned()...

Maybe define an isNull() and its negation isSet() to avoid using the negation operator ! too often ?

[tractorcow]

I think there's __isset magic method, so that isset($val) works.

[sminnee]

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.

    public function testZeroIsFalse()
    {
        $obj = new DataObjectTest\FieldTypes();
        $obj->MyInt = 0;
        $obj->MyDecimal = 0.00;
        $obj->MyCurrency = 0.00;
        $obj->write();

        $this->assertFalse((bool)$obj->MyInt);
        $this->assertFalse((bool)$obj->MyDecimal);
        $this->assertFalse((bool)$obj->MyCurrency);

        $obj2 = DataObjectTest\FieldTypes::get()->byId($obj->ID);
        $this->assertFalse((bool)$obj2->MyInt); // return false as hoped
        $this->assertFalse((bool)$obj2->MyDecimal); // returns true, bug
        $this->assertFalse((bool)$obj2->MyCurrency); // returned true, bug
    }

This would be another bug fixed by #7039.

[sminnee]

The nullable-numeric ticket is here #8135

[sminnee]

OK, with #8448 merged, this has been fixed, to be released in 4.4. #8590 provides a test to confirm this.

[sminnee]

Reopening this - it turns out that #8448 only fixed float values, not decimal.

[sminnee]

Fixed in 4.4