added: Sanitize::rename_attributes() #717
Conversation
library/SimplePie.php
Outdated
| @@ -650,6 +650,13 @@ class SimplePie | |||
| */ | |||
| public $strip_htmltags = array('base', 'blink', 'body', 'doctype', 'embed', 'font', 'form', 'frame', 'frameset', 'html', 'iframe', 'input', 'marquee', 'meta', 'noscript', 'object', 'param', 'script', 'style'); | |||
|
|
|||
| /** | |||
| * @var array Stores the default tags to be stripped by rename_attributes(). | |||
There was a problem hiding this comment.
Maybe use string[] as a more detailed type annotation.
There was a problem hiding this comment.
I want to keep it similar to:
simplepie/library/SimplePie.php
Lines 637 to 644 in 1aec297
library/SimplePie.php
Outdated
| @@ -1218,6 +1225,15 @@ public function strip_htmltags($tags = '', $encode = null) | |||
| } | |||
| } | |||
|
|
|||
| public function rename_attributes($tags = '', $encode = null) | |||
There was a problem hiding this comment.
Would be nice to have a doc comment.
library/SimplePie/Sanitize.php
Outdated
| @@ -145,6 +146,25 @@ public function pass_file_data($file_class = 'SimplePie_File', $timeout = 10, $u | |||
| } | |||
| } | |||
|
|
|||
| public function rename_attributes($tags = array()) | |||
There was a problem hiding this comment.
Maybe we could prefix the method name with set to make it clear it is just a setter. I know the old setters do not follow this convention but we could start switching to more modern names (and deprecate the old ones after #711).
There was a problem hiding this comment.
I agree, it would be better. Let's do it in another PR. I want to focus here on the rename method
library/SimplePie/Sanitize.php
Outdated
| } | ||
| else | ||
| { | ||
| $this->rename_attributes = explode(',', $tags); |
There was a problem hiding this comment.
Is there any need to support comma separated strings, rather than just arrays of strings? Apps can always do the explosion themselves.
There was a problem hiding this comment.
It is the same as:
simplepie/library/SimplePie/Sanitize.php
Lines 148 to 165 in 1aec297
library/SimplePie/Sanitize.php
Outdated
| } | ||
| else | ||
| { | ||
| $this->rename_attributes = false; |
There was a problem hiding this comment.
Why not just use array() and have the foreach be a no-op?
There was a problem hiding this comment.
It is the same as:
simplepie/library/SimplePie/Sanitize.php
Lines 148 to 165 in 1aec297
|
thanks @math-GH looks good |
|
The issue with the changes is once we release it, we will not be able able to change it (until 2.0) unless we include more compatibility shims. We should strive to make any new API clearer from the get go, not try to make it look like API that was designed ages ago. |
I want to bring a new method that we implemented into Simplepie of FreshRSS: FreshRSS/FreshRSS#4175
The use case:
Simplepie can clean the HTML with
strip_attributes(), but in some cases it would be very useful to keep the information of some attributes under another attribute name, f.e.idandclassThe new
rename_attributes()methods enables it. Theidandclassattributes could be renamed todata-sanitized-id/data-sanitized-class.The benefit: when HTML/XML/RSS is included into an existing HTML DOM the imported HTML/XML/RSS could have id/class attributes that are already defined in the DOM, but with another meaning. Rename this attributes brings id/class out of standard CSS behavior but keeps the information. It is still possible to style it with
div[data-sanitized-class*="CLASSNAME"]It would be a pleasure to extend Simplepie with this little improvement.