zencart / zencart

Zen Cart® is a full-function e-commerce application for your website.
https://github.com/zencart/zencart/releases
Other
375 stars 233 forks source link

function zen_date_diff returns a float #5491

Closed torvista closed 1 year ago

torvista commented 1 year ago

Should not the difference between two dates be an integer/whole number and not a float?

https://github.com/zencart/zencart/blob/67f798ef27c6f6006d8aadf7f0a3b0fa17348e06/includes/functions/functions_dates.php#L269-L288

lat9 commented 1 year ago

The commentary description indicates that it returns a bool|int (where the bool comes from I can't fathom); I agree that the returned value should be of (int) type.

That brings on another question, though ... why are we reinventing the wheel instead of simply using PHP's date_diff?

scottcwilson commented 1 year ago

The commentary description indicates that it returns a bool|int (where the bool comes from I can't fathom); I agree that the returned value should be of (int) type.

Yep, I think the bool comment should be removed and the final result should be casted to int.

That brings on another question, though ... why are we reinventing the wheel instead of simply using PHP's date_diff?

I think the answer is, this logic was written (in ZC 1.3.9) prior to the availability of date_diff in all relevant PHP versions (5.2.10-5.3).

drbyte commented 1 year ago

Should not the difference between two dates be an integer/whole number and not a float?

Yes, that'd be sensible.

round() returns a float

The commentary description indicates that it returns a bool|int (where the bool comes from I can't fathom).

Turns out the bool|int was set by my IDE when I was refactoring date functions into functions_dates.php in https://github.com/zencart/zencart/pull/3673

I agree that the returned value should be of (int) type.

Agreed.

drbyte commented 1 year ago

instead of simply using PHP's date_diff?

How about this rewrite?

/**
 * compute the days between two dates
 * @param string $date1 today, or any other day
 * @param string $date2 date to check against
 * @return int
 */
function zen_date_diff($date1, $date2)
{
    $d1 = date_create($date1);
    $d2 = date_create($date2);
    $interval = date_diff($d1, $d2);

    // %a is total number of days, regardless of months
    return (int) $interval->format('%a');
}
scottcwilson commented 1 year ago

Check for $d1 or $d2 == false?

drbyte commented 1 year ago

Check for $d1 or $d2 == false?

and take what action?

scottcwilson commented 1 year ago

Don't do further math and return 0 would be my suggestion.

drbyte commented 1 year ago

Returning 0 on error would potentially introduce unexpected bugs if the returned result is hoping for 0. That would be new behavior of the function.

Maybe die() would be a better reaction to invalid inputs.

lat9 commented 1 year ago

Returning 0 on error would potentially introduce unexpected bugs if the returned result is hoping for 0. That would be new behavior of the function.

Maybe die() would be a better reaction to invalid inputs.

die() in this case could be catastrophic for a site owner. Noting, though, that the only storefront use of that function in the base distribution is in `includes/modules/payment/paypal/paypal_functions.php'.

FWIW, I'm not aware of any plugins that use the zen_date_diff function.

scottcwilson commented 1 year ago

I agree with @lat9 - I don't like die(). Should we go back to a minimalist implementation - simply cast the return value and correct the misleading comment?

scottcwilson commented 1 year ago

Let's put this one to bed.