Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XPathAbstract] Refactor xpath abstract #4047

Merged
merged 7 commits into from
Apr 2, 2024

Conversation

Niehztog
Copy link
Contributor

@Niehztog Niehztog commented Apr 1, 2024

Yesterday when working on #4038 I was a bit too quick and later I got the idea how to structure the code cleaner and nicer. So I refactored the changes, keeping all existing functionality intact.

While refactoring I also stumbled accross #3672 and wondered if it makes sense to skip calling htmlspecialchars on the feed item contents in special cases. At least for the Atom format it does not seem to make any difference (I get escaped html tags no matter how I set this setting). Any thoughts about that? @User123698745

Copy link

github-actions bot commented Apr 1, 2024

Pull request artifacts

Bridge Context Status
BlizzardNews 1 untitled (current) ⚠️ (shutdown) 8192: Function utf8_decode() is deprecated in lib/XPathAbstract.php line 676
BlizzardNews 1 untitled (pr) ⚠️ (shutdown) 8192: Function utf8_decode() is deprecated in lib/XPathAbstract.php line 643

last change: Monday 2024-04-01 21:15:46

@dvikan
Copy link
Contributor

dvikan commented Apr 1, 2024

feed readers escape for html context anyways. And the atom/xml formats escape for xml context. So i think it's best to include raw unescaped html in feed items indeed.

i like this refactor. makes for easier maintenance. be careful not to break existing feeds who have toggled this checkbox already.

@User123698745
Copy link
Contributor

I added the optional raw_content parameter to the XPathBridge to allow e.g. concat('<b>', .//p/text(), '</b>') to produce html which in this example results in bold text. Without raw_content enabled, <b> and </b> would get escaped / it would have no effect.

Interestingly, you (accidentally?) broke this on master with commit 1c3c85d8ff5a6d071f688ef09ca93f275b4995af. Currently on master it behaves like raw_content is always on (before the default was off). However with this PR it seems you will (accidentally?) repair it. Your current PR commit 062d66ce1e19eb372b9ee5304c6beeef0a3fc664 behaves like it used to after I added raw_content back then.

How raw_content = off (default) should look like:
image
(currently broken on master, therefore this link will look like on)
https://rss-bridge.org/bridge01/?action=display&bridge=XPathBridge&url=https%3A%2F%2Ftomorrowcorporation.com%2F&item=%2F%2Fdiv%5B%40class%3D%22entryMain%22%5D&title=.%2Fh2&content=concat%28%27%3Cb%3E%27%2C+.%2F%2Fp%2Ftext%28%29%2C+%27%3C%2Fb%3E%27%29&raw_content=off&uri=.%2F%2Fa%2F%40href&author=&timestamp=&enclosures=&categories=&format=Html

How raw_content = on should look like:
image
https://rss-bridge.org/bridge01/?action=display&bridge=XPathBridge&url=https%3A%2F%2Ftomorrowcorporation.com%2F&item=%2F%2Fdiv%5B%40class%3D%22entryMain%22%5D&title=.%2Fh2&content=concat%28%27%3Cb%3E%27%2C+.%2F%2Fp%2Ftext%28%29%2C+%27%3C%2Fb%3E%27%29&raw_content=on&uri=.%2F%2Fa%2F%40href&author=&timestamp=&enclosures=&categories=&format=Html

@Niehztog
Copy link
Contributor Author

Niehztog commented Apr 1, 2024

@dvikan @User123698745 Thank you for your feedbacks. So if I understand you both correct, there is almost no use case for automatically escaping html tags in item contents, right?
So I made a new commit changing the default value for that setting, so that html tags won't get escaped unless you manually overwrite that setting.
I suggest to leave it in for now in case there are not so obvious use cases and people want to turn it on for their feeds. In the long run we shoud remove that setting and omit escaping feed item contents by invoking htmlspecialchars.

Do you both agree?

@dvikan
Copy link
Contributor

dvikan commented Apr 1, 2024

yes i think i agree. anyways i trust you on this one since im not vell versed with xpath bridge,

will merge unless noted otherwise

@User123698745
Copy link
Contributor

I agree, too

@dvikan dvikan merged commit fb66775 into RSS-Bridge:master Apr 2, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants