kylekatarnls / business-time

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

Laravel issues #43

Closed mikemand closed 3 years ago

mikemand commented 3 years ago

I'm not entirely sure when this started happening, but I noticed today that trying to check if the business is closed (or open, or is a holiday) using an instance of Laravel's Date facade (which is configured to use CarbonImmutable) is not working. However, if I use the facade to call Date::isClosed($myInstance) (or the other methods) it works fine. Here is a simplified example of my setup:

Package versions laravel/framework: 8.20.1 nesbot/carbon: 2.43.0 spatie/opening-hours: 2.10.1 cmixin/business-day: 1.11.0 cmixin/business-time: 1.11.0

Opening Hours

Illuminate\Support\Facades\Date::use(Carbon\CarbonImmutable::class);
Illuminate\Support\Facades\Date::setOpeningHours([
    "monday" => [],
    "tuesday" => [],
    "wednesday" => [
        "hours" => [
            "12:00-21:00",
        ],
    ],
    "thursday" => [
        "hours" => [
            "12:00-21:00",
        ],
    ],
    "friday" => [
        "hours" => [
            "12:00-21:00",
        ],
    ],
    "saturday" => [
        "hours" => [
            "12:00-21:00",
        ],
    ],
    "sunday" => [
        "hours" => [
            "12:00-18:00",
        ],
    ],
    "exceptions" => [
        "2021-04-04" => [
            "data" => "Easter",
            "hours" => [],
        ],
        "2021-07-04" => [
            "data" => "Independence Day",
            "hours" => [],
        ],
        "2021-11-25" => [
            "data" => "Thanksgiving",
            "hours" => [],
        ],
        "2021-12-25" => [
            "data" => "Christmas",
            "hours" => [],
        ],
    ],
]);

My date object

$date = \Illuminate\Support\Facades\Date::create(2021, 1, 2, 5, 0, 0, 'America/Denver');

When I check $date->isOpen() it says false, which is wrong. If I use the facade, it produces the correct result: \Illuminate\Support\Facades\Date::isOpen($date) => true. In both cases, if I get the current day's opening hours, it works fine: $date->getCurrentDayOpeningHours() or \Illuminate\Support\Facades\Date::getCurrentDayOpeningHours($date) returns:

Spatie\OpeningHours\OpeningHoursForDay {#2300 ▼
    #openingHours: array:1 [▼
      0 => Spatie\OpeningHours\TimeRange {#2437 ▼
        #start: Spatie\OpeningHours\Time {#2304 ▼
          #hours: 12
          #minutes: 0
          #data: null
        }
        #end: Spatie\OpeningHours\Time {#2301 ▼
          #hours: 21
          #minutes: 0
          #data: null
        }
        #data: null
      }
    ]
    #data: null
  }

Here are the tests I used dd on:

[
    'isOpen' => \Illuminate\Support\Facades\Date::isOpen($date),
    'isClosed' => \Illuminate\Support\Facades\Date::isClosed($date),
    'isHoliday' => \Illuminate\Support\Facades\Date::isHoliday($date),
    'isBusinessOpen' => \Illuminate\Support\Facades\Date::isBusinessOpen($date),
    'isBusinessClosed' => \Illuminate\Support\Facades\Date::isBusinessClosed($date),
    'getCurrentDayOpeningHours' => \Illuminate\Support\Facades\Date::getCurrentDayOpeningHours($date),
],
[
    'isOpen' => $date->isOpen(),
    'isClosed' => $date->isClosed(),
    'isHoliday' => $date->isHoliday(),
    'isBusinessOpen' => $date->isBusinessOpen(),
    'isBusinessClosed' => $date->isBusinessClosed(),
    'getCurrentDayOpeningHours' => $date->getCurrentDayOpeningHours(),
]

Results:

[
   "isOpen" => true,
   "isClosed" => false,
   "isHoliday" => false,
   "isBusinessOpen" => true,
   "isBusinessClosed" => false,
   "getCurrentDayOpeningHours" => Spatie\OpeningHours\OpeningHoursForDay, // See above
],
[
   "isOpen" => false,
   "isClosed" => true,
   "isHoliday" => false,
   "isBusinessOpen" => false,
   "isBusinessClosed" => true,
   "getCurrentDayOpeningHours" => Spatie\OpeningHours\OpeningHoursForDay, // See above
 ],

Even testing this with just basic CarbonImmutable instances has the same result, so maybe this isn't just a Laravel thing?

kylekatarnls commented 3 years ago

I don't get why it should be open Saturday at 5 AM as your config for Saturday is open only for 12:00-21:00.

kylekatarnls commented 3 years ago

Note that Date::isClosed() and Date::isOpen() does not take any argument, they just gives you the state for now when called statically, any date passed in should make no difference.

mikemand commented 3 years ago

Hmm. I thought that isOpen was true if there are any hours for that day? Is there a different method that I missed to check if the business will be open at any point during that day, which also takes into account any exceptions (either exceptional open hours or holidays)?

If not, I guess I can just count the result of getCurrentDayOpeningHours().

mikemand commented 3 years ago

I saw isOpenOn, but it seems kind of counterintuitive to check if the date is open on the same date: $date->isOpenOn($date->format('Y-m-d')).

kylekatarnls commented 3 years ago

No, what would be really counter-intuitive would be to ignore the hours you set in setOpeningHours when calling isOpen which is the very point of the library.

The first mistake here is the name you gave to your variable. Instances of Carbon (which extends DateTime) are not day-long dates, they handle both date and time, they are microsecond-precise moments.

So you should name your variable according to what it really contains, and you would not make such assumptions then: $moment->isOpen() or $5AMInDenver->isOpen().

And last, isOpenOn() makes way more sense to be called statically if you defined the openning hours statically:

Date::isOpenOn($date->format('Y-m-d'))

Those basic usages are well covered by the first examples of the documentation: https://github.com/kylekatarnls/business-time#features