Alright! Bringing back the dead to the realm of the living :-)
So it appears this is something that has been lingering for a very long time...

Taking over from issue #1120066-3: Support for width="123px" and #1120066-4: Support for width="123px":

The problem with forcing all `"` to change into quotes is that if a quote is desired in the title, then there is no way to supply it.

which is actually a problem encountered many times before, see: #1214186: options form="noform" Not Compatible with CKEditor? or #1958468: simplexml_load_string() parser error related with CKEditor.

The problem is rather simple, if a user wants to add double quotes in the title, the only way currently supported is to use the HTML entity ", for example:

[collapse title="This "is" a title with "double" quotes included" class="foo bar baz"]

See resulting screenshot:

There are several problems with that, but except the obvious usability issue it poses (being impractical to many non tech-savvy users, having to be documented, etc...), the biggest problem is probably related with the integration to CKEditor (or other wysiwyg editors), which would by default convert automatically these " entities in the HTML code to ", ending up modifying the code as follows:

[collapse title="This "is" a title with "double" quotes included" class="foo bar baz"]

See resulting screenshot:

In this case, users are most likely not aware at all of the change in the source code, or of any change other than the ones they made while editing the content.

When the item is saved with the code above, the following error messages are displayed:

Warning: simplexml_load_string(): Entity: line 1: parser error : attributes construct error in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): <collapse title="This "is" a title with "double" quotes included" class="foo bar in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): ^ in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): Entity: line 1: parser error : Couldn't find end of Start Tag collapse line 1 in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): <collapse title="This "is" a title with "double" quotes included" class="foo bar in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): ^ in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): Entity: line 1: parser error : Extra content at the end of the document in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): <collapse title="This "is" a title with "double" quotes included" class="foo bar in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).
Warning: simplexml_load_string(): ^ in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).

Which is very confusing... There is absolutely no clue for any user, tech-savvy or not, as to what the problem could have been....

Warning: simplexml_load_string(): <collapse title="This "is" a title with "double" quotes included" class="foo bar in _collapse_text_process_child_item() (line 522 of /sites/default/modules/collapse_text/collapse_text.module).

Would probably be the closest to letting users know the problem came from Collapse Text's markup in the content previously saved.

Perhaps an idea could be to systematically convert all double quotes " in the title attribute to &quot;, automatically, which would then resolve the usability issue (allowing users to directly input double quotes, instead of having to use &quot;) and the compatibility issue with rich text editors.

Several attempts were made in other issues, for example, see the patch at #1120066-3: Support for width="123px", or the comment in collapse_text.module, line 165:

<?php
  // fix the title element
  // not sufficient if title includes double-quotes
?>

Maybe we could try to investigate a little bit further these regular expressions or find another method to properly resolve this long lasting problem.

Please let me know if you would have any comments, feedback, questions, issues, objections, suggestions or concerns on any aspects of this bug report, I would be glad to provide more information or explain in more details.

Thanks again very much to everyone for your feedback, testing and reporting.
Cheers!

CommentFileSizeAuthor
#1 collapse_text-support-double-quotes-title-attribute-2487193-1.patch1.68 KBdydave
collapse_text-support-double-quotes-title-attribute-20150512-2.jpg31.54 KBdydave
collapse_text-support-double-quotes-title-attribute-20150512-1.jpg38.07 KBdydave

Comments

dydave’s picture

dydave commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new collapse_text-support-double-quotes-title-attribute-2487193-1.patch1.68 KB

Quick follow-up on this issue:

Unfortunately, regular expressions is not necessarily my strongest suit.... But I did spend quite some time trying to find a working regex that could potentially capture the " characters in collapse text's markup. Unfortunately, there seems to be too many possible cases, including the ones with faulty or missing quotes opening or closing attributes, for example:

[collapse title=this "is" a title with "double" quotes included" class="foo bar baz"]

So I assumed one more logical step would have to be added and turned myself to preg_replace_callback to potentially add one more level of processing to the captured string.
Currently, in collapse_text.module, line 166:

<?php
  $tag = preg_replace(
    '/ title=([^"].*?)(?= collapsed=| class=|\])/i',
    ' title="$1"',
    $tag
  );
?>

module wraps the title attribute in double quotes if none have been provided, but it doesn't correct faulty or missing quotes either, such as the example above.

So I assumed the capturing pattern should be modified to also include double quotes, whether faulty or not:
/ title=(.*?)(?= collapsed=| class=|\])/i, which would capture:
title=this "is" a title with "double" quotes included" in the example above and feed it to a callback function.

The callback function basically, strips off wrapping double quotes and then replaces all occurrences of " with &quot; before wrapping the value again with double quotes.
Still with the same example, the string returned after replacement would be:

[collapse title="this &quot;is&quot; a title with &quot;double&quot; quotes included&quot;" class="foo bar baz"]

watch carefully, how it has not only replaced the quotes included in the text of the title attribute, but it has also fixed the problem of the faulty quotes, wrongly closed.

Last problem encountered, the order of execution: whether Collapse Text is positioned before or after the HTML Filter (Correct faulty and chopped off HTML), actually determines the place in code where replacements would need to happen.
If replacements were made in the prepare callback of the filter, then Collapse Text would have to be positioned before the HTML Filter.
Ideally, we'd like to find a solution that would be independant of the position of Collapse Text, although it is currently still recommended on the project page that the Collapse Text

input filter must come after the HTML filter, and should go after the convert line breaks filter.

To be independant of the order, replacements would have to take place in the process callback of the filter.
 

Alright! So, as you may have guessed, I've taken a pretty deep stab at this, but the problem seems a bit too complicated to be committed straight away and ideally I would wish others could please help reviewing and testing the suggested solution and attached patch.

Please find attached to this comment an initial patch against collapse_text-7.x-2.x at 85656e4 that should provide a robust way to have this issue with double quotes resolved.
File attached as: collapse_text-support-double-quotes-title-attribute-2487193-1.patch.

Unfortunately, the patch comes without tests, so far.... indeed, test cases for the processing functions are currently inexistant, so we will most likely need to get back to this issue while working on another ticket focused entirely on SimpleTests for the module. Tagging Needs tests.

Getting this committed would be a great improvement for module's stability and robustness, fixing an important issue that's been dragging for more than 4 years...

Therefore, I would greatly appreciate your reviews, feedbacks, testing/reporting, suggestions, objections, concerns, questions or ideas on this issue, any help would be warmly welcome.

Please let me know if you would have any comments, feedback, questions, issues, objections, suggestions or concerns on any aspects of this comment, attached patch or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks very much in advance to everyone for your feedback, testing and reporting.
Cheers!

dydave’s picture

dydave commented
Status: Needs review » Closed (outdated)

Drupal 7 is not supported anymore, therefore this issue is unlikely to go any further.

Additionally: No activity or reply for more than 10 years.

If this issue is still valid for more recent versions of the module and Drupal core, please create a new ticket with the appropriate version.

Marking issue as Closed (outdated), for now.

Thanks everyone for your interest in the Collapse Text module and contributions! 😊

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.