Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I was on the way to find ways for WYSIWYG editors to identify the "used input mechanism or mechanisms" (be it HTML, wiki formatting, bbcode, markdown, etc), and realized that the HTML filter has this information tucked down in a setting. This is not only against providing a good filter list overview on the user interface (by the used "HTML filter", it is impossible the tell, whether it does wholesale HTML escaping, or just filters a few tags), but also makes it harder to find out whether the format disallows HTML completely (so that a HTML supporting RTE/WYSIWYG should not be listed as an option on the format), or it supports HTML, so we should provide HTML RTE/WYSIWYG as options to use for the format.
My suggestion is to break out the HTML escaping filter. Previously the "HTML filter" was a one-stop-shop for all HTML management in core, so it might have made sense to have the escaping setting hidden there. Although if you look on a D6 filter setup, there is also a HTML corrector, and as soon as you install some contrib module, the list of filters acting on HTML is getting longer. Also, if you look at the old (before patch) filter HTML configure form, most of its settings are useless and confusing when escaping is used:
Because Drupal works with HTML by default, I plan to build on the existence of this "Escape all HTML tags" filter to identify formats, where HTML is disallowed, so this will limit the available RTEs to attach to the format.
The patch itself looks much longer, but it changes a surprisingly small amount of code. Most changed lines are whitespace changes on the HTML formatting help code. I changed the names of the filters though to be more "active" in their wording and (hopefully) less technical. It would be great if people could help refine the text a bit still:
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | html_escaping_cleanup.patch | 2.82 KB | David_Rothstein |
| #8 | standalone-html-escape.patch | 19.92 KB | recidive |
| #7 | standalone-html-escape.patch | 19.86 KB | recidive |
| #2 | standalone-html-escape-1.patch | 26.84 KB | keith.smith |
| filters-new.png | 93.34 KB | Gábor Hojtsy | |
Comments
Comment #1
Gábor HojtsyOh, and by the way, the upgrade path is @todo in the patch, and awaits positive reviews on the approach to be received. :)
Comment #2
keith.smith CreditAttribution: keith.smith commentedHmm. You may want to just skip the attached patch; I feel like I got confused when editing and may have ventured into areas best left for another issue. My eyes are crossing, so I'm going to put this up for a bit; I attach it for posterity and so I can find it tomorrow.
Comment #3
floretan CreditAttribution: floretan commentedI would break the removal of the "style" attribute into a separate filter as well. Currently, it's not possible to let users choose custom text color and keep some control on what HTML structures they can use.
If this is in a separate filter, we could also have it allow only certain CSS attributes, like "color" or "text-align".
Comment #4
catch"class" is stripped as well isn't it? That really would be handy.
Comment #5
floretan CreditAttribution: floretan commentedI was wondering about that as well, and "class" is not stripped. Try this with the filtered HTML input format:
<em class="error">This is red</em>which gives this result: This is red. This could be exploited for nastier purposes.Maybe we should generalize this to a configurable attribute filter.
Comment #6
Gábor Hojtsyflobruit, catch: I think your suggestions sound nice, but are not in scope of this patch. This is already a big patch file although lots of the changes are just indentation. There is not yet even a configuration option in Drupal for stripping style attributes or not. Let's try to concentrate on breaking out the escaping filter, whether it is a good idea, get this in, so we can move forward with the WYSIWG / RTE roadmap and core could accept more patches to massage these filters into even more filters if you wish.
Comment #7
recidive CreditAttribution: recidive commentedThis is definitely an improvement and provides consistency for the HTML filter settings. Also the terminology is a lot better IMO. So a huge +1.
Just one question: is the
trim()call really necessary on$op = 'process'for the escape HTML filter?I've roughed in an upgrade path. I'm just not sure about the weight defaulting to 0 for the escape HTML filter.
Patch attached.
Comment #8
recidive CreditAttribution: recidive commentedI've tested the new filter and it works as expected.
Changed the update function to use a direct db query instead of a call to
filter_formats(). I've tested the update and it is working.Comment #9
Dries CreditAttribution: Dries commentedI've committed this patch to CVS HEAD. Thanks.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch seems to have missed removing some text that referred to the old way of doing things. A possible correction is attached (while I was at it, I also fixed an incorrectly-labeled link on the same page).
Comment #12
floretan CreditAttribution: floretan commentedYes, these need to be fixed. Patch applies cleanly and the proposed string changes make sense.
Comment #13
Gábor HojtsyAgreed, looks good. That "view tab" text is quite old, and I'm happy it is going to be fixed :) Thanks!
Comment #14
Dries CreditAttribution: Dries commentedGood catch. Committed to CVS HEAD. Thanks David.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.