PHP 5.4 introduces a new flag to htmlentities()
"which makes htmlentities()/htmlspecialchars() replace the invalid multibyte sequences with U+FFFD (UTF-8) or � (other encodings).
In a nutshell, all the invalid character sequences are replaced by "�" instead of discarding the whole thing. That would make debugging way easier in many cases where data is fed into Drupal in non-UTF8 (by the database incorrectly returning non-UTF8 errors, by system extensions, on data import, etc. examples of this type of issue are numerous).
Twig's autoescape uses this internally.
https://github.com/twigphp/Twig/blob/641090378dfff2913929306fbbf227269ba...
Beta phase evaluation
Issue category | Task because it improves DX and consistency with Twig's escape |
---|---|
Issue priority | Major because our Twig escaping will produce different results than Html::escape flags |
Unfrozen changes | Unfrozen because it only changes markup. |
Prioritized changes | The main goal of this issue is security and consistency |
Disruption | Not disruptive as it will be doing what Twig is already doing and invalid characters will encode into something visible |
Comment | File | Size | Author |
---|---|---|---|
#28 | 1211866-28.patch | 5.5 KB | stefan.r |
#28 | interdiff-21-28.txt | 3.18 KB | stefan.r |
#24 | 1211866-21.patch | 4.66 KB | stefan.r |
#24 | interdiff-20-21.txt | 2.88 KB | stefan.r |
#23 | 1211866-20.patch | 1.51 KB | stefan.r |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedTagging.
Comment #2
sunIf this works reliably, then it would be even be a major reason for bumping PHP requirements to 5.4.
Comment #3
DamienMcKennaI wouldn't recommend mandating PHP 5.4 be required for D8, but this definitely could be a good optional feature for when PHP 5.4 is available.
Comment #4
tsphethean CreditAttribution: tsphethean commentedDo we have a baked in means of detecting PHP version, or would something like
be appropriate?
Comment #5
tsphethean CreditAttribution: tsphethean commentedOn the assumption the above is ok i've rolled this patch. Tests pass on my local on php 5.3.25 and 5.4.15 - not entirely sure what to do about writing additional unit tests to cater for this. Do we want php version checking logic in our unit tests, or do we just case that the cases are what we expect?
Comment #6.0
(not verified) CreditAttribution: commentedFix preview
Comment #7
joelpittetPHP 5.4 is already a requirement and Twig already does this so it would also keep things in line with Twig's escape filter.
https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Extension/Core.php#L1011
+1 to this issue.
Comment #8
joelpittetRe-rolled.
Comment #9
joelpittetComment #11
alexpottThis issue seems like a really good idea. Postponing on #2550945: Add Html::escape()
Comment #12
joelpittetFYI twig is using this internally so another reason to have this.
https://github.com/twigphp/Twig/blob/641090378dfff2913929306fbbf227269ba...
Comment #13
stefan.r CreditAttribution: stefan.r commentedComment #14
stefan.r CreditAttribution: stefan.r commentedComment #15
stefan.r CreditAttribution: stefan.r commentedComment #16
alexpottLet's not add this documentation to the bit about how this method is different from Html::decodeEntities. It belongs in the list of conversions above.
Comment #17
stefan.r CreditAttribution: stefan.r commentedThe list is a list of entity equivalents though, if we want it to be in the list we could drop the " with their HTML entity equivalents" bit?
Comment #18
stefan.r CreditAttribution: stefan.r commenteda newline crept in
Comment #19
alexpottComment #20
alexpottI think it should be "replacement character" - too many capitals :). I like where these docs are now. Thanks.
Comment #21
stefan.r CreditAttribution: stefan.r commentedComment #22
stefan.r CreditAttribution: stefan.r commentedComment #23
stefan.r CreditAttribution: stefan.r commentedComment #24
stefan.r CreditAttribution: stefan.r commentedFixing some more test fails
Comment #27
joelpittetAwesome! Thanks for jumping on this after the HTML::escape blocker was in.
Comment #28
stefan.r CreditAttribution: stefan.r commentedclarifying some comments
Comment #35
joelpittetThank you, green means go.
Comment #36
alexpottNormal tasks need a beta evaluation.
Comment #37
joelpittetComment #38
alexpottCommitted 00360b9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #40
Wim LeersWow, this landed fast! Great job, @stefan.r :)