FIX Update date.lib.php Function num_between_day

[josett225]

FIX - When debugging Asset Module, I discovered that function num_between_day is returning nothing when $timestampStart equals (==) $timestampEnd
Normally it has to return
0 if $lastday = 0
1 if $lastday = 1

[eldy]

When fixing a bug in old version, it is better to backport the fix from a recent version.
Can you instead suggest a PR that take the code of develop branch that seems to already have this fixed ?

[josett225]

Hi @eldy
Sorry but I did not find the fix in the latest develop version but maybe I did not look in the right version. (See my screenshot)
This is why then I parsed all the versions and I stop at 12.

[josett225]

Hi @eldy
The correction in develop seems to be wrong as we need to get :
0 if $lastday = 0
1 if $lastday = 1
and not always 0

So I am suggesting to add the else part on my solution but we need to add <= to get the value 1 'mandatory in asset module.
Thoughts ?

[josett225]

Hi @eldy
The correction in develop seems to be wrong as we need to get :
0 if $lastday = 0
1 if $lastday = 1
and not always 0

So I am suggesting to add the else part on my PR but we need to add <= to get the value 1 mandatory in asset module in dev code. I can do another PR to fix dev first?
Thoughts ?

[eldy]

I don't see what you should get 1 when the 2 date are the same, because we should never call the method with the 2 timestamp that are same (that contains hours). So 0 is a default value when param are wrong.
In which part of code do you call the method with 2 similar values ?

[josett225]

Hi @eldy
It is in Asset management
We need to get when we add an asset the number of days between the starting date and the FY end date.
Therefore when the starting date of asset equals end date of FY, the value that has to be returned is 1 and not 0 because it is still 1 day of asset.

[josett225]

Hi @eldy
Reading again your point about the time, I believe we need to clean all the datetime params to remove time value or set all the dates submitted with time set up with 00:00:00 in asset code.
Based on this however we still need the modification of this PR.

[josett225]

Hi @eldy
Doing more investigation, I discovered also that the function is returning sometimes the wrong number of days.
Here is an exemple :
begin_date = 1514329200
end_date = 1538258400
LastDay = 1
BEGIN DATE FORMATTED 2017-12-27 00:00:00
END DATE FORMATTED 2018-09-30 00:00:00
Function num_between_day returned = 277
The correct number of days is 278

The issue is coming from the use of function floor() instead of ceil()

Here is the explanation :
Inputs:
Start Timestamp (begin_day): 1514329200 corresponds to 2017-12-27 00:00:00.
End Timestamp (end_day): 1538258400 corresponds to 2018-09-30 00:00:00.
Last Day Inclusion (LastDay): 1 (meaning we want to include the last day in the calculation).

1. Convert Timestamps to Dates:

   Start Date: 1514329200 → December 27, 2017, 00:00:00
   End Date: 1538258400 → September 30, 2018, 00:00:00

Both dates are precisely at midnight (00:00:00), so there's no partial-day issue to account for here.
2. Time Difference Between the Dates:

The difference between the two timestamps is calculated as follows:
Difference=1538258400−1514329200=23929200 seconds

Now, we convert the difference in seconds to days:
Days=23929200/(60×60×24)=276,9583333333333 so 277 days if using ceil()

Without including the last day, this would result in 277 days.
3. Include the Last Day:

Since we passed LastDay = 1, we want to include the last day in the count. To do this, we add 1 to the total number of days:
Total Days (Including Last Day)=277+1=278 days

4. Summary of the Calculation:

   Start Date: December 27, 2017, 00:00:00
   End Date: September 30, 2018, 00:00:00
   Difference in Days: 277 days (excluding the end date)
   Including the Last Day: We add 1 day to account for September 30, 2018.

Final Result:

The total number of days between 2017-12-27 (inclusive) and 2018-09-30 (inclusive) is 278 days.

Let's calculate the number of days between December 27, 2017 and September 30, 2018, using a calendar-based method by considering the days in each month:
Start Date: December 27, 2017
End Date: September 30, 2018

We'll break it down month by month.

1. December 2017:
   Start Date: December 27, 2017
   December has 31 days, so the number of days left in December from the 27th to the 31st is:
   31−27+1=5 days

2. January 2018: January has 31 days.

