azuyalabs / yasumi

The easy PHP Library for calculating holidays
https://www.yasumi.dev
Other
1.05k stars 155 forks source link

Possibility to Set default timezone #132

Closed IceShack closed 5 years ago

IceShack commented 5 years ago

All of my dates I'm using are in UTC, if I want to compare them with the holidays I always need to change the timezone. So I think setting a default timezone either globally Yasumi::defaultTimezone or during creation as an extra parameter Yasumi::create(Germany::class, 2017, 'de_DE', 'UTC') would be very helpful. As long as the default timezone is not set, it uses the timezone provided by the initialize method. I'll create a PR once this is approved. :-D

stelgenhof commented 5 years ago

The reason for the timezone not being configurable is that holidays should be calculated for the timezone (i.e. country) they are taking place in. If what you are suggesting would be done, then a strange situation occurs that for example Christmas may have already started (or finished) from the perspective of the other timezone.

Although that technically is correct, semantically has little meaning. You can consider for example New Year (Change at 00:00). If I would set New Year Midnight in Germany to a different timezone (for example Asia/Tokyo, which is UTC+9 hours), then the results would be that midnight has taken place already, where in reality it is still 9 hours before (UTC).

This is the primary reason, why a timezone isn't configurable, and any time conversation should be taken place outside this API.

Hope it makes sense :)

Cheers! Sacha

IceShack commented 5 years ago

I can't be the only one who stores dates as UTC in the database and relies on the frontend to decide which timezone the users in. As I said before all I want is to make the timezone decision myself. As of now that is impossible, because everything happens in the initialize method and I can't change that value by Inheritance or even Reflection. PS: I don't want to compare Germany New Years Eve with the Japanese one ;-)

stelgenhof commented 5 years ago

Sure, I store my dates in UTC as well. I am not familiar with your business case, so let me show you what I do in one of my projects.

I am sending a message to users when they login and that day is a public holiday. In this example I am using Japan's Mountain which takes place on August the 11th.

// User logged in on 2018-08-10 at 21:34:14 UTC (This is 2018-08-11 06:34:14 in Asia/Tokyo)
$login_date = Carbon::parse('2018-08-10 21:34:14', 'UTC'); // Date comes from the database

$japan = Yasumi::create('Japan', 2018);
$mountainDay = $holidays->getHoliday('mountainDay');

if($login_date->greaterThan($mountainDay)) {
    // Send the user an e-mail
}

The above example is a bit simplified of course and I am using the Carbon library for parsing and handling dates. In this case Carbon is taking care of the timezone conversions.

As you can see, there is no need for me to tell Yasumi what timezone I am using.

If you have an example of yourself, that would be great. So I can understand better your scenario/case.

Cheers! Sacha

IceShack commented 5 years ago

Sorry I should have added my business case earlier: For calculating vacation days, I need to know which days are holidays in date ranges. This is of course a simplified version of my code and should only represent an example for this issue:

use Carbon\Carbon;
use Yasumi\Provider\Germany\NorthRhineWestphalia;
use Yasumi\Yasumi;

