Enhancement request: replace $year on creation with $dateFrom and $dateTo

[Milamber33]

Currently when creating an instance of a holiday provider, you just pass it a year value and it will find all the holidays in that year for that region. This can be very inefficient to work with as you may want only the holidays within a short time period (yes this is easy enough to filter), and/or you may want to find out holidays that occur both sides of new years, which has to be handled manually with external code. Yasumi doesn't even return an error or warning if you check against a date outside the range the provider instance handles. So currently you have to vet the dates you're working with, and if they cross a year boundary you have to create TWO instances and check against both of them, rather than just passing the dates in and having the holiday provider do all the checking like it should.

To solve this I propose that the current $year parameter should be replaced with $dateFrom and $dateTo parameters, and have the holiday provider return all holidays within this date range, no matter how small or large it is.

I realise this is a breaking change that would require extensive rewriting of every holiday provider, but I think it's something that should be worked towards in a future major release.

[stelgenhof]

Thanks! Yes, the API was built on the basis that most scenarios would be based around that all annual holidays of a country are needed. I agree that some situations you only are interested in a subset of these, and that's where the filters come in :)

Having to create multiple instances to retrieve holidays spanning more than 1 year, is not an elegant solution indeed. So using a date range like you suggested would definitely be a good solution.

As far as I can see now, it for sure would be an extensive exercise as it would affect various areas (not only the date filtering).

Cheers! Sacha

[livingBEEF]

One could do something like this

foreach ($years as $year) {
    $this->year = $year;
    $this->initialize();
}

in AbstractProvider::__construct, additional filtering could then be done in addHolyday. If the target were be enable date ranges no greater than around year (keeping irregular holidays like Easter in mind), the required changes might be somewhat manageable, as $this->holydays[$shortName] would still mostly work as expected.

[stelgenhof]

@livingBEEF That is an option, however I am also concerned about performance. If a date range or multiple years are a possibility, then theoretically one could take a range from the year 1000 until 9999.

Some holiday providers use extensive calculations (e.g. Japan). In that case, calculation would take a long time. It may not be a common / practical scenario, but nevertheless possible.

[Milamber33]

Anyone who asks for all holidays between 1000 and 9999 deserves exactly what they get IMO. The job of an API like this is not to place any restrictions on usage, but to cater for all usage scenarios and let developers using it decide how to filter requests to sane date ranges.

[livingBEEF]

I think the most glaring issue is things like Nov 26, 2018 - Jan 6, 2019 (one calendar square) needing multiple providers. And allowing for range greater than one year would require more extensive changes anyway, since the AbstractProvider::$holidays array is indexed by holiday's shortName which doesn't change across years, which obviously wouldn't work with multiple years - all logic related to that would have to be scrapped. Adding the restriction of maximum one year date range would most likely require significantly less extensive changes and could be fully backwards compatible.

[Milamber33]

You're right about this being my main issue personally, and probably also the main issue that a lot of other people will face, so the simple extension you're suggesting is certainly a good short term fix, but in the long run I think the more extensive changes (possibly converting $this->holidays[$shortName] into an array rather than a single value) will make for a much better API. You used the example of a single calendar square needing multiple provider instances, what about if you're displaying a whole year calendar? There's still only a couple of days' overflow either side, but you would need three providers and there would be two instances of a couple of holidays (depending on how far you overflow).

Specifying a date range and having it be internally handled properly should get rid of at least some wasted processing of holidays that you don't need to worry about (i.e. in each calculation function have $minPossible and $maxPossible as fixed values, only do the actual calculation if the requested date range overlaps with this range), and it would make the API far more flexible. Like I said, I know it's a lot of work and involves breaking changes, and if no-one feels like putting in the spadework to actually do it I'll understand, but I figured there's no harm in asking.

[stelgenhof]

Good discussion! :)

@Milamber33 Agree that if users use that kind of date range, they get what they ask for. Just wanted to lay out the possible consequences if the year parameter would be replaced by a date range.

My personal preference would be to implement a date range - as suggested - rather than trying to 'extend' the current year so an issue like around the year switch could be overcome. It would indeed require an extensive effort, but would prefer do it right rather than an intermediate solution.

Cheers! Sacha

[github-actions[bot]]

This issue has been open 60 days with no activity. Please remove the stale label or comment, or this will be closed in 10 days.