3. February 2018: February 2018 is not a leap year, so it has 28 days.

4. March 2018: March has 31 days.

5. April 2018: April has 30 days.

6. May 2018: May has 31 days.

7. June 2018: June has 30 days.

8. July 2018: July has 31 days.

9. August 2018: August has 31 days.

10. September 2018:
    End Date: September 30, 2018
    From September 1st to September 30th (inclusive): 30 days

Total Days Calculation:

Now, let's sum up all the days:
5(December)+31(January)+28(February)+31(March)+30(April)+31(May)+30(June)+31(July)+31(August)+30(September)
5+31+28+31+30+31+30+31+31+30=278 days

Final Result:
The total number of days between December 27, 2017 and September 30, 2018, inclusive of both start and end dates, is 278 days.

Could I move forward and change the code to use ceil() and also the <= operand as already requested in dev and then backport this on version 12 or any version you may suggest?

[josett225]

Hi @eldy

After another round of test, I discovered other issues using ceil() function.
2022-10-01 00:00:00 - 2023-12-31 00:00:00 (458) instead of 457
2022-10-01 00:00:00 - 2022-12-26 00:00:00 (88) instead of 87
Therefore I believe using either function floor() or ceil() is bringing several issues.
I rewrote then the function :
`
function num_between_day($timestampStart, $timestampEnd, $includeLastDay = 0)
{
// If start date is greater than end date, return 0
if ($timestampStart > $timestampEnd) {
return 0;
}
// Create DateTime objects from the timestamps
$startDate = (new DateTime())->setTimestamp($timestampStart);
$endDate = (new DateTime())->setTimestamp($timestampEnd);

// Calculate the difference between the two dates
$interval = $startDate->diff($endDate);

// Get the difference in days
$nbOfdays = $interval->days;

// If the last day should be included, add 1 to the result
if ($includeLastDay == 1) {
    $nbOfdays += 1;
}

return $nbOfdays;

}
`

Let me know your thoughts.

[eldy]

Situation is more complex than that:
Start Date: 1514329200 → December 27, 2017, 00:00:00 yes in local TZ => But it is Tuesday 26 December 2017 23:00:00 in GMT
End Date: 1538258400 → September 30, 2018, 00:00:00 yes in a local TZ => But it is Saturday 29 September 2018 22:00:00 in GMT
As you can see, the hour is not the same, because one is inside the sun hour period and the other one is not, this is why you get 276,9583333333333 days instead of 277 (1 hour is missing). You try to make a difference between 2 dates that are not in the same referentiel. Result will alwyas be wrong in such case.

This is why there is the comment of the function say: "date must be UTC date !"

Whatever is the use of this function, results will always be unpredictable if parameters are not UTC dates. In your example, you are not using UTC dates.

So for
BEGIN DATE FORMATTED 2017-12-27 00:00:00
END DATE FORMATTED 2018-09-30 00:00:00
You have start_date = 1514332800 and end_date = 1538265600 in UTC
And the result is as expected: 277 with lastday to 0 and 278 with lastday to 1

So trouble seems not into the function, but into the caller of function that does not use GMT date. All phpunit tests pass with success the function, in all different use case, so i doubt trouble is inside this function.

	// With same hours
	$date1 = dol_mktime(0, 0, 0, 1, 1, 2012, 'gmt');
	$date2 = dol_mktime(0, 0, 0, 1, 2, 2012, 'gmt');

	$result = num_between_day($date1, $date2, 1);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(2, $result);

	$result = num_between_day($date1, $date2, 0);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(1, $result);

	// With different hours
	$date1 = dol_mktime(0, 0, 0, 1, 1, 2012, 'gmt');
	$date2 = dol_mktime(12, 0, 0, 1, 2, 2012, 'gmt');

	$result = num_between_day($date1, $date2, 1);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(2, $result);

	$result = num_between_day($date1, $date2, 0);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(1, $result);

	// With different date before and after sunlight hour (day to change sunlight hour is 2014-03-30)
	$date1 = dol_mktime(0, 0, 0, 3, 28, 2014, true);
	$date2 = dol_mktime(0, 0, 0, 3, 31, 2014, true);

	$result = num_between_day($date1, $date2, 1);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(4, $result);

	$result = num_between_day($date1, $date2, 0);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(3, $result);

	$result = num_between_day(1514332800, 1538265600, 0);
	print __METHOD__." result=".$result."\n";
	$this->assertEquals(277, $result);

