Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add data support for next open hours #89

Merged
merged 6 commits into from
Feb 13, 2019
Merged

Add data support for next open hours #89

merged 6 commits into from
Feb 13, 2019

Conversation

Sti3bas
Copy link
Contributor

@Sti3bas Sti3bas commented Feb 7, 2019

This PR fixes Call to a member function toDateTime() on array error when trying to call nextOpen method with:

'monday' => [
    [
        'hours' => '09:00-11:00',
        'data' => ['foobar'],
    ],
    '13:00-19:00',
],

@kylekatarnls kylekatarnls self-assigned this Feb 7, 2019
Copy link
Collaborator

@kylekatarnls kylekatarnls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular case is not documented, so I first need to check it's relevant to have it too and would create no conflict.

@@ -93,7 +93,11 @@ public function nextClose(Time $time)

protected function findNextOpenInWorkingHours(Time $time, TimeRange $timeRange)
{
if ($timeRange->containsTime($time) && next($timeRange) !== $timeRange) {
if ($timeRange->containsTime($time) && $next = next($timeRange) !== $timeRange) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to operators precedence, $next will always be a boolean (in this case, false), so ! $next instanceof Time will always be true. We are just always skipping a value here as far as I understand. I will try to create a unit test to confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.. the strangest thing is that if you just return false from this method all tests will still pass.

protected function findNextOpenInWorkingHours(Time $time, TimeRange $timeRange)
{
    return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we probably need more cases for our unit tests.

@kylekatarnls
Copy link
Collaborator

Thanks for your proposal. We will check it and get back in touch soon.

@kylekatarnls kylekatarnls merged commit f6d4a11 into spatie:master Feb 13, 2019
@kylekatarnls
Copy link
Collaborator

Thanks, with new changes, no this pass without error. Please try to update to dev-master to see if it solves your problem.

@kylekatarnls kylekatarnls mentioned this pull request Feb 13, 2019
@Sti3bas
Copy link
Contributor Author

Sti3bas commented Feb 13, 2019

Yes, dev-master solves the problem. Thank you!

@kylekatarnls
Copy link
Collaborator

Released in https://github.com/spatie/opening-hours/releases/tag/2.1.0-beta.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants