Feature #23583 Add current and fallback locales in WDT / Profiler #23625
Conversation
|
|
||
| <div class="metrics"> | ||
| <div class="metric"> | ||
| <span class="value">{{ collector.locale }}</span> |
There was a problem hiding this comment.
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 -
|
|
||
| {% if collector.fallbackLocales|length %} | ||
| <div class="metric"> | ||
| <span class="value">{{ collector.fallbackLocales | join(' | ') }}</span> |
There was a problem hiding this comment.
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.
|
Thanks for the review @javiereguiluz I've made some updates: -> use a comma for separate the values instead of | |
|
Adding to milestone 3.4 since this is where new features are merged. |
| * | ||
| * @return string | ||
| */ | ||
| public function getLocale() |
| * Gets the fallback locales. | ||
| * | ||
| * @return array | ||
| */ |
| */ | ||
| public function getLocale() | ||
| { | ||
| return !empty($this->data['locale']) ? $this->data['locale'] : '-'; |
There was a problem hiding this comment.
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)
e425993 to
2ccab30
Compare
|
@javiereguiluz Yes make sense. Here the update: -> remove phpdoc |
| @@ -13,6 +13,12 @@ | |||
|
|
|||
| {% set text %} | |||
| <div class="sf-toolbar-info-piece"> | |||
| <b>Locale</b> | |||
| <span class="sf-toolbar-status"> | |||
| {{ collector.locale | default('-') }} | |||
There was a problem hiding this comment.
the spaces around the pipe operator should be removed (same below)
4b4ea3b to
01ce8f0
Compare
|
@xabbuh Thx, update done. |
|
I think you could best rebase these changes onto the |
01ce8f0 to
98a8a6c
Compare
|
@xabbuh Yes, it's done too now. |
|
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
The goal of this PR is to add informations about the locale and the fallback locales in the translation WDT panel / and profiler