API Replace i18n message localisation with symfony/translation #6558

tractorcow · 2017-01-24T01:50:23Z

Requires:

Upgrade cms for new i18n backend silverstripe-cms#1724 (can be merged without framework changes)
BUG Fix missing default on _t(‘GridField.Filter’) silverstripe-reports#58

This feature includes advanced pluralisation based on http://symfony.com/doc/current/components/translation/usage.html#component-translation-pluralization

An important upgrading note for this change is that any localisation calls to _t() without a default will now trigger a warning.

All existing Zend_Translate libraries and dependencies have been removed.

This does not replace Zend_Locale, however, which will be addressed later with #6194

There are some potential performance issues with this implementation: I will post benchmarking analysis later.

tractorcow · 2017-01-24T01:54:25Z

I also have some preliminary feedback from @sminnee which I need to implement:

i18nTextCollector should collect strings, even if they do not have a default defined, to ensure they show up as editable within transifex.
usage of pluralised strings with i18n::_t (rather than i18n::pluralise) should be a warning.

tractorcow · 2017-01-24T02:09:36Z

Phew, travis-ci isn't slower, as I had feared. :) Still about ~12 minutes per test run.

chillu · 2017-01-24T01:53:06Z

composer.json

@@ -22,7 +22,9 @@
    "league/flysystem": "~1.0.12",
    "symfony/yaml": "~2.7",
    "embed/embed": "^2.6",
-    "swiftmailer/swiftmailer": "~5.4"
+    "swiftmailer/swiftmailer": "~5.4",
+		"symfony/config": "^2.8|^3",


Indentation

More like, in-don't-ation right?

chillu · 2017-01-24T02:13:51Z

docs/en/04_Changelogs/4.0.0.md

+In many cases, localisation strings which worked in 3.x will continue to work in 4.0, however certain patterns
+have been deprecated and will be removed in 5.0. These include:
+
+ - _t calls with a $context parameter, which is ignored.


Hmm, this was a useful for translators, since text collector picks it up - we just stopped using it because transifex didn't support it at the time. Looks like support for comments has been added now: https://docs.transifex.com/formats/yaml. I originally considered this a regression when moving from the old getlocalization.com service over to transifex.

Context is fairly essential for good translation quality - for example, an identifier like CMSMain.HANDLE="handle" doesn't make it clear to a translator if it's used a verb or noun. The fact that we haven't been including comments is unlikely to generate a lot of noise with translators, but that's mainly because we don't talk to translators enough (rather than them not valuing the feature).

OK - if there's a reason we're not using it that's no longer relevant, then maybe it's worth keeping.

Sorry for forking the discussion, I've commented a bit more on the original RFC: #6221 (comment)

Added this back.

chillu · 2017-01-24T02:17:30Z

docs/en/04_Changelogs/4.0.0.md

+ - _t calls with a $context parameter, which is ignored.
+ - _t calls with sprintf-style placeholders (`%s`). Replace with named placeholders instead.
+ - _t calls with non-associative injection arguments. Please use an associative array for all arguments.
+ - _t calls which do not include a default value.


The intention here was to avoid duplication - you can use the same _t() call multiple times in your code, without duplicating the default value and having them diverge over time. But relying on one "master" _t() call is just as prone to human error, since you might remove from the code later on, only leaving the _t() call without the default value. In this case, text collection might set the value to null.

I think it's a per-project decision though: Some translation workflows will rely on developers maintaining YAML/JSON files directly, rather than relying on text collection - so you shouldn't force them to duplicate their efforts by setting default values in _t() calls.

Perhaps warn_on_missing_default is global config option set to true by default? If it has been missed, it's much more likely that someone has just forgotten it as opposed to it being a special workflow.

Sorry for forking the discussion, I've commented a bit more on the original RFC: #6221 (comment)

Done as @sminnee suggested. missing_default_warning added.

chillu · 2017-01-24T02:26:50Z

docs/en/02_Developer_Guides/13_i18n/index.md

+More information on what forms these plurals can take for various locales can be found on the
+[CLDR documentation](http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html)
+
+The ability to pluralise strings is provided through the `i18n::pluaralise` method, which is similar to the


chillu · 2017-01-24T02:30:25Z

docs/en/04_Changelogs/4.0.0.md

+#### <a name="overview-i18n-api"></a>i18n API Additions / Changes
+
+* Upgrade of i18n to symfony/translation
+* Localisation based on language-only (without any specific locale) is now supported


