Suggested commit message:
Issue #2319233 by Sutharsan, sardara, ChristianAdamski, akoe: Fixed visible HTML on Available translation updates page.
Problem/Motivation
Double escaped string in Available translation update text. See screenshot:
Proposed resolution
Mark string as safe.
Steps to reproduce
1. Install drupal 8. Make sure automatic updates are switched off.
2. Enable Language module.
3. Enable Interface translation.
4. Add a language on admin/config/regional/language
5. Enable the Update manager module.
6. Go to admin/reports/translations, the status of the added language is "Missing translations for one project
Drupal core (8.0.0-beta4). File not found at http://ftp.drupal.org/files/translations/8.x/drupal/drupal-8.0.0-beta4.h... nor at translations://drupal-8.0.0-beta4.hu.po" or similar.
Remaining tasks
Decide whether variables containing safe string(s) should be renamed, or variables/logic should be moved from the preprocess to the the template.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Novice | Instructions | |
Manually test the patch | Novice | Instructions | |
Add steps to reproduce the issue | Novice | Instructions | |
Embed |
Novice | Instructions | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#43 | missing-translation.jpg | 147.35 KB | balagan |
#31 | screenshot-elmo 2015-01-15 10-12-55.png | 85.28 KB | andythomnz |
Comments
Comment #1
Sutharsan CreditAttribution: Sutharsan commentedAlthough, this patch has a working solution, I am not yet convinced that it is a proper one. The string which is marked safe is a concatenation of a translated string and a variable
$update['info']
. The latter is the result ofTranslationStatusForm::createInfoString()
. Should we improve the code by renaming the variables to$update['safe_info']
and the method toTranslationStatusForm::createSafeInfoString()
?Comment #2
Sutharsan CreditAttribution: Sutharsan commentedComment #3
oriol_e9gI think that you can use an inline template:
From:
To:
Comment #4
Sutharsan CreditAttribution: Sutharsan commentedI'm not convinced that an inline template for the final string is worth while. It would look like this:
I've made a slight change to the #1 patch. The 'info' must be inside the translatable string, otherwise it would fail with RTL languages.
Comment #6
Sutharsan CreditAttribution: Sutharsan commentedOn second thought, this is not even a "double escape" problem, it is actually a wrong string concatenation.
Comment #7
roderikMarking for Amsterdam. The patch is reviewable by experienced Drupal(contrib) people who know t(). Other tasks can be done by everyone (like updating the issue summary for the committers, because it has changed direction).
Comment #8
sardara CreditAttribution: sardara commentedI could reproduce the bug in my local environment. After applying the last patch, the bug is resolved.
See screenshot:
The path follows the coding standards.
Comment #10
sardara CreditAttribution: sardara commentedSince the core has changed, the patch doesn't apply anymore.
Re-rolled against latest head.
Comment #11
ChristianAdamski CreditAttribution: ChristianAdamski commentedOh, I just found this issue :(
We solved the same thing at https://www.drupal.org/node/2349749
Difference is: we used an inline_template.
I am not sure, which solution is the better way to go.
Comment #13
akoe CreditAttribution: akoe commentedJust tested this solution with patch #10 locally, it worked and successfully solved the issue.
We looked into possible solutions in https://www.drupal.org/node/2349749 and actually i prefer this solution since it seem easier.
Comment #14
roderikThose errors seem random. Re-testing...
Comment #16
star-szrAdding parent issue relationship.
Comment #17
jibranComment #18
roderikOK; since this patch was already needs-review in #10 (the failure was indeed 'random' at busy Drupalcon A'dam) and people agree that this is the best solution...
... I did a quick review and can set RTBC, I think. Patch still applies with minimal offset + fuzz, and with a 2-line patch that should be OK.
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedComment #21
lauriiiComment #22
roderikOK I'll remember to reroll patches with minimal offset + fuzz next time. For the rest; as #18.
(checked things are still the same.)
Comment #23
alexpottI'm not sure why we are using t() here.
Comment #24
subhojit777Comment #25
subhojit777Using String::format()
Comment #26
lauriiiUsing String::format is a good change. I was about to RTBC this but only thing I'm concerned still is whether we can use multi line array syntax while setting parameter for the function call or not.
Comment #27
Gábor HojtsyWhy would String::format() not do the same escaping as t() does? I don't see how this is fixing the issue?
BTW the multiline array syntax is no problem.
Comment #28
subhojit777We are not translating anything here. We are showing some strings in placeholder. I do not think we should use t() for that, String::format() would suffice in this case. That is what @alexpott told in #23 and we talked about this in IRC.
Comment #29
Gábor HojtsyThat does not answer my question. '@module (@date)' should escape the module name and date for output the same as it would in t() that you are removing. Switching from t() to String::format() should not change how the values are escaped and therefore should not fix this bug. Or what am I missing?
Comment #30
balagan CreditAttribution: balagan commentedComment #31
andythomnz CreditAttribution: andythomnz commentedHi, I have installed some modules in my Drupal environment and applied the patch. It appears to solve the problem, although I also don't understand why, Gábor.
Screenshot attached
Comment #32
herom CreditAttribution: herom commentedThis is to answer @Gábor's question of how the bug is being fixed.
This change is out-of-scope of this issue. It's ok to commit this too, but this string was escaped correctly before and after the patch.
The change that fixes this issue is here, by moving the "!info" part into
String::format
(ort()
, makes no difference). The double-escaping happened because only the'@module (@version).'
part of the string was being marked as safe int()
, and when the whole string was printed, it was unrecognized and escaped again.Comment #33
Gábor HojtsySo I needed to go one step further in understanding. String::format() adds the string to the SafeMarkup storage, so it will not be escaped later. So bringing strings under that storage protects them from being escaped later again. Agreed with the RTBC then.
Comment #34
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed f758884 and pushed to 8.0.x. Thanks!
Comment #36
Gábor HojtsySuperb, thanks all!
Comment #37
subhojit777@herom++ Nice explanation. Thanks!!
Comment #38
Gábor HojtsyI don't think this works anymore, seeing #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument landed.
Comment #39
subhojit777Comment #40
Gábor HojtsyComment #41
DevElCuy CreditAttribution: DevElCuy commentedComment #42
DevElCuy CreditAttribution: DevElCuy commentedRemoved tag by mistake.
Comment #43
balagan CreditAttribution: balagan commentedThe issue reported is still gone with the committed patch. See image.
Comment #44
balagan CreditAttribution: balagan commentedComment #45
Gábor HojtsyThat surprised me given #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument, but all right :)
Comment #46
balagan CreditAttribution: balagan commentedComment #47
balagan CreditAttribution: balagan commented