Missing holiday methods in ProviderInterface
iquito opened this issue · 20 comments
In v2.5 Yasumi::create now returns an instance of ProviderInterface, but this interface is missing most methods needed to actually do anything with holidays:
- No
isHolidaymethod - No
getHolidayDatesmethod (that is the one that my code is using) - No
betweenmethod - No
getHolidayNamesmethod - No
whenIsmethod
These would seem like the necessary methods to get a list of holidays and check if a date is a holiday or what holidays are within a certain timeframe. ProviderInterface has no method at all to check for holidays - which is why my static analyzers report Method Yasumi\ProviderInterface::getHolidayDates does not exist, and anybody using static analyzers and Yasumi will get these kind of errors. The methods are currently still there in AbstractProvider, but according to ProviderInterface they are not necessary.
@iquito Thank you very much for your issue. Some other users have already reported the same and I am working on a fix for this.
Thanks for your effort! I didn't see an issue for this yet, and v2.5 just came out, so I thought I would mention it in case this wasn't known yet :-)
Sorry, indeed not, however a PR was created that fixed one of the missing methods you mentioned. In the meantime I started working on a fix in a different branch. (https://github.com/azuyalabs/yasumi/tree/tests-fixes)
It could make sense to use two interfaces - a "user-facing" one that implements the methods to check for holidays, dates, etc. (representing a very stable set of methods that almost never need to change), and a "provider-facing" one that implements methods like "initialize" or "getHoliday", which might be important for the provider to work but seem less useful for somebody just using the library. Then you can just type hint the user-facing one for Yasumi::create but still internally check that both interfaces are implemented.
The AbstractProvider class is kind of a messy class, I admit. These two interface could certainly be identified.
Our PHPStan CI also reports now errors:
------ ------------------------------------------------------------------
Line src/Utility/Calendar.php
------ ------------------------------------------------------------------
26 Call to an undefined method Yasumi\ProviderInterface::between().
------ ------------------------------------------------------------------
------ -------------------------------------------------------------------------
Line src/Utility/SprykerHolidays.php
------ -------------------------------------------------------------------------
20 Call to private method christmasEve() of class Yasumi\Provider\Germany.
21 Call to private method newYearsEve() of class Yasumi\Provider\Germany.
------ -------------------------------------------------------------------------
We will pin down back to 2.4 then for now.
It depends on what methods you want to offer and which ones are internal-adjacent - would also make it easier to change the internal-adjacent ones and communicate to users what methods they should use. Then you could add @deprecated (and/or trigger a E_USER_DEPRECATED) to the methods that you want to remove (or make private/protected), to avoid immediate BC-breaks, as using deprecated methods will still show up in static analyzers.
@iquito All methods have been created with the intent to be public. I don't see any specific reason to make some of them protected/private. With that background, the ProviderInterface should have had all those methods defined, which unfortunately was not the case with the 2.5 release.
BTW, I've made fixes in the develop branch, so if you're interested you may want to test with that branch.
I mention it more because I don't know the exact motivations behind the methods, so I don't want to presume, and sometimes some methods exist but are more internal or legacy. The interface looks fine to me in the develop branch, although I would hide the initialize-method for the user (or you could mark it as @internal for now, so nobody calls it explicitly in their code), while all the other methods seem okay to expose.
Thanks for checking the fixes. Good idea to mark the initialize method to @internal.
@iquito 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.
I can report that PHPStan now is green again with develop branch :) For us at least, with our usage.
I would recommend to update "phpstan/phpstan": "^0.12", to "^1.2" now
Also level 7+ is recommended to have a good check for your API, 8 is nullable safety and nice to have, but 7 will help a lot with interface contracts and types as well.
@dereuromark Thanks! I had some issues with phpstan 1.2 before and had to revert to 0.12. I'll check that. Also checking for level 7 as well :)
What kind of issues? Maybe I have an idea. Had to fix myself hundreds of repos here :)
I don't recall specifically but got many false negatives. Anyway, upgraded to 1.4 and all seem good :)
The dev-develop branch fixes my phpstan errors as well.
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.
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.
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.