// These dates get returned from the ORM so I can't just change the timezone
$dateStart = Carbon::parse('2019-01-01 00:00:00', 'UTC');
$dateEnd = Carbon::parse('2019-01-04 23:59:59, 'UTC');

// count weekdays in range
$dayCount = $dateStart->diffInWeekdays($dateEnd, false);

// First holiday would be newYearsDay (2019-01-01T00:00:00+01:00)
$provider = Yasumi::create(NorthRhineWestphalia::class, 2019, 'de_DE');
foreach ($provider as $holiday) {
    $carbonHoliday = Carbon::instance($holiday);
    // for new years this would return false as newYearsDay converted to UTC would be 2018-12-31T23:00:00+00:00
    if ($carbonHoliday->between($dateStart, $dateEnd)) {
        $dayCount--;
    }
}
echo $dayCount;

This would return 4 instead of the correct 3 workdays in range.

stelgenhof commented 5 years ago

Thanks for the example. Very helpful. So if I understand correctly, the expected number of vacation days is 4, however your example only returns 3.

The reason is that the time of your $dateEnd is '00:00:00', which means 'start of the day', but isn't a full completed day. When doing a date comparison, this date will not be included, because also the date to check against has a time value of 00:00:00.

If you add this after the $dateEnd..... line:

$dateEnd->endOfDay();

you will get 4 vacation days.

IceShack commented 5 years ago

No, the expected number of vacation days should be 3 as '2019-01-01' is new years day and "between" $dateStart and $dateEnd. But it returns 4, because of comparing 2 different timezones to each other. It has nothing to do with $dateEnd->endOfDay(); This code actually works correctly, if all dates use the same timezone.

stelgenhof commented 5 years ago

Sorry then misunderstood you first example. However, if I run your original example again, I get 3 days as output. (as you expected).

IceShack commented 5 years ago

Yeah should have tested the example. So the end date actually needs to be endOfDay. I fixed the example, which still doesn't change my issue though.

stelgenhof commented 5 years ago

Today I've spent more time understanding your example and did some experimentation.

Still I am confused what your business case is. Your initial date range returns 3 weekdays (January 1st, 2nd and 3rd). The second part of your example where you use Yasumi, you reduce the number of days in case the day in your day range happens to be a holiday. This part is I believe not needed, as for 2019 January 1st falls on a weekday. Should a holiday fall in the weekend, it isn't counted either because it is not a weekday.

Apart from your proposal regarding the timezone, you would not need to this check with Yasumi at all.

Also, I've tried many varieties with timezone conversions, and you will always have an offset of 1 hour. Keep in mind that Yasumi returns only a single point in time (the start time of that event), and not a date range. Just checking a DateTime instance against a date range with different timezones, is too simplified. If you do, you also then need to check the end time of the event.

Your proposal (as I understand) would basically mean you do a force override of the local timezone of the Holiday instance. This might solve your problem/business case, but would result in other issues when using the instance for the local timezone again. Feels to me that you would then swap out the timezone depending on the circumstance, which is in my opinion unmanageable.

Happy to hear your feedback/thoughts.

Cheers! Sacha

IceShack commented 5 years ago

Hi Sacha,

I try it from a business perspective now with no code. :wink:

Employee John has 25 vacation days each year. John plans a vacation from 2019-04-15 to 2019-04-28 which are 14 days in total. But he would only use 8 of his vacation days, because the 19th and 22nd are public holidays and he only works from monday to friday. John also went on a vacation from the 2019-01-01 to 2019-01-04 which are 4 days in total, but the 1st is a public holiday, so he only used 3 vacation days. So he has 14 vacation days left.

The first vacation gets calculated correctly and my code returns the proper amount of vacation days used, because the public holidays are not on the first or last day of the vacation. The second vacation gets calculated with 4 days instead of the actual 3 vacation days used. This is because I compare the Holiday (with the 'Europe/Berlin' timezone) against the vacation (with the 'UTC' timezone). The problem is that the holiday date 2019-01-01T00:00:00+01:00 is not inside the vacation date range of 2019-01-01T00:00:00+00:00 and 2019-01-04T23:59:59+00:00.

All I do in the code is check if any of the holidays are inside of the vacation date range and reduce the number of vacation days used for the vacation. In my example I didn't add the logic which checks if its a weekday or not to simplify the example.

I hope this helps to better understand my problem. :smile:

stelgenhof commented 5 years ago

Hi Moritz,

Thank you very much for providing your business case. It indeed helped me much better understanding what you want to achieve.

As said, DateTime instances - and also Yasumi's Holidays - are a single point in time and not a date range. For your example, it is irrelevant what the start time of a holiday is, however since you get the dates from an external source (with the time element), you also then need to provide an end time. That would make it a bit more complex when checking whether a date falls into a date range or not.

As a matter of fact, Yasumi's API has a function that will help with your case: isWorkingDay. This function will check if the given date is a working day or not. It actually ignores the time element of the date and has logic for the weekend days as well (Supporting all countries).

Please have a look at this gist I've created for you: https://gist.github.com/stelgenhof/14a14e1ab2d3123cf81aaff866ecda49

Taking a bit of a different approach and using the isWorkingDay function, It calculates perfectly all the remaining days (and consumed days) in your business case. The gist may look a bit complicated but contains just some helper functions to format the output nicely in your shell.

Cheers! Sacha

IceShack commented 5 years ago

Hi Sacha,

Thank you for the gist! That works for me.

stelgenhof commented 5 years ago

@IceShack Thanks! Good to hear it works for you. I'll go ahead and close this issue and the PR.

Cheers! Sacha