[FrameworkBundle] fixed custom domain for translations in php templates #21359

Merged
merged 1 commit into from Feb 3, 2017

Projects

None yet

6 participants

@robinlehrmann
Contributor
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20135
License MIT
Doc PR
@xabbuh xabbuh added the Translator label Jan 20, 2017
@@ -37,7 +39,29 @@ class PhpExtractor extends AbstractFileExtractor implements ExtractorInterface
*
* @var array
*/
- protected $sequences = array(
+ protected static $sequences = array(
@xabbuh
xabbuh Jan 21, 2017 Member

The static change should be reverted.

@@ -85,7 +109,7 @@ public function setPrefix($prefix)
*/
protected function normalizeToken($token)
{
- if (isset($token[1]) && 'b"' !== $token) {
+ if ('b"' !== $token && isset($token[1])) {
@xabbuh
xabbuh Jan 21, 2017 Member

IMO this should be reverted as it is not part of the bug fix.

@@ -105,13 +129,34 @@ private function seekToNextRelevantToken(\Iterator $tokenIterator)
}
}
+ private function seekToMessageParamsEnd(\Iterator $tokenIterator)
@xabbuh
xabbuh Jan 21, 2017 Member

maybe better name this seekBehindMessageParams

@xabbuh
xabbuh Feb 3, 2017 Member

I would now rename this too (something like skipMethodArgument())

*/
- private function getMessage(\Iterator $tokenIterator)
+ private function provideRawString(\Iterator $tokenIterator)
@xabbuh
xabbuh Jan 21, 2017 Member

What about getStringValue() instead?

@@ -153,26 +198,38 @@ protected function parseTokens($tokens, MessageCatalogue $catalog)
$tokenIterator = new \ArrayIterator($tokens);
for ($key = 0; $key < $tokenIterator->count(); ++$key) {
- foreach ($this->sequences as $sequence) {
+ foreach (self::$sequences as $sequence) {
@xabbuh
xabbuh Jan 21, 2017 Member

Must then be reverted too.

+ '(',
+ self::MESSAGE_TOKEN,
+ ',',
+ T_LNUMBER,
@aitboudad
aitboudad Jan 21, 2017 Member

it can be an expression or function call

@robinlehrmann
robinlehrmann Jan 21, 2017 Contributor

I added a new test with array_map function and the tests passes. Can you explain what you mean please :) ?

@aitboudad
aitboudad Jan 21, 2017 Member
<?php echo $view['translator']->transChoice('msg', 10 + 1, [], 'not_messages'); ?>
<?php echo $view['translator']->transChoice('msg', intval(4.5), [], 'not_messages'); ?>
@robinlehrmann
robinlehrmann Jan 21, 2017 Contributor

Okay, I added this lines and the Tests don't pass. Thank you!
I will fix it asap.

@xabbuh
xabbuh Jan 22, 2017 Member

I guess you can use the same approach here that is already used for skipping the message params.

+ for (; $tokenIterator->valid(); $tokenIterator->next()) {
+ $t = $tokenIterator->current();
+
+ if ('[' === $t[0] || '(' === $t[0]) {
@aitboudad
aitboudad Jan 21, 2017 Member

you should not take account of braces inside a string

->trans('msg', [ 'p' => '[ \'' ], 'not_messages')
@robinlehrmann
robinlehrmann Jan 21, 2017 Contributor

I added this line to my tests too and it passes.

@xabbuh
xabbuh Jan 22, 2017 Member

@aitboudad In your example, $t will be an array consisting of three elements where the value at index 0 is T_CONSTANT_ENCAPSED_STRING if I am not mistaken.

@robinlehrmann

I added some tests and it passes.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jan 24, 2017
@robinlehrmann
Contributor

Hi, @aitboudad @xabbuh
I've fixed the issue and added some tests for it. Could you review my changes please ?

@@ -24,6 +24,8 @@
class PhpExtractor extends AbstractFileExtractor implements ExtractorInterface
{
const MESSAGE_TOKEN = 300;
+ const MESSAGE_ARGUMENTS_TOKEN = 1000;
@xabbuh
xabbuh Feb 3, 2017 Member

I would name this METHOD_ARGUMENTS_TOKEN

@robinlehrmann robinlehrmann [FrameworkBundle] fixed custom domain for translations in php templates
78c0ec5
@xabbuh
xabbuh approved these changes Feb 3, 2017 View changes

👍

Status: Reviewed

@fabpot
Member
fabpot commented Feb 3, 2017

Thank you @robinlehrmann.

@fabpot fabpot merged commit 78c0ec5 into symfony:2.7 Feb 3, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Feb 3, 2017
@fabpot fabpot bug #21359 [FrameworkBundle] fixed custom domain for translations in …
…php templates (robinlehrmann)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] fixed custom domain for translations in php templates

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

Commits
-------

78c0ec5 [FrameworkBundle] fixed custom domain for translations in php templates
ff33768
This was referenced Feb 6, 2017
@robinlehrmann robinlehrmann deleted the robinlehrmann:issue-20135 branch Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment