Feature #23583 Add current and fallback locales in WDT / Profiler #23625

nemoneph · 2017-07-22T12:32:01Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23583
License	MIT
Doc PR	-

The goal of this PR is to add informations about the locale and the fallback locales in the translation WDT panel / and profiler

javiereguiluz · 2017-07-24T07:30:54Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig

+
+    <div class="metrics">
+        <div class="metric">
+            <span class="value">{{ collector.locale }}</span>


When some data is missing, we use a dash (-) instead of an empty string (e.g. -> <td class="text-right">{{ listener.priority|default('-') }}</td>). The problem with a white space is that you don't know if it's because of some error or because there's really no data.

In TranslationDataCollector.php you used an empty string when there's no locale, so maybe here we can check if it's empty and display a -

javiereguiluz · 2017-07-24T07:31:35Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig

+
+        {% if collector.fallbackLocales|length %}
+            <div class="metric">
+                <span class="value">{{ collector.fallbackLocales | join(' | ')  }}</span>


Here, in addition to showing a - when there are no fallback locales, I'd use a comma (,) instead of a pipe (|) to separate the values.

nemoneph · 2017-07-24T18:23:33Z

Thanks for the review @javiereguiluz

I've made some updates:

-> use a comma for separate the values instead of |
-> if locale is empty, set the default value to '-' (check/set this in TranslationDataCollector is enough ?)
-> if the fallback_locales is empty, set the default value to '-'

nicolas-grekas · 2017-07-25T12:48:25Z

Adding to milestone 3.4 since this is where new features are merged.

fabpot · 2017-07-25T12:49:23Z

src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php

+     *
+     * @return string
+     */
+    public function getLocale()


phpdoc can be removed

fabpot · 2017-07-25T12:49:41Z

src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php

+     * Gets the fallback locales.
+     *
+     * @return array
+     */


javiereguiluz · 2017-07-25T13:33:23Z

src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php

