Follow-up to #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests
LocaleJavascriptTranslationTest was omitted from the scope of #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests includes test coverage for !placeholder use with (it appears) translated string discovery from JavaScript. We need to evaluate this test (and the functionality that it tests) in light of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand.
Code from original issue:
+++ b/core/modules/link/src/Tests/LinkFieldTest.php
--- a/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
+++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
+++ b/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
@@ -58,7 +58,7 @@ public function testFileParsing() {
"Double Quote \"Escaped\" t" => '',
'Double Quote Concat strings t' => '',
- 'Context !key Args t' => 'Context string',
+ 'Context @key Args t' => 'Context string',
'Context Unquoted t' => 'Context string unquoted',
'Context Single Quoted t' => 'Context string single quoted',
@@ -73,7 +73,7 @@ public function testFileParsing() {
@@ -73,7 +73,7 @@ public function testFileParsing() {
"Double Quote plural{$etx}Double Quote @count plural" => '',
"Double Quote \"Escaped\" plural{$etx}Double Quote \"Escaped\" @count plural" => '',
- "Context !key Args plural{$etx}Context !key Args @count plural" => 'Context string',
+ "Context @key Args plural{$etx}Context @key Args @count plural" => 'Context string',
"Context Unquoted plural{$etx}Context Unquoted @count plural" => 'Context string unquoted',
TBD
None
Probably none.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | replace_placeholder-2567727-7.patch | 2.66 KB | sutharsan |
Comments
Comment #2
xjmThanks @Sutharsan!
Let's split this into two issues please, one for the locale/JS thing and one for the views thing. The unit test will be covered by the patch that actually deprecates or removes the placeholder once we decide which of those we're doing, so no need to have a separate issue for it.
Comment #3
sutharsan commentedComment #4
sutharsan commentedIssue splitted.
Comment #5
xjmComment #6
sutharsan commentedComment #7
sutharsan commentedLets start with getting this test working...
Comment #8
sutharsan commentedBut that made me realize that we overlooked (did not look at) the placehoders in JavaScript. Our Drupal.formatString() still uses '@', '!' and '%'. Just removing the test for '!' is not enough. If we want a ':' placeholder for url's in JS too, we need a test for that too.
Comment #9
sutharsan commentedComment #10
dawehnerAs long the other issue is not done yet I would actually suggest to ADD test coverage for @placeholder and keep the existing one for !. This way this issue can be committed without
being blocked on the other one.
Comment #11
sutharsan commented::testFileParsing() is about finding of translatable strings when parsing a JS file. Not about placeholder replacement, not even about finding placeholders. It only checks if a string and its context is found.
To me the usage of '@' or '!' in the placeholder of this test is merely an example and documenting that we have this kind of placeholder. IMHO there is no need that this issue should be blocked by placeholder replacement issue (or vice versa).
Comment #12
dawehnerWell you still get rid of all the !placeholder cases ... and by that we loose a tiny little bit of test coverage. ... Maybe its just me.
Comment #13
joelpittet@Sutharsan and @dawehner I think you are both saying the same thing.
We need to:
Does that work for you both or do you disagree with these steps forward?
Comment #14
dawehner@joelpittet Agreed on you
Comment #15
joelpittetAdded steps to the Issue summary.
Comment #26
catchThe test has moved (core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php), but this is still valid. Needs a new patch based on #13.
Comment #27
catch