Translate some locales in order to match locales available from bower dependencies #4878

phansys · 2018-01-04T01:53:21Z

I am targeting this branch, because there are some locales from bower dependencies which are not found under the current implementation.

Changelog

### Fixed
- Not found issues for some locales which are not present in frontend dependencies like `moment` or `select2`

TODO

Add tests.

Closes #3642.

core23 · 2018-01-04T05:02:27Z

src/Resources/views/standard_layout.html.twig

-                {% if locale[:2] != 'en' %}
-                    <script src="{{ asset('bundles/sonatacore/vendor/select2/select2_locale_' ~ locale|replace({'_':'-'}) ~ '.js') }}"></script>
+                {# localize select2 #}
+                {% if sonata_admin.adminPool.getOption('use_select2') %}


Can we move this stuff to a twig function to reduce the code inside the template?

with tests, preferrably

greg0ire · 2018-01-04T15:03:17Z

src/Twig/Extension/SonataAdminExtension.php

+     *
+     * @return null|string
+     */
+    public function getCanonicalizedLocaleForMoment(array $context)


Should be final

greg0ire · 2018-01-04T15:03:24Z

src/Twig/Extension/SonataAdminExtension.php

+     * Returns a canonicalized locale for "moment" NPM library,
+     * or `null` if the locale's language is "en", which doesn't require localization.
+     *
+     * @param array $context


Please remove this

You mean the whole docblock @greg0ire?

no, just this line, it's tautologic, whereas the rest is interesting

greg0ire · 2018-01-04T15:04:10Z

src/Twig/Extension/SonataAdminExtension.php

+            return null;
+        }
+
+        // `moment: ^2.8` only ships "es" and "es-do" locales for "es" language


Why use backticks here?

Beacuse this is a reference to a version constraint declared in bower.json at sonata-project/core-bundle.

I mean why use this over quotes, etc... markdown is not recognized here, or is it some other markup? For what tool are you using this markup

This is not for a tool, I use backticks just to make explicit that this constraint is used here as a snippet.

Ok, why not

greg0ire · 2018-01-04T15:04:38Z

src/Twig/Extension/SonataAdminExtension.php

+     * Returns a canonicalized locale for "select2" NPM library,
+     * or `null` if the locale's language is "en", which doesn't require localization.
+     *
+     * @param array $context


Please remove this

phansys · 2018-01-04T15:42:53Z

@greg0ire, how I should execute bower install in the test suite in order to check for the available locales?
I was thinking to iterate over the resulting files to make sure we are not out of date with the shipped locales for these libraries.
Should I use different environments with distinct command flags for lowest / highest version resolutions?

greg0ire · 2018-01-04T16:08:01Z

@phansys this sounds overkill, but if we were to accept that, I think it would mean a separate Travis job just for that.

phansys · 2018-01-04T17:37:34Z

Your concerns are about performance? I was thinking to use the symfony/finder, which is really fast.

greg0ire · 2018-01-04T17:39:53Z

My concerns are about maintenance, but maybe you can try it anyway and I will be convinced :P

svenluijten · 2018-01-08T09:00:59Z

src/Twig/Extension/SonataAdminExtension.php

