[TwigBundle] make date formats and number formats configurable #13554

xabbuh · 2015-01-30T08:59:43Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13552
License	MIT
Doc PR	TODO

This adds new Twig configuration options that make it possible to
configure the format of both numbers and dates as well as timezones
without the need to write custom code.

For example, using the new configuration options can look like this:

twig:
    date:
        format: d.m.Y, H:i:s
        interval_format: %%d days
        timezone: Europe/Berlin
    number_format:
        decimals: 2
        decimal_point: ,
        thousands_separator: .

stof · 2015-01-30T09:04:04Z

this implementation is wrong. the Twig_Environment should not be configured through the TwigEngine, as this means that the twig service is not configured fully whne using it directly rather than using the templating component

xabbuh · 2015-01-30T09:06:26Z

I see your point, but do we have a better place where we can call methods of the Twig extensions?

stof · 2015-01-30T09:10:16Z

@xabbuh Well, there is 2 solutions here:

registering the Twig_Extension_Core explicitly (so that we have a private service for it)
using a service configurator to apply the logic. An example can be found in DoctrineBundle used there

stof · 2015-01-30T09:10:47Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

+                ->arrayNode('number_format')
+                    ->addDefaultsIfNotSet()
+                    ->children()
+                        ->scalarNode('decimals')->defaultValue(0)->end()


should be an integer node

xabbuh · 2015-01-30T10:06:36Z

@stof Thanks for pointing me towards service configurators.

xabbuh · 2015-01-30T10:08:48Z

src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml

@@ -165,5 +166,14 @@
            <argument type="service" id="http_kernel" />
            <argument>%twig.exception_listener.controller%</argument>
        </service>
+
+        <service id="twig.configurator.environment" class="Symfony\Bundle\TwigBundle\DependencyInjection\Configurator\EnvironmentConfigurator">


Interestingly, we cannot make the service private because it then gets inlined and the XML dumper fails here: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php#L182

The error message then is:

ContextErrorException: Warning: DOMElement::setAttribute() expects parameter 2 to be string, object given in /var/www/symfony/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php line 182

Casting the second argument to a string should fix the issue properly.

no it won't. The issue is that the XML format cannot accept inline services for the configurator, and so cannot handle the case of private configurator service definitions getting inlined in place where they are used

Imho the same would be true for other formats as well since the definition does not contain the service id.

@xabbuh the PHP dumper already supports this case. And the YamlDumper is irrelevant, because it is already unusable on a compiled container (the YAML config format does not support inline service definitions in arguments for instance)

Please have a look at #13557 for a proposal about making it possible to use inlined configurator services in the XML format.

Given that factories and configurators can now be inlined, this is not an issue anymore and I made the service private.

I guess you need to update the depdency to require the DependencyInjection component version where this has been fixed.

I'm not sure if we really need to do this. It would mean to add a conflict rule to the Composer config. The issue has been solved in the latest patch version of all maintained Symfony versions though. How did we deal with things like this in the past?

We didn't pay enough attention to such things so far. But to be correct it would be required.

xabbuh · 2015-03-23T19:26:23Z

ping @symfony/deciders

Tobion · 2015-03-23T20:10:55Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

+                    ->children()
+                        ->scalarNode('format')->defaultNull()->end()
+                        ->scalarNode('interval_format')->defaultNull()->end()
+                        ->scalarNode('timezone')->defaultNull()->end()


should be documented that null means date_default_timezone_get

xabbuh · 2015-03-27T15:04:31Z

I addressed @Tobion's last comments and added an entry to the changelog.

Tobion · 2015-03-27T16:20:08Z

@stof what do you think about #13554 (comment)

xabbuh · 2015-03-27T17:17:26Z

By the way, why is the DependencyInjection component not a mandatory requirement? Does this make sense at all?

xabbuh · 2015-03-31T15:23:17Z

ping @symfony/deciders Would be nice if we could get this into 2.7. Bumping the DependencyInjection version imho could be discussed afterwards.

Tobion · 2015-03-31T20:25:08Z

👍

aitboudad · 2015-03-31T21:11:15Z

👍

fabpot · 2015-04-02T13:34:25Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

+        $container->setParameter('twig.number_format.decimal_point', $config['number_format']['decimal_point']);
+        $container->setParameter('twig.number_format.decimals', $config['number_format']['decimals']);
+        $container->setParameter('twig.number_format.thousands_separator', $config['number_format']['thousands_separator']);
+


All those parameters should be removed and instead, you should inject the value in the service directly. That's what we are doing now as much as possible to avoid polluting the parameters with information you don't need to access.

stloyd · 2015-04-03T08:27:12Z

src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml

+            <argument />
+            <argument />
+            <argument />
+            <argument />


Maybe to not confuse developers looking where those arguments are set add comment like:

This adds new Twig configuration options that make it possible to configure the format of both numbers and dates as well as timezones without the need to write custom code. For example, using the new configuration options can look like this: ```yaml twig: date: format: d.m.Y, H:i:s interval_format: %%d days timezone: Europe/Berlin number_format: decimals: 2 decimal_point: , thousands_separator: . ```

fabpot · 2015-04-03T12:26:39Z

Thank you @xabbuh.

…igurable (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBundle] make date formats and number formats configurable | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13552 | License | MIT | Doc PR | TODO This adds new Twig configuration options that make it possible to configure the format of both numbers and dates as well as timezones without the need to write custom code. For example, using the new configuration options can look like this: ```yaml twig: date: format: d.m.Y, H:i:s interval_format: %%d days timezone: Europe/Berlin number_format: decimals: 2 decimal_point: , thousands_separator: . ``` Commits ------- e9bc23b make date formats and number formats configurable

King2500 · 2015-04-06T14:07:16Z

This is fine. As long as you dont have multiple locales. Wouldn't it make more sense to configure it (additionally?) via translation files?

smolinari · 2015-04-09T06:41:02Z

I made that same argument here: #13552 (comment)

and got no love.

I agree there needed to be a better overall way to generally make changing formats easier, however, I see it the same way as @King2500 .

Date, number and even currency formats should be part of ( are defined by?) the end user's locale. If you only need one format, then you most likely only have your application working with users who live in one locale (if there are other reasons for changing the formatting in an application globally, please let me know, as I don't understand the reasoning/ use cases). If you have different users with different locales using the application, then you have to use different locale formats. How does this improvement work with those other locale formats?

Also, if you actually do need to use different formatting schemes than the locales offer (and again, I'd love to understand why this might be necessary in a global perspective), then the locale definition is what should be overridable (or possibly make custom locale definitions? Though, that makes even less sense...hmmmm.).

Or actually, this extension

http://twig.sensiolabs.org/doc/extensions/intl.html

has the right logic and I think should actually be part of the core twig package (of an internationally usable templating system). With it in the core, then the locales should be configurable as to which locales are used in the application (defined by each user) and/ or which are overridden or new or customized and how. So that way, when you create templates with variables needing formatting, Twig already takes the user's locale (or the special overridden formatting) into consideration and outputs the formatting accordingly (yes, I'd even leave out the filters and only use a filter, if overriding in the template is actually necessary).

If the addition of the extension to the core causes "too much overhead" and why others that don't really need it can avoid using it (and why it is an extension?), then I'd venture to say, the whole formatting system in Twig needs to be revisited. It wasn't created with an "international templating system" in mind, which any template system should be first and foremost.

Scott

…igs (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBundle] allow to configure custom formats in XML configs | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13554 | License | MIT | Doc PR | Commits ------- 5a3a24b allow to configure custom formats in XML configs

xabbuh force-pushed the issue-13552 branch from 43439c7 to 98ca178 Compare January 30, 2015 09:00

stof reviewed Jan 30, 2015
View reviewed changes

xabbuh force-pushed the issue-13552 branch from 98ca178 to 58c1ff5 Compare January 30, 2015 10:05

xabbuh reviewed Jan 30, 2015
View reviewed changes

fabpot added the TwigBundle label Feb 5, 2015

Tobion reviewed Mar 23, 2015
View reviewed changes

xabbuh force-pushed the issue-13552 branch 3 times, most recently from c7716eb to 82b249b Compare March 27, 2015 15:03

xabbuh force-pushed the issue-13552 branch from 82b249b to f4f3a7a Compare March 27, 2015 19:10

fabpot reviewed Apr 2, 2015
View reviewed changes

xabbuh force-pushed the issue-13552 branch from f4f3a7a to 5133a4f Compare April 3, 2015 08:24

stloyd reviewed Apr 3, 2015
View reviewed changes

xabbuh force-pushed the issue-13552 branch from 5133a4f to e9bc23b Compare April 3, 2015 08:30

fabpot merged commit e9bc23b into symfony:2.7 Apr 3, 2015

xabbuh deleted the issue-13552 branch April 3, 2015 12:41

xabbuh mentioned this pull request Jul 9, 2017

[TwigBundle] allow to configure custom formats in XML configs #23459

Merged

Uh oh!

[TwigBundle] make date formats and number formats configurable #13554

[TwigBundle] make date formats and number formats configurable #13554

Uh oh!

Conversation

xabbuh commented Jan 30, 2015

Uh oh!

stof commented Jan 30, 2015

Uh oh!

xabbuh commented Jan 30, 2015

Uh oh!

stof commented Jan 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jan 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Mar 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Mar 27, 2015

Uh oh!

Tobion commented Mar 27, 2015

Uh oh!

xabbuh commented Mar 27, 2015

Uh oh!

xabbuh commented Mar 31, 2015

Uh oh!

Tobion commented Mar 31, 2015

Uh oh!

aitboudad commented Mar 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Apr 3, 2015

Uh oh!

King2500 commented Apr 6, 2015

Uh oh!

smolinari commented Apr 9, 2015

Uh oh!

Uh oh!