[josett225]

Well I will take a Doliprane before it is done by USA ;-)
Understood, I will look into it.

[eldy]

Well I will take a Doliprane before it is done by USA ;-) Understood, I will look into it.

The problem is surely here:
$fiscal_period_start = dol_get_first_day($date_temp['year'], $date_temp['mon'], false);
The param false says the date is in TZ of server so not in UTC, because we want the fiscal period according to the TZ of server.
And at other places, dates are also defined with getCurrentPeriodOfFiscalYear() that also returns dates according to TZ of server.
All those date are not clean to be used with num_between_day().

i added a param to solve the trouble around the function getCurrentPeriodOfFiscalYear() so it can return a value in UTC.

[josett225]

Hi @eldy

Many thanks for all the explanation. This is now crystal clear.
Before submitting another PR to fix ASSET module, could you please let me know what I can do?

• can I use dol_mktime(0, 0, 0, 1, 1, 2012, 'gmt') everywhere in ASSET module to remove the hour 12 and use gmt?
• can I use gmt eveywhere in any function call to avoid any issues?
• could you please also validate the if ($timestampStart <= $timestampEnd) (<= instead of <) modification as I need it to return value 1d here (if not I will need to add a if in asset on this line? Remember the issue is when the start date of the asset equals the end date of the fiscal year. The answer has to be 1d.
  $nb_days = min($nb_days_in_year, num_between_day($begin_date, $end_date, 1));
  

  dolibarr/htdocs/asset/class/asset.class.php

  Line 1113 in b3b8f52

  $nb_days = min($nb_days_in_year, num_between_day($begin_date, $end_date, 1));

• do I need to submit a new PR to change this operator in develop branch? And then submit a new one to backport in other branches if you agree?

Waiting your directions

[eldy]

It's difficult to may to say if you can use gmt everywhere. it depends only what is the date, what it represent and how we plan to use it and for what.
the only thing that is sure is that to use the num_between_days, date MUST be gmt dates to avoid offset that appear when using solar hours.

For the fix of the <= instead of <, you can try this, push the fix on the develop branch (this is a critical change that can have side effect), and we will see if CI says every thing is ok. If yes, i will validate it on develop.

[hregis]

@josett225 @eldy i don't understand this problem !

if ($timestampStart < $timestampEnd)

2012-01-01 between 2012-01-02
1 if lastday = 0,
2 if lastday = 1

why would you ($timestampStart <= $timestampEnd) ???

[hregis]

@josett225 create another function or specific module if you would your specific function !

[hregis]

@josett225 This function was done for a short time! here you give examples over a year! adds a function that calculates over a year or more!

[hregis]

@josett225 and have a good day ! ;-)

[hregis]

@josett225 and don't forget leap years (bisextile), that's probably the problem! ;-) Good luck !

[josett225]

Hi @eldy and @hregis
Thanks for your comments and feedback.

• GMT versus TZ
  I will clean Asset module to use GMT with hours set at 00:00:00 in another PR.
• < versus <=
  This has nothing to do with years(by the way its works very well in this case)
  As already explained the function has to return 1 when start date == end date if option lastday == 1. (And 0 if lastday == 0)
  This is the aim of this function by default.
  Not sure why I need to develop something new and what is the impact on any other codes.
  I already gave an example above but I am pleased to put it back again.
  In assets, if the day of starting the asset == the last day of FY then the function has to return 1 as this is 1 day of calculation asset.

2012-01-01 between 2012-01-01
0 if lastday = 0,
1 if lastday = 1

Therefore this is a bug in this function and we need to avoid to add extra code to compensate this issue.

Let me know if you need more information and enjoy your day.

[hregis]

@josett225 the function being made to calculate the number of days between two dates, when is it if the date is exactly the same? This is a philosophical question... can there be a last day? :-)

[hregis]

@josett225
https://www.php.net/manual/en/function.date-diff.php
https://www.php.net/manual/en/datetime.diff.php

[eldy]

As this may change behaviour, if we change something, it must be in develop. A PR was merged in develop so we can test for v21 if everything is ok and we can cancel this one.