+     */
+    public function getLocale()
+    {
+        return !empty($this->data['locale']) ? $this->data['locale'] : '-';


A quick comment about using - as a return value: the PHP methods of data collectors can be called from other PHP code (e.g. when using the profiler in tests). So I always prefer to return values optimized for PHP. In this case, null or an empty string (as you originally did) would be more convenient than returning -. The - symbol is just a visual detail used to let the user know that this value is empty ... so maybe is better to move that to the Twig template. Thanks!

(same comment for getFallbackLocales() ... retuning an empty array may be better)

nemoneph · 2017-07-25T17:20:35Z

@javiereguiluz Yes make sense.

Here the update:

-> remove phpdoc
-> use default('-') in view instead of TranslationDataCollector

xabbuh · 2017-09-04T16:25:04Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig

@@ -13,6 +13,12 @@

        {% set text %}
            <div class="sf-toolbar-info-piece">
+                <b>Locale</b>
+                <span class="sf-toolbar-status">
+                    {{ collector.locale | default('-') }}


the spaces around the pipe operator should be removed (same below)

nemoneph · 2017-09-05T17:24:47Z

@xabbuh Thx, update done.

xabbuh · 2017-09-11T15:32:59Z

I think you could best rebase these changes onto the 3.4 branch now and change the target branch here.

nemoneph · 2017-09-11T17:10:40Z

@xabbuh Yes, it's done too now.

xabbuh

failure is not related

fabpot · 2017-09-13T16:49:11Z

Thank you @nemoneph.

… / Profiler (nemoneph) This PR was merged into the 3.4 branch. Discussion ---------- Feature #23583 Add current and fallback locales in WDT / Profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23583 | License | MIT | Doc PR | - The goal of this PR is to add informations about the locale and the fallback locales in the translation WDT panel / and profiler Commits ------- 98a8a6c Feature #23583 Add current and fallback locales in WDT / Profiler

…locales in WDT / Profiler (nemoneph) This PR was merged into the 3.4 branch. Discussion ---------- Feature symfony#23583 Add current and fallback locales in WDT / Profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#23583 | License | MIT | Doc PR | - The goal of this PR is to add informations about the locale and the fallback locales in the translation WDT panel / and profiler Commits ------- 98a8a6c Feature symfony#23583 Add current and fallback locales in WDT / Profiler

carsonbot added Status: Needs Review Feature labels Jul 22, 2017

nemoneph force-pushed the feature/23583 branch from 5f1a700 to f0e0ee9 Compare July 22, 2017 12:35

javiereguiluz reviewed Jul 24, 2017

View reviewed changes

javiereguiluz added WebProfilerBundle Status: Reviewed and removed Status: Needs Review labels Jul 24, 2017

nemoneph force-pushed the feature/23583 branch from f0e0ee9 to 3f7edd0 Compare July 24, 2017 18:20

nicolas-grekas added this to the 3.4 milestone Jul 25, 2017

fabpot requested changes Jul 25, 2017

View reviewed changes

javiereguiluz reviewed Jul 25, 2017

View reviewed changes

nemoneph force-pushed the feature/23583 branch 2 times, most recently from e425993 to 2ccab30 Compare July 25, 2017 17:15

nemoneph force-pushed the feature/23583 branch from 2ccab30 to 4b4ea3b Compare July 26, 2017 17:06

xabbuh reviewed Sep 4, 2017

View reviewed changes

nemoneph force-pushed the feature/23583 branch from 4b4ea3b to 01ce8f0 Compare September 5, 2017 17:23

Feature symfony#23583 Add current and fallback locales in WDT / Profiler

98a8a6c

nemoneph force-pushed the feature/23583 branch from 01ce8f0 to 98a8a6c Compare September 11, 2017 17:08

nemoneph changed the base branch from master to 3.4 September 11, 2017 17:09

xabbuh approved these changes Sep 12, 2017

View reviewed changes

javiereguiluz approved these changes Sep 12, 2017

View reviewed changes

fabpot approved these changes Sep 13, 2017

View reviewed changes

fabpot merged commit 98a8a6c into symfony:3.4 Sep 13, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature #23583 Add current and fallback locales in WDT / Profiler #23625

Feature #23583 Add current and fallback locales in WDT / Profiler #23625

nemoneph commented Jul 22, 2017 •

edited

Loading

javiereguiluz Jul 24, 2017

javiereguiluz Jul 24, 2017

nemoneph commented Jul 24, 2017 •

edited

Loading

nicolas-grekas commented Jul 25, 2017

fabpot Jul 25, 2017

fabpot Jul 25, 2017

javiereguiluz Jul 25, 2017

nemoneph commented Jul 25, 2017

xabbuh Sep 4, 2017

nemoneph commented Sep 5, 2017

xabbuh commented Sep 11, 2017

nemoneph commented Sep 11, 2017

xabbuh left a comment

fabpot commented Sep 13, 2017

Feature #23583 Add current and fallback locales in WDT / Profiler #23625

Feature #23583 Add current and fallback locales in WDT / Profiler #23625

Conversation

nemoneph commented Jul 22, 2017 • edited Loading

javiereguiluz Jul 24, 2017

Choose a reason for hiding this comment

javiereguiluz Jul 24, 2017

Choose a reason for hiding this comment

nemoneph commented Jul 24, 2017 • edited Loading

nicolas-grekas commented Jul 25, 2017

fabpot Jul 25, 2017

Choose a reason for hiding this comment

fabpot Jul 25, 2017

Choose a reason for hiding this comment

javiereguiluz Jul 25, 2017

Choose a reason for hiding this comment

nemoneph commented Jul 25, 2017

xabbuh Sep 4, 2017

Choose a reason for hiding this comment

nemoneph commented Sep 5, 2017

xabbuh commented Sep 11, 2017

nemoneph commented Sep 11, 2017

xabbuh left a comment

Choose a reason for hiding this comment

fabpot commented Sep 13, 2017

nemoneph commented Jul 22, 2017 •

edited

Loading

nemoneph commented Jul 24, 2017 •

edited

Loading