Hello,
the htmlentities do encode the title to prevent "quotes" in the markup, but it is already encoded (by drupal core i guess),
This function seems useless: 

/**
   * Transform the entity title to a attribute.
   *
   * @remarks
   *   The title of the entity and site can not contain double-qoutes. These are
   *   encoded into html chars.
   */
  private function getAttributeTitle($entity) {
    if (isset($entity->title)) {
      return array(
        self::TITLE_ATTRIBUTE => htmlentities($entity->title . ' - ' . variable_get('site_name'), ENT_COMPAT),
      );
    }
    return array();
  }

The result is that titles with quotes and special characters are displayed with htmlentites in the facebook description.

CommentFileSizeAuthor
#11 addthis-double-html-encoding-2803109-11-7.x-4.0-alpha6.patch513 bytesshivamitakari
#9 addthis-2803109-pass-9.patch1.49 KBvijaycs85
#9 addthis-2803109-fail-9.patch2 KBvijaycs85
#3 addthis-double_htmlentities_encoded-2803109-3.patch1.02 KBgisle

Comments

Elyoukey created an issue. See original summary.

  • gisle committed 212d145 on 7.x-4.x 
    Issue #2803109 by gisle: Fixed double htmlentities encoded
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Version: 7.x-4.0-alpha6 » 7.x-4.x-dev
StatusFileSize
new addthis-double_htmlentities_encoded-2803109-3.patch1.02 KB

Confirming that htmlentities are encoded twice if present in the entity title.

However, I don't think the function getAttributeTitle() is useless, as it wraps the title in an array.

But I agree that there is no need to double sanitize htmlentities, so I've removed the specific call to htmlentities()that does that (see attached patch).

I've also tested that there is no regression for #2644044: Warning htmlspecialchars . The following title shows up as expected on FaceBook

Test of "ąęśćżłóæøå" & similar 'stuff' in title

Please review.

I've also pushed this patch to the latest snapshot of the 7.x-4.x branch.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Active » Needs review

Changing status.

danyg’s picture

danyg commented

I agree with code modifications, but if the real idea was to remove double-quotes (@remarks The title of the entity and site can not contain double-qoutes. These are encoded into html chars.) we should change the original code to this:

return array(
        self::TITLE_ATTRIBUTE => str_replace('"', "'", $entity->title) . ' - ' . variable_get('site_name'),
      );

or replace the whole string:

return array(
        self::TITLE_ATTRIBUTE => str_replace('"', "'", $entity->title . ' - ' . variable_get('site_name')),
      );

It'll replace the double-qotes with simple apostrophes.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

@danyg, I don't think the @remarks you refer to makes sense, so I think we should ignore it.

Just because some programmer believed that the title of the entity and site can not contain double-qoutes, we have no obligation to preserve this misunderstanding.

I don't think a CMS should alter what the author wrote unless there is a good reason to do so. Is there a good reason for preventing people from using double quotes in the title of the entity and site?

matglas86’s picture

matglas86 commented
Priority: Normal » Minor
Status: Needs review » Needs work

@gisle I would like to see a test for this. Could you make that? Espacially because you said

I've also tested that there is no regression for #2644044: Warning htmlspecialchars . The following title shows up as expected on FaceBook

It would be nice to make sure this keeps working in the future.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

Thanks everyone for working on this. It would be nice to have another release with fixes like this. I have requested for another release (tag) at #2860839: Adding new release. Also we will try to write some test around this.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new addthis-2803109-fail-9.patch2 KB
new addthis-2803109-pass-9.patch1.49 KB

Adding test to check title. Removed the changes from #2 to prove it fixes the issue.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs work » Needs review
shivamitakari’s picture

shivamitakari commented
StatusFileSize
new addthis-double-html-encoding-2803109-11-7.x-4.0-alpha6.patch513 bytes

I'm attaching same file which mentioned in comment #2,
but its specific to version 7.x-4.0-alpha6