Improved: Fetch articles with selector but do not delete the class attribute. (Simplepie: new method: rename_attribute)#4175
Conversation
|
Looks good at a first glance 👍🏻 |
Co-authored-by: Alexandre Alapetite <[email protected]>
| if ($tags) | ||
| { | ||
| if (is_array($tags)) | ||
| { | ||
| $this->rename_attributes = $tags; | ||
| } | ||
| else | ||
| { | ||
| $this->rename_attributes = explode(',', $tags); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| $this->rename_attributes = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
You could remove a layer by doing:
if (!$tags) {
$this->rename_attributes = false;
} elseif (is_array($tags)) {
$this->rename_attributes = $tags;
} else {
$this->rename_attributes = explode(',', $tags);
}I find that it makes the code more readable.
There was a problem hiding this comment.
You could also change the order like:
if (is_array($tags)) {
$this->rename_attributes = $tags;
} elseif (is_string($tags)) {
$this->rename_attributes = explode(',', $tags);
} else {
$this->rename_attributes = false;
}This will reject everything that is not supported. I don't know if this something we want though. In this case if you have a number, it will not be treated as a string.
There was a problem hiding this comment.
I don't think it makes sense to reject anything. If it's available as data-whatever you can use it in CSS or JS; if it's not you can't.
There was a problem hiding this comment.
The first example is not rejecting anything. It's a rewrite without the layers. The second example is just that, an example :)
You're right about not rejecting things at this level.
There was a problem hiding this comment.
it is copy&paste from strip_htmltags() below (lines 168ff).
I would say that it does not make sense to have another structure.
|
I changed this PR to draft. Have some discussions in the Simplepie repro. Let's wait for the results there. Had already to change something (some copy&paste faults....) |
|
New function is merged in Simplepie upstream. Ready for review here |
|
|
||
| protected function rename_attr($attrib, $xpath) | ||
| { | ||
| $elements = $xpath->query('//*[@' . $attrib . ']'); |
There was a problem hiding this comment.
A bit late for my feedback, but some kind of sanitising of $attrib could have been good. Not there, but in the public function.
There was a problem hiding this comment.
please help me to understand. What kind of sanitizing do you mean?
There was a problem hiding this comment.
If $attrib happens to be something like ] or any other special character, it will make the XPath crash or worse
4 participants
My User Story:
As a reader I want to configure the content of fetched articles, so that I can hide parts that I do not need.
Changes proposed in this pull request:
User side:
idandclassattributes will not deleted anymore. It will renamed todata-sanitized-id/data-sanitized-classdata-sanitized-classtags (f.e.div[data-feed="XX"] div[data-sanitized-class*="CLASSNAME"]Here an example, that the fetched article footer could be re-styled with CSS (here with
background-color:red)technical side:
rename_attr()$rename_attributesis an empty array$simplePie->rename_attributes('class');is set, so that theclassattribute is renamed todata-sanitized-classHow to test the feature manually:
data-sanitized-classPull request checklist: