Add Html::escape()

Another reason to do this: the ability to invert Html::decodeEntities() with a signature you'd expect: "encode" instead of "decode".

Let's add tests and make it a child of the critical that definitely needs it.

Looks great for me!

One sec - we should replace all htmlspecialchars() in core too.

Here we go.

Filed #2551107: StylePluginBase should use Html::decodeEntities() that I spotted whilst doing this patch. Kinda related.

So let's do #1211866: Enable ENT_SUBSTITUTE flag in Html::escape once this lands - and it seems like a really good reason to do this issue.

Yes please, this blocks a bunch of work we'd like to clean up in other issues.

@catch regarding #12 it doesn't look default because it's loading from ini settings, so if we exclude it and they have a different setting in their ini, it would be less predictable, no?

Reviewed the patch and DrupalCI agrees.

With this patch, we would have:

  /**
   * Decodes all HTML entities including numerical ones to regular UTF-8 bytes.
   *
...
   */
  public static function decodeEntities($text) {
    return html_entity_decode($text, ENT_QUOTES, 'UTF-8');
  }

and a method that looks like the opposite:

  /**
   * Encodes all HTML entities.
...
   */
  public static function encodeEntities($text) {
    return htmlspecialchars($text, ENT_QUOTES, 'UTF-8');
  }

Just to make sure, is this what we want since the opposite of htmlspecialchars() is htmlspecialchars_decode(), and the opposite of html_entity_decode() is htmlentities()?

#16 that is a great point - thoughts everyone?

#16 is excellent.

However:

16:57:33 <WimLeers> alexpott: "html_entity_decode() is the opposite of htmlentities() " — http://php.net/manual/en/function.html-entity-decode.php
16:57:37 <WimLeers> alexpott: PHP--
16:58:10 <WimLeers> http://php.net/manual/en/function.htmlentities.php: "This function is identical to htmlspecialchars() in all ways, except with htmlentities(), all characters which have HTML character entity equivalents are translated into these entities."
16:58:32 <alexpott> WimLeers: so maybe we're okay
16:58:37 <WimLeers> yeah

So we're basically layering sanity on top of PHP's insanity, and the patch is correct AFAICT.

No harm in adding some documentation.

So here's why we use html_entity_decode and htmlspecialchars(). And is also another good reason to have these methods existing so all of core, contrib and custom take the same opinion.

@yesct spotted a grammar mistake.

+1, that makes a ton of sense.

