Locale:* bakery command

[amosfolz]

Adds three Bakery commands:

• locale:missing-keys check for missing locale keys
• locale:missing-values check for empty/duplicate locale values.
• locale:fix-keys Fix locale keys by adding any missing key:values from another locale. I.e. add all key:values that exist in en_US locale files (but not in es_ES files) into es_ES locale files.

locale:fix-keys can also be used to create the folder structure and all locale files for a brand new locale.

[codecov]

Codecov Report

Merging #992 into develop will decrease coverage by 0.45%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             develop     #992      +/-   ##
=============================================
- Coverage      66.82%   66.36%   -0.46%     
- Complexity      2080     2098      +18     
=============================================
  Files            160      162       +2     
  Lines           6791     6838      +47     
=============================================
  Hits            4538     4538              
- Misses          2253     2300      +47

Impacted Files	Coverage Δ	Complexity Δ
...nkles/core/src/Bakery/LocaleMissingKeysCommand.php	0% <0%> (ø)	9 <9> (?)
...les/core/src/Bakery/LocaleMissingValuesCommand.php	0% <0%> (ø)	9 <9> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfa02d2...8be4d2d. Read the comment docs.

[codecov]

Codecov Report

Merging #992 into develop will increase coverage by 0.41%.
The diff coverage is 78%.

@@              Coverage Diff              @@
##             develop     #992      +/-   ##
=============================================
+ Coverage      65.44%   65.85%   +0.41%     
- Complexity      2007     2046      +39     
=============================================
  Files            164      172       +8     
  Lines           6977     7096     +119     
=============================================
+ Hits            4566     4673     +107     
- Misses          2411     2423      +12

Impacted Files	Coverage Δ	Complexity Δ
app/sprinkles/account/src/Account/Registration.php	75.29% <ø> (ø)	23 <0> (ø)	⬇️
...rinkles/account/src/Authenticate/Authenticator.php	92.79% <ø> (+0.9%)	40 <0> (ø)	⬇️
...es/admin/src/ServicesProvider/ServicesProvider.php	80.95% <ø> (ø)	4 <0> (ø)	⬇️
app/sprinkles/core/src/Database/Models/Model.php	69.69% <ø> (ø)	10 <0> (ø)	⬇️
.../account/src/ServicesProvider/ServicesProvider.php	77.77% <ø> (+0.17%)	11 <0> (-2)	⬇️
...les/core/src/ServicesProvider/ServicesProvider.php	88.17% <ø> (+5.15%)	28 <0> (-8)	⬇️
...prinkles/core/src/Filesystem/FilesystemManager.php	76.92% <ø> (ø)	6 <0> (ø)	⬇️
...les/account/src/Authorize/AuthorizationManager.php	100% <ø> (ø)	20 <0> (ø)	⬇️
...inkles/core/src/Error/Handler/ExceptionHandler.php	34.66% <ø> (ø)	26 <0> (ø)	⬇️
app/sprinkles/core/src/Csrf/SlimCsrfProvider.php	80.76% <ø> (ø)	11 <0> (ø)	⬇️
... and 48 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a469ed4...22cd1bf. Read the comment docs.

[lcharette]

Class name is broken :
https://github.styleci.io/analyses/X0Lyvx?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

This could also be optimized by having one class extend the other so the common methods can be reused. There's probably more optimization we could add in userfrosting/i18n to load all keys.

[lcharette]

Related #671

[lcharette]

Also, quick note about that code vs testing. You have a foreach in a foreach in an if in a foreach... This will probably make it hard to create a meaningful test for that class. You should look at separating the code into more protected methods to avoid this nesting. One example, the table io generation should be in it's own method. This would also means it could probably be reused between both commands.

(And yes, I know. MigrateStatusCommand is the opposite of what I just said :P )

[amosfolz]

Also, quick note about that code vs testing. You have a foreach in a foreach in an if in a foreach... This will probably make it hard to create a meaningful test for that class. You should look at separating the code into more protected methods to avoid this nesting. One example, the table io generation should be in it's own method. This would also means it could probably be reused between both commands.

(And yes, I know. MigrateStatusCommand is the opposite of what I just said :P )

I will need some help on how to do this in a different way. Basically this entire command revolves around a whole lot of arrays...

Outdated

[lcharette]

I've made some changes in 8e13cc7. Most changes are style related or adding the PHP 7 type hint for method arguments and return values. I've also optimized the code to make use of return value where possible instead of class properties. Also FYI, class properties where marked as static (eg. protected static $path;), which is not necessary in this case (they were not called statically).

