azuyalabs / yasumi

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

No easy way to get substituted holiday translations? #181

Closed zachleigh closed 4 years ago

zachleigh commented 4 years ago

I love this package and have been using it for years. I used to be able to simply do $holiday->translations['en'] to get translations, even for substituted holidays. Now it seems like I have to do something like this monstrosity:

if ($holiday instanceof SubstituteHoliday) {
    $original = $holiday->substitutedHoliday;

    $holiday_en = $original->translations['en'];
    $holiday_jp = $original->translations['ja'];

    $substitute_en = $holiday->substituteHolidayTranslations['en'];
    $substitute_jp = $holiday->substituteHolidayTranslations['ja'];

    $en = str_replace('{0}', $holiday_en, $substitute_en);
    $jp = str_replace('{0}', $holiday_jp, $substitute_jp);
} else {
    $en = $holiday->translations['en'];
    $jp = $holiday->translations['ja'];
}

Is there no better way? Couldn't find it in the docs..

c960657 commented 4 years ago

I suggest adding an optional argument for $holiday->getName() that allows you to override the provider's displayLanguage.

This would allow you to do $holiday->getName('en') and $holiday->getName('ja').

I'll be happy to create a PR for this.

stelgenhof commented 4 years ago

@zachleigh Indeed this isn't really friendly. @c960657 Pre v2.2.0 the substituted holiday would return all the available translations, whereas v2.2.0 would simply return blank if you use the translations attribute.

Pre v2.2.0

<?php
require 'vendor/autoload.php';

$holidays = Yasumi\Yasumi::create('Japan', 2019);
$childrensDay = $holidays->getHoliday('substituteHoliday:childrensDay');
print_r($childrensDay->translations);

would return:

Array
(
    [en_US] => Children's Day Observed
    [ja_JP] => 振替休日 (こどもの日)
)

v2.2.0 would return an empty array.

I think it is better if the behavior stays the same (without any override parameter).

c960657 commented 4 years ago

v2.2.0 introduced locale fallback, so we don't have the en_US and ja_JP entries anymore, just en and ja. Preserving the previous behaviour would be difficult.

In general, it is hard to preserve BC for public instance variables.

It would not be difficult to populate translations for substitute holidays, assuming we just use the en/ja keys. I suggest we do this for BC, but that we deprecate such access. Instead I suggest that we add the optional argument for getName() as mentioned, and possibly a new accessor, getTranslations() that returns all available translations.

c960657 commented 4 years ago

It would not be difficult to populate translations for substitute holidays, …

On second thought, it is not completely trivial.

E.g. New Year's Day has just one English translation, so the translations array would contain just en (in addition to the non-English locales). The substitute pattern has multiple English patterns (en, en_US, en_CA etc.), so the translations array for substituteHoliday:newYearsDay would need to be populated with entries for the union of locales supported by the substituted holiday and the substitution pattern. This is doable, but it seems like a lot of effort. The resulting translations array would still need some locale-fallback-aware post-processing in order to be useful for the use-case described in this issue.

github-actions[bot] commented 4 years ago

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