+        }
+
+        // `moment: ^2.8` only ships "es" and "es-do" locales for "es" language
+        if ('es' === $lang && !in_array($locale, ['es', 'es-do'], true)) {


This poses the same problem for nl-nl. Moment only has nl.js, but Symfony's request locale is nl-nl.

Can you do something about it @phansys ?

@svenluijten, @greg0ire: 83484cd#diff-83a367a366e6d48e0d8ea08cea7f0351R451

OskarStark · 2018-01-16T17:57:40Z

Can you please check FlintCI?

OskarStark

👍🏻

phansys · 2018-01-16T18:01:38Z

@OskarStark, FlintCI suggested changes are not related to this PR. Would I apply them?

OskarStark · 2018-01-16T18:02:43Z

Strange, could you rebase?

OskarStark · 2018-01-16T18:05:17Z

tests/Twig/Extension/SonataAdminExtensionTest.php

+    {
+        $context = $this->mockExtensionContext($original);
+
+        $this->assertSame($this->twigExtension->getCanonicalizedLocaleForSelect2($context), $canonicalized);


Please rename canonicalized to expected and use it as the first argument, your code result should be the second argument.

Also switch the signature parameters in line 2353 please

Same for the other method

Thank you @OskarStark, done.

phansys · 2018-01-16T22:10:30Z

Rebased.

OskarStark

Afterwards we can merge this PR

OskarStark · 2018-01-17T07:29:20Z

src/Twig/Extension/SonataAdminExtension.php

+            $locale = 'es';
+        }
+
+        // @todo: there are more locales which are not supported by moment and they need to be translated/normalized/canonicalized here


do we really need this comment?

I think yes @OskarStark, since this method isn't getting rid of them (see #4878 (review) by instance). I think these matches should be added by users which know the right locale fallback in each case.

OskarStark · 2018-01-17T07:30:00Z

tests/Twig/Extension/SonataAdminExtensionTest.php

+    /**
+     * @dataProvider select2LocalesProvider
+     */
+    public function testCanonicalizedLocaleForSelect2($original, $expected)


please use $expected as the first argument, like in the assertations.

Same for the other method

greg0ire · 2018-01-17T09:24:13Z

src/Twig/Extension/SonataAdminExtension.php

+    public function getFunctions()
+    {
+        return [
+            new \Twig_SimpleFunction('canonicalize_locale_for_moment', [$this, 'getCanonicalizedLocaleForMoment'], ['needs_context' => true]),


Please use the new, namespaced version. See https://github.com/rectorphp/rector/blob/master/src/config/level/twig/underscore-to-namespace.yml

greg0ire · 2018-01-17T09:25:21Z

tests/Twig/Extension/SonataAdminExtensionTest.php

+     */
+    public function testCanonicalizedLocaleForMoment($original, $expected)
+    {
+        $context = $this->mockExtensionContext($original);


Please get rid of this one time variable

greg0ire · 2018-01-17T13:04:39Z

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

git rebase -i origin/3.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already pushed. Don't forget the --force option otherwise git will try to merge both things together.

phansys · 2018-01-17T13:07:10Z

Rebased and squashed @greg0ire.

phansys · 2018-01-25T16:15:20Z

@core23, @OskarStark; is there something preventing this PR to be merged. Please let me know if there is something on my side that could be done.

OskarStark · 2018-01-25T16:22:45Z

@greg0ire please give a final review, thank you!

… dependencies

phansys · 2018-01-30T17:48:09Z

Thank you @OskarStark.

phansys force-pushed the handling_locales_from_bower_dependencies branch 2 times, most recently from a6584fb to 698cb91 Compare January 4, 2018 02:12

core23 requested changes Jan 4, 2018

View reviewed changes

OskarStark added the patch label Jan 4, 2018

phansys force-pushed the handling_locales_from_bower_dependencies branch from 111f66e to 2fa76a7 Compare January 4, 2018 15:02

greg0ire requested changes Jan 4, 2018

View reviewed changes

svenluijten reviewed Jan 8, 2018

View reviewed changes

phansys force-pushed the handling_locales_from_bower_dependencies branch from 3c9f241 to 3b55476 Compare January 16, 2018 16:14

OskarStark approved these changes Jan 16, 2018

View reviewed changes

OskarStark requested changes Jan 16, 2018

View reviewed changes

phansys force-pushed the handling_locales_from_bower_dependencies branch from 138040c to dd1c609 Compare January 16, 2018 18:30

OskarStark requested changes Jan 17, 2018

View reviewed changes

greg0ire reviewed Jan 17, 2018

View reviewed changes

OskarStark previously approved these changes Jan 17, 2018

View reviewed changes

greg0ire previously approved these changes Jan 17, 2018

View reviewed changes

greg0ire requested a review from core23 January 17, 2018 13:04

phansys dismissed stale reviews from greg0ire and OskarStark via 483b0fa January 17, 2018 13:06

phansys force-pushed the handling_locales_from_bower_dependencies branch from be99547 to 483b0fa Compare January 17, 2018 13:06

phansys force-pushed the handling_locales_from_bower_dependencies branch from 483b0fa to 4dc0c39 Compare January 17, 2018 13:21

OskarStark previously approved these changes Jan 25, 2018

View reviewed changes

core23 previously approved these changes Jan 25, 2018

View reviewed changes

Translate some locales in order to match locales available from bower…

83484cd

… dependencies

phansys dismissed stale reviews from core23 and OskarStark via 83484cd January 25, 2018 20:16

phansys force-pushed the handling_locales_from_bower_dependencies branch from 4dc0c39 to 83484cd Compare January 25, 2018 20:16

OskarStark approved these changes Jan 27, 2018

View reviewed changes

core23 approved these changes Jan 27, 2018

View reviewed changes

OskarStark merged commit 33ff7c4 into sonata-project:3.x Jan 28, 2018

phansys deleted the handling_locales_from_bower_dependencies branch January 30, 2018 17:48

phansys mentioned this pull request Jul 27, 2019

feature/encore-webpack #5461

Closed

9 tasks

Translate some locales in order to match locales available from bower dependencies #4878

Translate some locales in order to match locales available from bower dependencies #4878

Conversation

phansys commented Jan 4, 2018 • edited Loading

Changelog

TODO

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

phansys commented Jan 4, 2018

greg0ire commented Jan 4, 2018

phansys commented Jan 4, 2018

greg0ire commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Jan 16, 2018

OskarStark left a comment

Choose a reason for hiding this comment

phansys commented Jan 16, 2018

OskarStark commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansys commented Jan 16, 2018

OskarStark left a 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

greg0ire commented Jan 17, 2018

phansys commented Jan 17, 2018

phansys commented Jan 25, 2018

OskarStark commented Jan 25, 2018

phansys commented Jan 30, 2018

phansys commented Jan 4, 2018 •

edited

Loading