yasumi icon indicating copy to clipboard operation
yasumi copied to clipboard

Certain holidays functions are now private

Open MatthiasKuehneEllerhold opened this issue 3 years ago • 7 comments

We're using your library in order to calculate the amount of working days from now to a specific date in the future.

For this we need a relativly accurate decision if a day is a working day or not. The Germany-Provider does this pretty good, but we have a company policy that certains days (christmas eve and new years eve) are a holiday too. So Ive used your Germany-Provider and manually added both these dates using the ChristianHolidays-Provider via the methods $germany->addHoliday(christmasEve(..)) (same for NYE).

In version 2.5 youve made these methods private - why?

For the time being Ive copied the content of methods into my code - but thats not the way its supposed to be.

Could you please do one of the following things?

Solution 1: make these kinds of functions public If you'd made these kinds of functons public other providers / callers could re-use them and reduce the amount of copy-paste in the code. So I could add these holidays after the creation of the "base" provider Germany (or Saxony).

Solution 2: make these kinds of function protected The "correct" way in terms of hierarchy would be to create a new provider inheriting from Saxony and add these holidays in the constructor. With christmasEve() being private to Germany I cant do that!

I'd have to do this for every each Region-provider (e. g. a second subclass for Bavaria) ... thats why I prefer the first proposed solution!

If you want I could provide PRs for both solutions, I'd prefer the first one though ;-)

MatthiasKuehneEllerhold avatar Jan 31 '22 13:01 MatthiasKuehneEllerhold

@MatthiasKuehneEllerhold Thank you very much for this issue. The reason for the reduced visibility is that these methods shouldn't have been exposed in the first place as these are internal methods.

The correct way is indeed extending the provider class as shown in this example: https://www.yasumi.dev/docs/cookbook/custom_provider/

For that, these method should indeed be protected and not private. My apologies, I may have overlooked that.

stelgenhof avatar Feb 02 '22 11:02 stelgenhof

So one of the question my app has to answer is "When is holiday XY?" In <2.5 I could use these method like christmasEve() for certain holidays. For other holidays I had to know the magic number (well string key) of it to use getHoliday() or whenIs().

I'd like to propose that these magic number (string keys) are moved to public constants in their classes so we can query the provider with something like $provider->getHoliday(Germany::CHRISTMAS_EVE) instead of using the internal magic number (string key) 'christmasEve'.

MatthiasKuehneEllerhold avatar Feb 04 '22 08:02 MatthiasKuehneEllerhold

@MatthiasKuehneEllerhold The develop branch contains all recent bugfixes. You may want to check it out in your project if you like. I am preparing a new release soon addressing the latest identified issues.

stelgenhof avatar Feb 04 '22 13:02 stelgenhof

Since this issue has not had any activity within the last 90 days, I have marked it as stale. I will close it if no further activity occurs within the next 10 days.

github-actions[bot] avatar May 06 '22 00:05 github-actions[bot]

My custom provider adds Christmas Eve the way it is shown in the official docs

class MyProvider extends NorthRhineWestphalia
    public function initialize(): void
    {
        parent::initialize();

        $this->addHoliday($this->christmasEve($this->year, $this->timezone, $this->locale));
    }
}

It stopped working with version 2.5. The solution for me was to manually use the trait ChristianHolidays

So this works:

use Yasumi\Provider\ChristianHolidays;

class MyProvider extends NorthRhineWestphalia

    use ChristianHolidays;

    public function initialize(): void
    {
        parent::initialize();

        $this->addHoliday($this->christmasEve($this->year, $this->timezone, $this->locale));
    }
}

To make newYearsEve work, you have to use the trait CommonHolidays in this way.

raumi75 avatar Jun 21 '22 10:06 raumi75

@raumi75 Have you tried your solution with the dev branch? The develop branch contains all recent bugfixes. You may want to check it out in your project if you like.

stelgenhof avatar Jun 21 '22 14:06 stelgenhof

Since this issue has not had any activity within the last 90 days, I have marked it as stale. I will close it if no further activity occurs within the next 10 days.

github-actions[bot] avatar Sep 20 '22 00:09 github-actions[bot]

Since this issue has not had any activity within the last 90 days, I have marked it as stale. I will close it if no further activity occurs within the next 10 days.

github-actions[bot] avatar Dec 22 '22 00:12 github-actions[bot]

Since this issue has not had any activity within the last 90 days, I have marked it as stale. I will close it if no further activity occurs within the next 10 days.

github-actions[bot] avatar Mar 26 '23 00:03 github-actions[bot]

Closing (resolved in release 2.6.0)

stelgenhof avatar Apr 26 '23 16:04 stelgenhof