Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the ICU files are built if there are no corresponding non-ICU translations #322

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

steveWinter
Copy link

In #312 it is observed that ICU translations are not dumped at all. One suggested solution to this is to add a corresponding non-ICU file in the same domain, and while this works it's not ideal.

I believe that the check in the dumper is incorrect and that the icu file should be skipped only if the corresponding non-ICU file does exist, whereas it's currently being skipped if it doesn't exist!

@pacproduct
Copy link

pacproduct commented Nov 22, 2021

Hi @steveWinter, glad I'm not the only one seeing this issue ^^
I have doubts about in_array($cleanedDomain, $domains, true) because if I understand the code correctly, $domains does not contain the list of domains, but the list of translations, per domain (domains are the keys).
See #312 (comment)

But even if we fixed this if correctly, I think function TranslationDumper::getTranslations needs work too, as it does not return ICU entries currently, does it?

(BTW, did you find a workaround for now? I mean... does adding a non-ICU file work for you? I can't make it work...)
EDIT: Okay got it, adding an empty file without the "+intl-icu" suffix AND adding an entry in the "bazinga_js_translation.yaml" configuration file does work, BUT it also names the folder containing translations differently (does not include the suffix anymore).

Thx :)

@steveWinter
Copy link
Author

@pacproduct looking at your other comments in #312 I suspect you have other problems here because either this patch, or adding in the empty file as others have suggested solved the issue for me and I now have several production instances running with my fork while I wait for this PR to be merged (or the issue to be solved in some other way)

@pacproduct
Copy link

You may well be right that I'm facing an issue in a particular context making it more difficult to solve ^^'

This said, considering the code being modified:

            foreach ($domains as $domain => $translations) {

                $cleanedDomain = Util::cleanDomain($domain);
                if ($domain !== $cleanedDomain && in_array($cleanedDomain, $domains, true)) {
                    // e.g.: skip "messages+intl-icu" if "messages" exists. They will get merged after.
                    continue;
                }

                // [...]

My understanding if that we loop over $domains and extract the key $domain on one side, and $translations on the other side: Which makes me think that the in_array used above searches $cleanedDomain among translations, and not domains.
That's probably always going to return FALSE and avoid the loop from being broken, which in turn includes all domains (and that solves the problem - and could be breaking what this code was supposed to do initially, which is unclear to me).

What I'm trying to say I suppose is that the change suggested looks to me like it's never going to enter the if condition anymore.

Or am I missing something? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants