Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] fixed custom domain for translations in php templates#21359
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
robinlehrmann commentedJan 20, 2017
| 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 |
| * @var array | ||
| */ | ||
| protected$sequences =array( | ||
| protectedstatic$sequences =array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thestatic change should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| for ($key =0;$key <$tokenIterator->count(); ++$key) { | ||
| foreach ($this->sequencesas$sequence) { | ||
| foreach (self::$sequencesas$sequence) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Must then be reverted too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| } | ||
| } | ||
| privatefunctionseekToMessageParamsEnd(\Iterator$tokenIterator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe better name thisseekBehindMessageParams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would now rename this too (something likeskipMethodArgument())
| protectedfunctionnormalizeToken($token) | ||
| { | ||
| if (isset($token[1]) &&'b"' !==$token) { | ||
| if ('b"' !==$token &&isset($token[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO this should be reverted as it is not part of the bug fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| * match allowed tokens. | ||
| */ | ||
| privatefunctiongetMessage(\Iterator$tokenIterator) | ||
| privatefunctionprovideRawString(\Iterator$tokenIterator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutgetStringValue() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| for (;$tokenIterator->valid();$tokenIterator->next()) { | ||
| $t =$tokenIterator->current(); | ||
| if ('[' ===$t[0] ||'(' ===$t[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you should not take account of braces inside a string
->trans('msg', ['p' =>'[\'' ],'not_messages')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added this line to my tests too and it passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@aitboudad In your example,$t will be an array consisting of three elements where the value at index 0 isT_CONSTANT_ENCAPSED_STRING if I am not mistaken.
| '(', | ||
| self::MESSAGE_TOKEN, | ||
| ',', | ||
| T_LNUMBER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it can be an expression or function call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a new test with array_map function and the tests passes. Can you explain what you mean please :) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
<?phpecho$view['translator']->transChoice('msg',10 +1, [],'not_messages');?><?phpecho$view['translator']->transChoice('msg',intval(4.5), [],'not_messages');?>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, I added this lines and the Tests don't pass. Thank you!
I will fix it asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess you can use the same approach here that is already used for skipping the message params.
robinlehrmann left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added some tests and it passes.
robinlehrmann commentedFeb 3, 2017
Hi,@aitboudad@xabbuh |
| class PhpExtractorextends AbstractFileExtractorimplements ExtractorInterface | ||
| { | ||
| constMESSAGE_TOKEN =300; | ||
| constMESSAGE_ARGUMENTS_TOKEN =1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would name thisMETHOD_ARGUMENTS_TOKEN
xabbuh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Status: Reviewed
fabpot commentedFeb 3, 2017
Thank you@robinlehrmann. |
…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