In the bigger changes:

• There was a edge case in missing-keys where a key containing an array wouldn't display as missing if the target locale had the key, but containing a string.
• I changed the locale:fix-keys options from -f to -l. f should be associated with force whenever possible. In this case, I added the force option so the confirmation can be skipped.

Let me know if some of my changes doesn't give the right result, or if you don't understand some of them (might be my personal coding preference).

Before merging this in, I'll try to add some tests for those three commands.

[lcharette]

I've been thinking about this command since my last commit and I think the whole logic of comparing the files is flawed.

The commands you created compares the content of locale/en_US/messages.php with locale/es_ES/messages.php for example. The problem is comparing files doesn't represent the end result which merge all arrays from all files. It is great to do single file comparison, but let's say the Spanish translation is in locale/es_ES/mensajes.php. The commands will show all keys from the English as missing, but the translation will actually works in UF.

I could also create false positive if the core translation has a key that contains an array ('FOO' => array(...);), but a sprinkle overwrite that ('FOO' => 'BAR';).

I think it would be best if the translator would have a feature to return a list of all registered keys. Since 4.4 Milestone focus on translation improvement, I'll push this feature to 4.4. At the same time, improvements to the i18n package (#868) will probably help with this, and it also means the comparison can probably be moved to i18n and the translator itself.

[amosfolz]

I think this entire feature has had a lot of creep outside what I was really trying to accomplish. As per @alexweissman original post in #671 , this was primarily meant to be a tool for helping to keep the built-in translation files up-to-date.

The commands you created compares the content of locale/en_US/messages.php with locale/es_ES/messages.php for example. The problem is comparing files doesn't represent the end result which merge all arrays from all files. It is great to do single file comparison, but let's say the Spanish translation is in locale/es_ES/mensajes.php. The commands will show all keys from the English as missing, but the translation will actually works in UF.

This is true but I'm not sure it's necessarily a flaw. I think when you write code there are assumptions that have to be made. In this situation the assumption is that a standardized, logical naming schema will be used for translation files. It seems it would get extremely complicated to write code that will update/create the translation files without these simple file naming conventions in place.

I think it would be best if the translator would have a feature to return a list of all registered keys. Since 4.4 Milestone focus on translation improvement, I'll push this feature to 4.4. At the same time, improvements to the i18n package (#868) will probably help with this, and it also means the comparison can probably be moved to i18n and the translator itself.

I'm fine with this but I still think #1006 needs to be considered for 4.3. This will allow translators to update any missing translations and can be added in 4.3.*

[lcharette]

Maybe both method of comparing a locale could live? 🤔

I'm fine with this but I still think #1006 needs to be considered for 4.3. This will allow translators to update any missing translations and can be added in 4.3.*

I agree. Changed #1006 to 4.3 milestone.

[amosfolz]

I think it would be best if the translator would have a feature to return a list of all registered keys.

I think this could pretty easily be done by adding a method or two to the MessageTranslator class.
For example:

        $locator = $this->ci->locator;
        $builder = new LocalePathBuilder($locator, 'locale://', [en_US]);
        $loader = new ArrayFileLoader($builder->buildPaths());
        $translator = new MessageTranslator($loader->load());

$translator will have an array of all en_US keys.

We could just add a method to return $translator->items, which would be an array of all en_US keys/values in this example.

I actually looked back at my notes and this was my original approach for comparison between two different locales, but ended up changing it because because I couldn't see a way to find what files had what keys defined.

[lcharette]

That would probably work great with #868, so you can get a list of keys directly :

$keys = $translator->getKeys();
$FrKeys = $trsanslator->getKeys('fr_FR');

[lcharette]

@amosfolz I've reworked your command into a single locale:compare command. Having missing-keys and missing-values was weird. The compare command list all difference between the two locales. Obviously, the new command makes uses of the new methods available in i18n to list all keys (and the actual comparison has been moved to i18n also).

Regarding the locale:fix command, I don't think it's a good fit yet. The points I didn't like was the use of php-cs-fixer inside the code (makes it required in the non-dev composer packages) and the fact the file are actually modified (which required the necessary permissions).

What I propose, is we add in future version an output argument to the compare:locale, and eventually create a new command, which will output the difference, or the whole locale, in a specific version (display, cvs, txt, array, etc.)