[drupal-devel] [feature] aggregator: make filtered HMTL tags configurable

[Morbus Iff]

Issue status update for 
http://drupal.org/node/29275
Post a follow up: 
http://drupal.org/project/comments/add/29275

 Project:      Drupal
 Version:      cvs
 Component:    aggregator.module
 Category:     feature requests
 Priority:     normal
 Assigned to:  Uwe Hermann
 Reported by:  Uwe Hermann
 Updated by:   Morbus Iff
 Status:       patch (code needs review)

"In time for the code freeze" is the wrong justification. Forcing a
patch through just so it can beat the code freeze allows quick and
dirty code, as opposed to the right code. Encouraging quick patches
allows the least tested and least designed code to get in during the
point when the code should be most stable and eyeballed. I'd much
rather the right code than a bunch of quick patches that do things
half-assed (in design, not quality - your patch is just fine for what
it attempts to do, just not the best way to do it).




Morbus Iff



Previous comments:
------------------------------------------------------------------------

Sun, 21 Aug 2005 00:13:21 +0000 : Uwe Hermann

Attachment: http://drupal.org/files/issues/aggregator_allowed_html_tags.patch (2.34 KB)

I'm currently trying to get aggregator.module in shape for being used as
a planet-like software for a Planet Drupal [1]. Here's a patch which
allows the site admin to specify which HTML tags are stripped from
feeds (or not). This is hardcoded in aggregator.module right now, the
attached 2-line patch (for HEAD) makes it configurable.


I propose to add at least "<img> <font> <blockquote> <div> <span>
<code>" to the default list of allowed HTML tags, but I'll leave that
for another patch. Any objections?


[1] http://drupal.org/node/28194




------------------------------------------------------------------------

Sun, 21 Aug 2005 03:18:30 +0000 : Prometheus6

+1


I know it works because it's identical to what I did.




------------------------------------------------------------------------

Sun, 21 Aug 2005 08:22:54 +0000 : Junyor

This has come up before: http://drupal.org/node/14104.  Could you
possibly use input filters instead of a setting for this?




------------------------------------------------------------------------

Sun, 21 Aug 2005 08:59:41 +0000 : Bèr Kessels

sorry. -1.
IMHO aggregator should be small, clean and lean. No options, features
or whatevers. (node aggregator does this for free, and much more. If
you want advanced aggregation, then please add your development to this
module, or to another -new- advanced aggregation module)




------------------------------------------------------------------------

Sun, 21 Aug 2005 09:09:28 +0000 : robertDouglass

-1 for this approach and a big +1 for running aggregators through
filters that can be configured.




------------------------------------------------------------------------

Sun, 21 Aug 2005 09:10:44 +0000 : robertDouglass

I have to add that I would rather have this patch than nothing, since as
Uwe correctly points out, the behavior is hardcoded and not suitable for
many common cases.




------------------------------------------------------------------------

Sun, 21 Aug 2005 09:36:38 +0000 : Bèr Kessels

can we not use the default filter system here?




------------------------------------------------------------------------

Sun, 21 Aug 2005 14:38:28 +0000 : Prometheus6

Yes, the filter system could be used, but if you really want lean and
mean this really minor patch gives you much of the functionality of one
of the filters that ships in core without a lot of UI development. It
will NEVER make the 4.7 code freeze.


I suggest applying this patch for 4.7 and working on using the filter
system in the aggregator later.




------------------------------------------------------------------------

Sun, 21 Aug 2005 15:00:44 +0000 : Morbus Iff

-1. Filters.




------------------------------------------------------------------------

Sun, 21 Aug 2005 15:03:49 +0000 : Prometheus6

Clarification: Adapting the aggregator to use the filter system is a big
enough job that doing so in time for the code freeze is unlikely.
Seriously, how important is is? It HAS been raised before...


Meanwhile you get a significant improvement for two lines of code.
There is literally no down side to applying this patch that I can
see...I really don't understand why it would NOT be applied.




------------------------------------------------------------------------

Sun, 21 Aug 2005 15:09:12 +0000 : Uwe Hermann

I agree with Prometheus6. The patch changes absolutely no functionality
in any way, it simply makes something configurable which is hard-coded
right now. I'd rather have this in 4.7. than nothing.