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

Replace %count% with a given number out of the box #19795

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
8 participants
@bocharsky-bw
Copy link
Contributor

commented Aug 31, 2016

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

We already have this feature for transchoice Twig filter, but why only for it? It will be consistent to have this for translator in general. We already have a $number parameter in transChoice() which we could easily use for that.

Before

$this->get('translator')
    ->transChoice('1 apple|%count% apples', 7, [
        '%count%' => 7,
    ]);

After:

$this->get('translator')
    ->transChoice('1 apple|%count% apples', 7);
@@ -95,7 +95,7 @@ public function trans($message, array $arguments = array(), $domain = null, $loc
public function transchoice($message, $count, array $arguments = array(), $domain = null, $locale = null)
{
return $this->translator->transChoice($message, $count, array_merge(array('%count%' => $count), $arguments), $domain, $locale);
return $this->translator->transChoice($message, $count, $arguments, $domain, $locale);

This comment has been minimized.

Copy link
@jvasseur

jvasseur Aug 31, 2016

Contributor

You need to keep this in case the twig bridge is used with an older version of the translator.

This comment has been minimized.

Copy link
@bocharsky-bw

bocharsky-bw Aug 31, 2016

Author Contributor

You're right, thanks!

@@ -198,6 +198,10 @@ public function trans($id, array $parameters = array(), $domain = null, $locale
*/
public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null)
{
$parameters = array_merge(array(

This comment has been minimized.

Copy link
@inso

inso Aug 31, 2016

Contributor

Why not?
$parameters['%count%'] = $number;

This comment has been minimized.

Copy link
@bocharsky-bw

bocharsky-bw Aug 31, 2016

Author Contributor

Good question! Because of this way we allow devs to override this parameter with their own value. I mean something like this:

print $this->get('translator')
    ->transChoice('1 apple|%count% apples', 7, [
        '%count%' => 'no'
    ]); // will print "no apples"

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Aug 31, 2016

Member

What about:

$parameters += array('%count%' => $number);

?

This comment has been minimized.

Copy link
@rybakit

rybakit Aug 31, 2016

Contributor

@ogizanagi See this comment: #7029 (comment)

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

This new behavior can be tested by updating the TranslatorTest::getTransChoiceTests and removing %count% from the given parameters.
Also a new tests case should ensure that the %count% placeholder is overwritable if passed as parameter :)

@bocharsky-bw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

Hey @ogizanagi , thanks for your help!

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

Thank you @bocharsky-bw.

@fabpot fabpot closed this Sep 14, 2016

fabpot added a commit that referenced this pull request Sep 14, 2016

feature #19795 Replace %count% with a given number out of the box (bo…
…charsky-bw)

This PR was squashed before being merged into the 3.2-dev branch (closes #19795).

Discussion
----------

Replace %count% with a given number out of the box

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

We already have this feature for [transchoice](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/TranslationExtension.php#L98) Twig filter, but why only for it? It will be consistent to have this for translator in general. We already have a `$number` parameter in `transChoice()` which we could easily use for that.

Before
```php
$this->get('translator')
    ->transChoice('1 apple|%count% apples', 7, [
        '%count%' => 7,
    ]);
```

After:
```php
$this->get('translator')
    ->transChoice('1 apple|%count% apples', 7);
```

Commits
-------

4c1a65d Replace %count% with a given number out of the box

@bocharsky-bw bocharsky-bw deleted the bocharsky-bw:patch-1 branch Sep 14, 2016

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

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.