Accept DatePeriods as holidays

[sagikazarmark]

Holidays are usually not just one day (especially in summer). DatePeriods can be iterated through, so you can transform them into days internally.

But actually I wonder if holidays could be some kind of special day as well. For example a Holiday object. It could be used to ease creating different types of holidays (simple DateTime or DatePeriod).

If you like any of the above, I would be happy to provide a PR.

Big :+1: for this package. I was thinking about one for years now, but never had the time to make it.

[florianv]

Hello, thanks for your feedback.

I agree we shoud also accept DatePeriod instances.

We could have a Holidays class instead which wraps an array of DateTime or a DatePeriod and represents a collection of holiday.

Note that holidays are currently cached in a hashmap for fast lookup:

• https://github.com/florianv/business/blob/master/src/Business.php#L340
• https://github.com/florianv/business/blob/master/src/Business.php#L354

The same idea should be used in the Holidays class (by iterating the DatePeriod and storing the dates on the first access).

Also, the Holidays class must be serializable because the Business class is.

If you want to send a PR I will merge it happily :)

[sagikazarmark]

OK, then I will follow this roadmap:

• Implement DatePeriod supporting logic
• Implement Holiday class which wraps the previous

Do you prefer separate PRs?

[florianv]

Ok it sounds good to me :)

I would use Holidays (note the extra s) as class name instead of Holiday as it's a collection.

Do you prefer separate PRs?

No, it's fine with a single PR

[sagikazarmark]

Hashmap is a good idea. I can imagine two solutions related to intervals, I wonder which is better:

1. Iterate over interval when adding it to the collection and store each days in the hashmap. (Pro: faster I think, Con: more objects must be stored)
2. Store the intervals separated from the hashmap and check each intervals separately. (Pro: less objects stored, Con: slower because of checking each intervals)

[sagikazarmark]

Just realised I was using the long term: Actually we are talking about date PERIODS, not intervals

[sagikazarmark]

Should we accept DatePeriod directly? The reason why I think not: this way it is possible to pass in a period with a custom interval (which is not a day). So I think we should accept the following values as holidays:

1. A DateTime object: a single day
2. An array of two DateTime objects : transformed into a DatePeriod with a daily interval (OR a custom period implementation which accepts the two dates?)

[florianv]

Iterate over interval when adding it to the collection and store each days in the hashmap. (Pro: faster I think, Con: more objects must be stored)

Yes this is what I was thinking about.

this way it is possible to pass in a period with a custom interval (which is not a day)

We don't care because if the interval is not a day, some dates will be duplicated but only one will remain in the hash map.

[sagikazarmark]

some dates will be duplicated but only one will remain in the hash map.

Normally I would agree, but what if the interval is set to seconds? IMO that's a possible memory leak.

Another problem is that what we really need is an interval between two dates. Why should we force people then to pass in a DatePeriod object with an extra parameter in it's constructor which is not a required input from them?

I have something in my mind: We need to accept an initial configuration in the Holidys constructor, which is preferably an array of elements. If an element is a DateTime object, then it is taken as one day. If the element is an array of DateTime objects, it is considered to be an interval and processed internally. We could also add a public API for additional configuration: addDay and addInterval methods could accept the same parameters as described above.

What do you think?

[florianv]

If an element is a DateTime object, then it is taken as one day. If the element is an array of DateTime objects, it is considered to be an interval and processed internally

You can create a class DateTimePeriod to represent a period. In the Holidays constructor you can accept an array of DateTime and DateTimePeriod.

We could also add a public API for additional configuration: addDay and addInterval methods could accept the same parameters as described above.

I prefer Value Objects to be immutable when possible. With my above proposal you can inject arrays in the Holidays constructor so they are not needed.

[sagikazarmark]

That's a fine approach as well. One last concern: isn't DateTimePeriod name somewhat collapsing with DatePeriod?

[florianv]

That's a fine approach as well. One last concern: isn't DateTimePeriod name somewhat collapsing with DatePeriod?

I can't find a better name but feel free to use an other one