azuyalabs / yasumi

The easy PHP Library for calculating holidays
https://www.yasumi.dev
Other
1.05k stars 155 forks source link

Certain holidays functions are now private #276

Closed MatthiasKuehneEllerhold closed 1 year ago

MatthiasKuehneEllerhold commented 2 years ago

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 ;-)

stelgenhof commented 2 years ago

@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.

MatthiasKuehneEllerhold commented 2 years ago

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'.

stelgenhof commented 2 years ago

@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.

github-actions[bot] commented 2 years ago

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.

raumi75 commented 2 years ago

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.

stelgenhof commented 2 years ago

@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.

github-actions[bot] commented 2 years ago

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] commented 1 year ago

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] commented 1 year ago

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.

stelgenhof commented 1 year ago

Closing (resolved in release 2.6.0)