kylekatarnls / business-time

Carbon mixin to handle business days and opening hours
MIT License
296 stars 14 forks source link

Inconsistent behaviour with Laravel and config file #45

Closed jryd closed 3 years ago

jryd commented 3 years ago

Today is a public holiday in Queensland, Australia (Anzac Day). I was alerted to an issue where our application thought that we were open today when we were not.

We have a config/carbon.php file that looks like this:

<?php

return [
    'opening-hours' => [
        'monday' => [env('BUSINESS_OPENING_HOUR', '07:00') . '-' . env('BUSINESS_CLOSING_HOUR', '16:30')],
        'tuesday' => [env('BUSINESS_OPENING_HOUR', '07:00') . '-' . env('BUSINESS_CLOSING_HOUR', '16:30')],
        'wednesday' => [env('BUSINESS_OPENING_HOUR', '07:00') . '-' . env('BUSINESS_CLOSING_HOUR', '16:30')],
        'thursday' => [env('BUSINESS_OPENING_HOUR', '07:00') . '-' . env('BUSINESS_CLOSING_HOUR', '16:30')],
        'friday' => [env('BUSINESS_OPENING_HOUR', '07:00') . '-' . env('BUSINESS_CLOSING_HOUR', '16:30')],
        'saturday' => [],
        'sunday' => [],
        'exceptions' => [],
    ],
    'holidaysAreClosed' => true,
    'holidays' => [
        'region' => 'au-qld',
    ],
];

From my understand of the README.md, this should make Business Time globally enabled.

Yet, this is my output for a range of tests:

$date = Carbon::now('Australia/Brisbane');
echo $date->toDateTimeString()."\n";

var_dump($date->isOpen());
var_dump($date->isBusinessOpen()); // use isBusinessOpen to consider holidays as closed all day long
echo $date->nextClose()."\n";
return;

2021-04-26 09:54:34
bool(true)
bool(true)
2021-04-26 16:30:00

However, I get the correct output if I do this:

BusinessTime::enable(Carbon::class, [
    'monday' => ['09:00-17:00'],
    'tuesday' => ['09:00-17:00'],
    'wednesday' => ['09:00-17:00'],
    'thursday' => ['09:00-17:00'],
    'friday' => ['09:00-17:00'],
    'saturday' => [],
    'sunday' => [],
    'holidays' => [
        'region' => 'au-qld', // Load the official list of holidays from Australia - QLD
    ],
]);
$date = Carbon::now('Australia/Brisbane');
echo $date->toDateTimeString()."\n";

var_dump($date->isOpen());
var_dump($date->isBusinessOpen()); // use isBusinessOpen to consider holidays as closed all day long
echo $date->nextClose()."\n";

2021-04-26 09:56:28
bool(true)
bool(false)
2021-04-26 17:00:00

Is there an issue where the config file is not automatically making BusinessTime globally available?

kylekatarnls commented 3 years ago

Hello,

I can't verify env('BUSINESS_OPENING_HOUR', '07:00') and env('BUSINESS_CLOSING_HOUR', '16:30') are properly set. And also there is a difference for the holidaysAreClosed option.

As we have multiple differences between the working and non-working situations, it lets a wide range of possible root causes. Could you first narrow this by testing the exact same config in ::enable() and in config/carbon.php?

kylekatarnls commented 3 years ago

Here is my test:

BusinessTime::enable(Carbon::class, [
    'monday' => ['09:00-17:00'],
    'tuesday' => ['09:00-17:00'],
    'wednesday' => ['09:00-17:00'],
    'thursday' => ['09:00-17:00'],
    'friday' => ['09:00-17:00'],
    'saturday' => [],
    'sunday' => [],
    'holidaysAreClosed' => true,
    'holidays' => [
        'region' => 'au-qld',
    ],
]);

var_dump(Carbon::parse('2021-04-26 10:00')->isOpen());
var_dump(Carbon::parse('2021-04-26 10:00')->isBusinessOpen());

I got false for both (which is the expected behavior), substitute of 04-26 for 04-25 is OK.

I'm checking the version with the config file.

jryd commented 3 years ago

Hi @kylekatarnls,

Thanks for looking into this.

I've adjsuted my config file to:

<?php

return [
    'opening-hours' => [
        'monday' => ['09:00-17:00'],
        'tuesday' => ['09:00-17:00'],
        'wednesday' => ['09:00-17:00'],
        'thursday' => ['09:00-17:00'],
        'friday' => ['09:00-17:00'],
        'saturday' => [],
        'sunday' => [],
        'exceptions' => [],
    ],
    'holidays' => [
        'region' => 'au-qld',
    ],
];

Using your example:

var_dump(Carbon::parse('2021-04-26 10:00', 'Australia/Brisbane')->isOpen());
var_dump(Carbon::parse('2021-04-26 10:00', 'Australia/Brisbane')->isBusinessOpen());

bool(true)
bool(true)
jryd commented 3 years ago
BusinessTime::enable(Carbon::class, [
    'monday' => ['09:00-17:00'],
    'tuesday' => ['09:00-17:00'],
    'wednesday' => ['09:00-17:00'],
    'thursday' => ['09:00-17:00'],
    'friday' => ['09:00-17:00'],
    'saturday' => [],
    'sunday' => [],
    'holidaysAreClosed' => true,
    'holidays' => [
        'region' => 'au-qld',
    ],
]);

var_dump(Carbon::parse('2021-04-26 10:00')->isOpen());
var_dump(Carbon::parse('2021-04-26 10:00')->isBusinessOpen());

I can confirm that your test works correctly for me when I enable BusinessTime inline (manually) with my code.

kylekatarnls commented 3 years ago

Can you check the Laravel provider is properly discovered and called, if it is, BusinessTime::enable() should actually be called: https://github.com/kylekatarnls/business-time/blob/master/src/BusinessTime/Laravel/ServiceProvider.php#L27

You also can check what are the classes enabled and the config array at this point to see if it maps the proper settings.

jryd commented 3 years ago

Hi Kyle,

Using some dd() statements, I've verified that the config is correct and that the provider is definitely running.

However the output from our tests above is still incorrect and when I ask the package for all the 2021 Holidays, I get an empty array...

Any ideas?

kylekatarnls commented 3 years ago

Hello, sorry for the delay.

I found out the issue in the way we load holidays for each class (Carbon\Carbon, Carbon\CarbonImmutable and Illuminate\Support\Carbon). Because it change the value of a variable reused in the loop, the region was set only for Carbon\Carbon and not for other classes.

It's fixed in 1.11.1, please try to update and you should now be able to set the region right from the config file of Laravel.