This looks great, +1 to using those php functions... So Html::encodeEntities(Html::decodeEntities($text) != $text as it would still take out the unneeded HTML entity equivalents, but that's still leaving the string in a "cleaner" state.

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -338,6 +338,9 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
   +   * output UTF-8 which supports characters such as "é".
   

   Nit: s/output/outputs, and maybe we can mention é along with é just so it's totally clear what we mean?

2. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -348,4 +351,25 @@ public static function decodeEntities($text) {
   +   *   The input $text, with all HTML entities encoded.
   

   Nit: the input ($text)?

@stefan.r I don't see #24.1 in #21 maybe it was fixed?

No idea on the parens around $text in the return comment, haven't run into that convention before, likely doesn't need to even have the variable and could say 'The input text with all HTML entities encoded.'

RTBC++ change can be made on commit.

@alexpott and I just discussed this at length; I don't have time to summarize the discussion unfortunately but @alexpott is going to update the patch and/or post.

@xjm expressed concerns that Html::encodeEntities() is not the opposite of Html::decodeEntities() and that it is possible the following code does not return TRUE.

$encoded = Html::encodeEntities($string);
$decoded = Html::decodeEntities($encoded);
return $string === $decoded;

That is highly problematic code.

Plus it becomes clear that calling the method encodeEntities() when you read the docs in the patch:

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -348,4 +351,25 @@ public static function decodeEntities($text) {
+  /**
+   * Encodes all HTML entities.
+   *

Nope it does not do this at all.

However the symmetrically of encode/decode is not the primary reason to do this patch. PHP adds features to htmlspecialchars() that we want to use like #1211866: Enable ENT_SUBSTITUTE flag in Html::escape and having a central place makes this simple for core and possible for contrib/custom.

After further discussion with @xjm and @ksenzee we decided the name should be something descriptive about what the method actually does. The new patch goes for escapeSpecialChars(). The word escape is used because that is what is happening and specialChars because it is only special characters and to map to htmlspecialchars() for those familiar with PHP. However given the importance of getting this name correct - maybe people have better ideas? Zend's Escaper uses escapeHtml(), the Twig filter uses escape - so perhaps we should drop the SpecialChars. This would clash with SafeMarkup::escape() (which does not always escape - magic :) ) but we have plans to remove that in #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping.

The patch also adds test coverage of the relationship between Html::decodeEntities() and Html::escapeSpecialChars() to prove the fact that they are not opposites.

Updated IS

+1, this is better! I think the method name is fine (otherwise ::convertSpecialChars?)

Maybe we could clarify the list of conversions that htmlspecialchars() does in the docs, I think there's only 4 or 5.

re #32 I don't think we need to be that specific - what happens (very very unlikely) if htmlspecialchars() escapes a new character in PHP 7.4?

    /**
     * Escape a string for the HTML Body context where there are very few characters
     * of special meaning. Internally this will use htmlspecialchars().
     *
     * @param string $string
     * @return string
     */
    public function escapeHtml($string)

That's Zend's documentation

Here's the Twig documentation which http://twig.sensiolabs.org/doc/filters/escape.html

And for complete-ness our old check_plain() docs https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/chec...

What is common between the documentation of both Zend and Twig is that they both point out that htmlspecialchars() is used internally.

The patch attached adds this and improves the one line description to say exactly what it does.

Why not Html::escape()? That'd be in line with what Twig and Zend call it. #28 indicates it is possible. Is there a good reason to not go with that much simpler name?

My reasoning was that we're only escaping special characters and to tie in to the PHP function name. But I agree the simplicity of Html::escape() is alluring.

Also it could be argue that even if we don't manage to remove SafeMarkup::escape() the fact that that ends up calling this method means that having the same name actually makes sense.

I like the simplicity of HTML::escape too and likely won't be another useful escaping. There are other tools in php for escaping everything that are used less frequently.

I'm not going to be able to roll the patch right now @wim or @joel do either of you wanna do the refactoring honours - there also be some docs and PHPUnit method name changes. It should be enough to do a case sensitive replace escapeSpecialChars with escape and then fix comment flow issues.

I'll reroll.

Back to RTBC, thanks @Wim Leers. Double checked nothing was missed in the renaming.

Alright, I'm more comfortable with this with the updated name, and I can see the case for adding the method to easily allow changes like #1211866: Enable ENT_SUBSTITUTE flag in Html::escape in the future.

The documentation is a little odd and has a couple grammatical errors, so I'll propose an update.

#33 outlines a potential reason to not list all the characters - I think if some needs that level of detail they should be referring to the documentation that does the actual escaping. I'm not sure we should not shy away from pointing developers to real PHP documentation where appropriate and we have this covered with the @see htmlspecialchars().

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -338,14 +338,55 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
+   * - <
+   * - >
+   * - '
+   * - "
+   * - &

But if we are going to do this why not copy from php.net and do something like...

- '&' (ampersand) becomes '&'
- '"' (double quote) becomes '"'
- "'" (single quote) becomes ''' (or ')
- '<' (less than) becomes '&lt;'
- '>' (greater than) becomes '&gt;'

But as I pointed out in #46 we already have the @see htmlspecialchars().

I don't understand why #33 is a reason not to list out the characters. We can update the docs if we change what the method does in the future. However, I see no reason to cloak what the method does behind a layer of mystery and force people to parse what the specific PHP.net flags do -- otherwise, what is the point of having a method in the first place if not to remove the need for the developer to make these decisions?

People can tell that it uses htmlspecialchars() internally by reading the code and they can read the docs for it then if they choose to in order to see how it works. But IMO it's much better to just explain in plain English what the method does so that developers understand what "escaping" is at all. Otherwise, it hides it behind an intimidating shroud and makes developers think they cannot understand it.

One has to love php.net's '"' and "'" ... cause that is easy to read :)

Re: #9 Yes, which is why I didn't do that.

So if a developer needs to know what escaping is then we need to go further and tell what the character becomes as in #47.

I disagree, but someone else can add that if they like.

The improvements to the docs help make the intentions clear. I'd likely not put (for example) or (e.g.) in parens as it feels to be a side note/whisper/supplementary info, but don't really think that changes the core intention behind the comment fixes, so RTBC.

Fine by me... I think the argument around

But IMO it's much better to just explain in plain English what the method does so that developers understand what "escaping" is at all.

is inconsistent when we don't say what the characters become - but that is no reason to block commit of this further.

rtbc +a gazilllion.

Actually I'll see if I can fix #53 and try to incorporate a compromise on #47

Ok wasn't sure if I should period the end of those items or not, or to quote them or not, so I decided in both cases to not because it added more visual clutter and possible confusion as brought up in #49

Anyways, hopefully this helps?

going to review this

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -338,14 +338,55 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
+   * This method is not the opposite of Html::decodeEntities() because that
+   * method will, for example, decode "é" to "é", wherearas this method
+   * will not change "é" to "é". However, Html::decodeEntities() will

I think you meant whereas. Replace wherearas with whereas.

I have read the entire patch and everything else looks good.

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -338,14 +338,55 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
+   * This method is not the opposite of Html::decodeEntities() because that
+   * method will, for example, decode "é" to "é", wherearas this method
+   * will not change "é" to "é". However, Html::decodeEntities() will
+   * decode all special characters that are converted to HTML entities by
+   * this method.

I am confuse. If I understand correctly escape() will change special characters to HTML entities and decodeEntities() will change HTML entities to special characters. Maybe make doc block less confusing?

Ok I believe the problem @kgoel was having was a bit of double negatives going on so I tried to refactor it a few times to get it into a positive form that can be slightly easier to grasp.

So as to avoid the confusion of the equivalent to if (!!$true).

Fixes #59 as well.

@joelpittet, thank you for improving the doc block. RTBC pending on test bot

I think we can improve the documentation by having shorter and more concise sentences.

@alexpott, I like the doc block and the difference between Html::decodeEntities() and HTML::escape() is very clear.

This looks like it covers all the things. RTBC++

I think this looks good now

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -338,14 +338,54 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
+   * - ' (single quote) becomes ' (or ')

Just ' as we don't have the ENT_HTML5 flag set, so we never translate it to '

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\Html;

This breaks Drupal installation for me: Mac OS X, PHP 5.5.25, MySQL 5.6.24.

I think the problem is that we have Html class in the same directory as HtmlTag class and when we try import \Drupal\Component\Utility\Html PHP is confused with other class.
Quick fix would be to add alias for imported class or to use full namespace when needed.

Providing a patch.

I think the problem is that we have Html class in the same directory as HtmlTag class and when we try import \Drupal\Component\Utility\Html PHP is confused with other class.
Quick fix would be to add alias for imported class or to use full namespace when needed.

I always wondered why there is not tool out there which detects those problematic lines of code ...

@mbovan yeah we've seen this before - it's a PHP bug. We had this in the plugin system somewhere. Yep - see #2170471: ContextAwarePluginBase compromised after commit of Core PluginBase and https://bugs.php.net/bug.php?id=55068

FYI: This is happening because we have Drupal\Core\Render\Element\Html

@dawehner there doesn't need to be a tool - PHP was fail at compile time.

@dawehner there doesn't need to be a tool - PHP was fail at compile time.

Ah that just fails in case both Html and HtmlTag is loaded, right?
So what about writing a test which requires all classes ;)

@dawehner yeah but we do that in the installer and it's not breaking... there is something environment about this.

I'm getting this locally - I have no idea why this is not happening on testbot - let's fix and discuss adding a failing test in a followup. Although this test is not going to catch a new render element that uses Html. php--

We got a test pass on DrupalCI so I cancelled the pift test.