Don't overwrap WordPress i18n functions#1753
Merged
jarednova merged 2 commits intotimber:masterfrom Jul 26, 2018
Merged
Conversation
Normally, when the argument of a Twig Function is a Callable, Twig creates such call (see Twig's `compileCallable()`).
Eg:
```
{{__("foo")}}
=> echo twig_escape_filter($this->env, call_user_func_array($this->env->getFunction('__')->getCallable(), array("foo")), "html", null, true)
```
This has the side effect that effect resulting template loose the xgettext friendly syntax.
By using feeding string to Twig Function, the result is clearer (to xgettext):
```
{{__("foo")}}
=> echo twig_escape_filter($this->env, __("foo"), "html", null, true);
````
Since wrapping these functions is of no use (same arguments), and since compiled templates containing these functions
may very well have to be looked into for gettext*() function calls, it's better to avoid it.
This was referenced Jul 20, 2018
Codecov Report
@@ Coverage Diff @@
## master #1753 +/- ##
============================================
+ Coverage 94.83% 94.91% +0.07%
+ Complexity 1536 1525 -11
============================================
Files 48 48
Lines 3604 3582 -22
============================================
- Hits 3418 3400 -18
+ Misses 186 182 -4
Continue to review full report at Codecov.
|
Member
|
WOW is that cleaner. I added a test in #1759 to confirm everything still works — and it does. Merged! |
drzraf
pushed a commit
to drzraf/Gettext
that referenced
this pull request
Oct 28, 2018
Current Twig parser operate by 1. Transforming Twig as PHP code = tokenize() + parse() + render() 2. Using a PhpCode extractor (and its specific configuration about function name) Disadvantage: * Twig rendered PHP code can be transformed/wrapped in call_user_func() making that gettext functions undetectable by PhpCode extractor. (See for example timber/timber#1753). * Can't handle templates making use of custom Twig extensions (very common) This patch offer an extractor that: * Parse Twig generated AST tree (= tokenize()+parse()) * Recursively iterate over node three to find function gettext calls. Advantages: * Operating sooner, at the AST level, Twig expressions like `{{ __("foo", "domain") }}` aren't yet wrapped. * More robust because it directly iterates over the AST from Twig parser instead of looking at PHP source-code. * Supports initialized `$twig` environment, thus supporting user-defined extensions? * Possibly more efficient. Ref: wp-cli/i18n-command#59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: #
Issue
Normally, when the argument of a Twig Function is a Callable, Twig creates such call (see Twig's
compileCallable()).Eg:
This has the side effect that resulting template loose the xgettext friendly syntax.
Solution
By feeding string to Twig Function, the result is clearer (to xgettext):
Impact
Since wrapping these functions is of no use (same arguments), and since compiled templates containing these functions may very well have to be looked into for gettext*() function calls, it's better to avoid it.
No impact is expected (other than reducing function calls)
Usage Changes
no
Considerations
Testing
no