Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Apr 3, 2015

Conversation

Projects
None yet
8 participants
@xabbuh
Copy link
Member

commented Jan 30, 2015

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: .

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch from 43439c7 to 98ca178 Jan 30, 2015

@stof

This comment has been minimized.

Copy link
Member

commented Jan 30, 2015

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

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2015

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

@stof

This comment has been minimized.

Copy link
Member

commented Jan 30, 2015

@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
->arrayNode('number_format')
->addDefaultsIfNotSet()
->children()
->scalarNode('decimals')->defaultValue(0)->end()

This comment has been minimized.

Copy link
@stof

stof Jan 30, 2015

Member

should be an integer node

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch from 98ca178 to 58c1ff5 Jan 30, 2015

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2015

@stof Thanks for pointing me towards service configurators.

@@ -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">

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 30, 2015

Author Member

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

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 30, 2015

Member

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

This comment has been minimized.

Copy link
@stof

stof Jan 30, 2015

Member

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 30, 2015

Author Member

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

This comment has been minimized.

Copy link
@stof

stof Jan 30, 2015

Member

@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)

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 30, 2015

Author Member

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 24, 2015

Author Member

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

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 25, 2015

Member

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

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 27, 2015

Author Member

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?

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 27, 2015

Member

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

@fabpot fabpot added the TwigBundle label Feb 5, 2015

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

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

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 23, 2015

Member

should be documented that null means date_default_timezone_get

->addDefaultsIfNotSet()
->children()
->scalarNode('format')->defaultNull()->end()
->scalarNode('interval_format')->defaultNull()->end()

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 23, 2015

Member

IMO this should specify the same defaults as in https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Extension/Core.php#L18 since you also added the defaults for number_format

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 24, 2015

Author Member

Imho using null here makes more sense since then the default format is not modified when calling setDateFormat() in the configurator.

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 25, 2015

Member

but having concrete defaults makes it easier to understand and also serves as an example value. furthermore, setNumberFormat does not support null. so for it the default is overwritten anyway. this is why is should be done for both consistently.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 27, 2015

Author Member

Changed that too.

{
$rootNode
->children()
->arrayNode('date')

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 23, 2015

Member

should add info that this section configures the default behavior of the date filter

->scalarNode('timezone')->defaultNull()->end()
->end()
->end()
->arrayNode('number_format')

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 23, 2015

Member

should add info that this section configures the default behavior of the number_format filter

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 24, 2015

Author Member

done

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch 3 times, most recently from c7716eb to 82b249b Mar 24, 2015

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2015

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

@Tobion

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

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

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2015

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

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch from 82b249b to f4f3a7a Mar 27, 2015

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2015

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

@Tobion

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

👍

1 similar comment
@aitboudad

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2015

👍

$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']);

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 2, 2015

Member

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.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Apr 3, 2015

Author Member

done

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch from f4f3a7a to 5133a4f Apr 3, 2015

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

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 3, 2015

Contributor

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

<!-- Above arguments are set in TwigExtension -->
make date formats and number formats configurable
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: .
```

@xabbuh xabbuh force-pushed the xabbuh:issue-13552 branch from 5133a4f to e9bc23b Apr 3, 2015

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 3, 2015

Thank you @xabbuh.

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

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Doing some proofreading and checking your coding style.
Details

fabpot added a commit that referenced this pull request Apr 3, 2015

feature #13554 [TwigBundle] make date formats and number formats conf…
…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

@xabbuh xabbuh deleted the xabbuh:issue-13552 branch Apr 3, 2015

@King2500

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2015

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

This comment has been minimized.

Copy link

commented Apr 9, 2015

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

nicolas-grekas added a commit that referenced this pull request Jul 11, 2017

bug #23459 [TwigBundle] allow to configure custom formats in XML conf…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.