In time of enabling the "Testing" module in Drupal 8 if the requirements for this module is not matched then some error messages are shown. If there are any issues related to permission in the "simpletest" directory created by this module then the errors shown have double escaped string.
This one is one of many issues related to [meta] Fix double-escaping due to Twig autoescape.
Possible Solution:
Fixing this with New inline_template implementations.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Add steps to reproduce the issue | Novice/needed? | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#25 | simpletest_module-2319667-25.patch | 1.4 KB | aneek |
#25 | interdiff-2319667-18-25.txt | 1.13 KB | aneek |
#18 | simpletest_module-2319667-18.patch | 1.41 KB | aneek |
#18 | interdiff-2319667-15-18.txt | 716 bytes | aneek |
#15 | simpletest_module-2319667-15.patch | 1.24 KB | aneek |
Comments
Comment #1
aneek CreditAttribution: aneek commentedComment #2
aneek CreditAttribution: aneek commentedComment #3
aneek CreditAttribution: aneek commentedComment #4
roderikTagging Amsterdam2014 + Novice because I think the issue has at least one novice task in it. Here's my impression:
Please decide for yourself if "Add steps to reproduce the issue" is needed. If you cannot see easily from the description/screenshot, this is neeeded.
If you can in fact reproduce it easily, you can probably find the affected strings in the source code. Creating the patch may not be that hard, because the suggested solution (inline_templates), as defined in the linked page, is well defined.
You may be able to find another person (novice contributor) to review/discuss if 'inline_templates' is actually the right solution to implement, after reading the linked page.
Comment #5
dankh CreditAttribution: dankh commentedWe are reproducing the issue dankh, henkit.
Comment #6
dankh CreditAttribution: dankh commentedTo reproduce the issue
Error message after installing the Testing module
We will work on a patch.
Comment #7
dankh CreditAttribution: dankh commentedFirst we tried fixing the issue in the simpletest module itself, but after discussing the issue with Joel Pittet (@joelpittet) he pointed us out that the issue is more general and a better way to fix it is to modify the way strings are printed by drupal_check_module in install.inc.
In drupal_check_module requirements description strings get concatenated. Some of them are safe and some are not. When the strings get concatenated they are all marked as not safe. This is why tags in these strings get escaped and they are printed as is in the error message. This patch checks if the description string is safe and if it is it flags the whole concatenated string as safe.
This patch could also fix similar issues in other hook_requirements implementations.
People participating in this patch : @dankh, @henkit, @joelpittet .
Comment #8
joelpittetThis is super minor but this should move down 3 for alphabetical.
This can be compressed a bit if you move the if condition directly around the SafeMarkup::set()
Comment #9
vurt CreditAttribution: vurt commentedI start to work on this...
Comment #10
vurt CreditAttribution: vurt commentedWe applied the stuff joel suggested in #8.
the patch is a lot shorter now ;)
We attached the patch, an interdiff and an image showing all possible requierement error messages with HTML tags.
People participating in this patch : @dankh, @henkit, @vurt, @gngn, @joelpittet.
Comment #11
aneek CreditAttribution: aneek commented@vurt & @joelpittet, should we be really using
SafeMarkup::set()
?Comment #12
vurt CreditAttribution: vurt commentedWhat do you suggest as an alternative?
Comment #13
joelpittetThanks I think this is a more global fix for all. Thanks everybody for going through it!
Hope you found it helpful to get a better understanding on how we are keeping the theme layer as safe as we can.
@aneek The only variable that is unknown in that equation is the 'description' key. We only mark the complete string as safe if that variable is also safe. I'm quite positive this is the best way to deal with this.
I'm marking this to "needs work" because to prevent people questioning we should add a comment above the condition to that effect. Could one of you assign yourself and add that comment?
Comment #14
aneek CreditAttribution: aneek commented@joelpittet, okay got your point. Indeed we are also using
SafeMarkup::isSafe
so I think its quite a good approach.Thanks @vurt for the patch. I can add a comment to the code block.
Comment #15
aneek CreditAttribution: aneek commentedGuys here is another approach.
Instead of using
SafeMarkup::isSafe()
, I've usedSafeMarkup::escape()
. This function will do the safe checks and also if the string is not safe then sanitize it also. Please see SafeMarkup::escape for more reference.Attaching a patch for review.
Comment #16
aneek CreditAttribution: aneek commentedComment #17
joelpittetThanks Aneek, that does the same thing in a different way. Can you put a comment above the SafeMarkup::set() because that is the likely scrutinized but and we'd prefer this doesn't get reverted by a misunderstanding. Otherwise both are RTBC. Thanks for the iterations and finding the bug!
Comment #18
aneek CreditAttribution: aneek commentedSure @joelpittet. I've added comment just before the
SafeMarkup::set()
. Please have a look at the diff and let me know if the comment is understandable or needs more details.Thanks!
Comment #19
aneek CreditAttribution: aneek commentedComment #20
joelpittetI think the comment gives a good enough indication. @vurt or @dankh if you have any additions to that comment to help clarify or clean up, please feel free to submit a patch or suggestion.
In the mean time I'm marking this as RTBC to get it in. Thanks again all!
Comment #21
vurt CreditAttribution: vurt commentedI tested the new patch and it works as expected.
Comment #22
joelpittetAny suggestions on the comment explaining the SafeMarkup::set @vurt?
Comment #23
vurt CreditAttribution: vurt commentedThe comments make sense for me (but I must add that I am no expert on this topic...).
There is a typo: "Prepair" -> "Prepare" in one comment.
Comment #24
joelpittetNice catch, we can remove that comment. We want it to be succinct and clear as to what we are doing.
Couple things I was thinking too:
We can likely remove this comment and it's spelled wrong as @vurt pointed out.
This can be removed as well, I think it doesn't add much.
Maybe add " and the concatinated string is desginated safe by running through the t() function."
Comment #25
aneek CreditAttribution: aneek commentedThanks @joelpittet & @vurt for your valuable feedbacks. I've addressed those mentioned points in the patch. Please verify.
Thanks!
Comment #26
aneek CreditAttribution: aneek commentedComment #27
vurt CreditAttribution: vurt commentedI tested again - output is still fine.
Comment #28
joelpittetPerfect! Thanks for your patience. Let's get it in:)
Comment #29
alexpottDiscussed with @chx on IRC since we are trying to limit the number of calls to
SafeMarkup::set()
but this is the installer and no one should be copying code from here.Committed d96918c and pushed to 8.0.x. Thanks!
Comment #32
jibran