Wasn't that supported already? For example, we've got es.yml for generic Spanish, and es_MX.yml for Mexican Spanish.

I think what's changed is that you can do i18n::set_locale('es') now, where before you could have a set of es strings, but you'd still need to set i18n::set_locale('es_MX').

chillu · 2017-01-24T02:31:36Z

docs/en/04_Changelogs/4.0.0.md

+* Localisation based on language-only (without any specific locale) is now supported
+* i18nEntityProvider::provideI18nEntities() Now is expected to return only a single array
+  map of key to default values.
+* i18n keys for '.PLURAL_NAME' and '.SINGULAR_NAME' have been changed back to FQN class names


"FQN" isn't a commonly used term - just say "include their namespace"

FQCN is probably what you mean, but I agree it's best to avoid these abreviations where possible

I'll use <Namespaced\ClassName>.PLURALS

chillu · 2017-01-24T02:45:56Z

docs/en/02_Developer_Guides/05_Extending/05_Injector.md

@@ -121,6 +121,21 @@ As well as properties, method calls can also be specified:
 	      - [ pushHandler, [ %$DefaultHandler ] ]


+## Using constants as variables
+
+Any of the core constants can be used as a service argument by quoting with back ticks "`".


Awesome change, that would've come in handy a couple of times already!

chillu · 2017-01-24T02:47:12Z

lang/en.yml

    SINGULARNAME: 'Permission Role Code'
  SilverStripe\Security\RememberLoginHash:
    PLURALNAME: 'Remember Login Hashs'
+    PLURALS:
+      one: 'A Remember Login Hash'
+      other: '{count} Remember Login Hashs'


Hashs -> Hashes

chillu · 2017-01-24T02:50:29Z

src/Dev/Backtrace.php

@@ -155,7 +155,7 @@ public static function backtrace($returnVal = false, $ignoreAjax = false, $ignor
     * @param int $argCharLimit
     * @return string
     */
-    public static function full_func_name($item, $showArgs = false, $argCharLimit = 10000)
+    public static function full_func_name($item, $showArgs = false, $argCharLimit = 100)


What does that have to do with i18n?

Oops, this was just debugging changes I forgot to revert. :)

chillu · 2017-01-24T02:53:20Z

src/Dev/Tasks/i18nTextCollectorTask.php

@@ -6,7 +6,7 @@
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\Debug;
 use SilverStripe\Dev\BuildTask;
-use SilverStripe\i18n\i18nTextCollector;
+use SilverStripe\i18n\TextCollection\i18nTextCollector;


What do you think about moving text collection to its own module? As I've outlined in my comments around the _t() default value, I don't think text collection is the only translation workflow we should support - some teams will just manage their YAML/JSON separately. The reason I'm bringing this up is namespacing: If it's a separate module, we'd probably use SilverStripe\i18nTextCollection\i18nTextCollector instead of SilverStripe\i18n\TextCollection\i18nTextCollector (since the SilverStripe\i18n namespace should be core only)

Sure, but in another change. :)

chillu · 2017-01-24T03:02:46Z

src/i18n/Messages/Symfony/FlushInvalidatedResource.php

+ * Some arbitrary resource which expires when flush is invoked.
+ * Uses a canary file to mark future freshness requests as stale.
+ *
+ * @link https://media.giphy.com/media/fRRD3T37DeY6Y/giphy.gif for use case


hah, nice easter egg

chillu · 2017-01-24T03:04:24Z

src/i18n/Messages/Symfony/FlushInvalidatedResource.php

+     */
+    protected static function canary()
+    {
+        return TEMP_FOLDER . '/catalog.i18n_canary';


The class name and PHPDoc ("Some arbitrary resource") implies this is a generic helper, yet the canary file name is hardcoded to catalog.i18n_canary. Shouldn't it take a mandatory identifier as a constructor arg?

It needs to be accessible to flush handler, which is a static method, otherwise I'd make it a constructor argument.

robbieaverill · 2017-01-24T05:16:22Z

docs/en/04_Changelogs/4.0.0.md

+* Zend_Translate removed
+* i18n::_t $context parameter deprecated
+* i18n::_t Support for sprintf-style `%s` arguments deprecated
+* i18n::_t Using non-associative injection with named parameters is now an error


Minor: Can you either wrap things like this in backticks or escape the markdown characters to prevent the formatting breaking?

API Implement enhanced pluralisation Remove Zend_Translate and all Zend dependencies from i18n Deprecated $context from i18n::_t() Warn on missing default string for i18n::_t()

tractorcow · 2017-01-25T04:10:10Z

Updated based on feedback, as well as result of conversation over at #6221 (comment)

Rolled pluralisation functionality into the i18n::_t() method Warnings on missing default can now be turned off

tractorcow · 2017-01-25T22:44:41Z

Note: CMS build can be merged ahead of framework build, so we can restart and see if these tests pass.

silverstripe/silverstripe-cms#1724

tractorcow · 2017-01-26T04:26:20Z

silverstripe/silverstripe-cms#1724 is green

chillu · 2017-01-30T20:14:21Z

src/i18n/i18n.php

+        // inject the variables from injectionArray (if present)
+        $sprintfArgs = [];
+        if ($default && !preg_match('/\{[\w\d]*\}/i', $default) && preg_match('/%[s,d]/', $default)) {
+            Deprecation::notice('5.0', 'sprintf style localisation variables are deprecated');


We also need to replace existing core uses: #6582.

tractorcow mentioned this pull request Jan 24, 2017

RFC Replace or upgrade Zend_Translate #6221

Closed

This was referenced Jan 24, 2017

Upgrade cms for new i18n backend silverstripe/silverstripe-cms#1724

Merged

BUG Fix missing default on _t(‘GridField.Filter’) silverstripe/silverstripe-reports#58

Merged

chillu reviewed Jan 24, 2017

View reviewed changes

robbieaverill reviewed Jan 24, 2017

View reviewed changes

API Replace i18n message localisation with symfony/translation

8a07c56

API Implement enhanced pluralisation Remove Zend_Translate and all Zend dependencies from i18n Deprecated $context from i18n::_t() Warn on missing default string for i18n::_t()

tractorcow force-pushed the pulls/4.0/i18n-symfony branch from 34d34b2 to 719a00d Compare January 25, 2017 04:09

Restored context parameter to i18n, and added to a “comment” key

de02a3f

Rolled pluralisation functionality into the i18n::_t() method Warnings on missing default can now be turned off

tractorcow force-pushed the pulls/4.0/i18n-symfony branch from 719a00d to de02a3f Compare January 25, 2017 05:12

tractorcow mentioned this pull request Jan 25, 2017

Enhancement: SilverStripe 4 compatibility silverstripe/silverstripe-gridfield-bulk-editing-tools#137

Merged

dhensby merged commit c81959c into silverstripe:master Jan 26, 2017

tractorcow deleted the pulls/4.0/i18n-symfony branch January 26, 2017 20:35

This was referenced Jan 27, 2017

[4.0] dev/build fails when requireDefaultRecords uses a Zend class #6572

Closed

[4.0] "Reserved indicator ` cannot start a plain scalar" on Travis CI builds for a module #6574

Closed

This was referenced Jan 30, 2017

[2008-10-14] Support pluralization in i18n _t() #1896

Closed

Replace deprecated sprintf rules in core translations #6582

Closed

chillu reviewed Jan 30, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Replace i18n message localisation with symfony/translation #6558

API Replace i18n message localisation with symfony/translation #6558

tractorcow commented Jan 24, 2017 •

edited by dhensby

Loading

tractorcow commented Jan 24, 2017

tractorcow commented Jan 24, 2017

chillu Jan 24, 2017

tractorcow Jan 24, 2017

chillu Jan 24, 2017

sminnee Jan 24, 2017

chillu Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

sminnee Jan 24, 2017 •

edited

Loading

chillu Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

chillu Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

dhensby Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

chillu Jan 24, 2017

chillu Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

tractorcow Jan 25, 2017

chillu Jan 24, 2017

chillu Jan 24, 2017

tractorcow Jan 25, 2017

robbieaverill Jan 24, 2017

tractorcow commented Jan 25, 2017

tractorcow commented Jan 25, 2017

tractorcow commented Jan 26, 2017

chillu Jan 30, 2017

API Replace i18n message localisation with symfony/translation #6558

API Replace i18n message localisation with symfony/translation #6558

Conversation

tractorcow commented Jan 24, 2017 • edited by dhensby Loading

tractorcow commented Jan 24, 2017

tractorcow commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Jan 25, 2017

tractorcow commented Jan 25, 2017

tractorcow commented Jan 26, 2017

Choose a reason for hiding this comment

tractorcow commented Jan 24, 2017 •

edited by dhensby

Loading

sminnee Jan 24, 2017 •

edited

Loading