-
Notifications
You must be signed in to change notification settings - Fork 343
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
Various fixes #906
Various fixes #906
Conversation
@@ -212,7 +212,7 @@ public function getId() { | |||
*/ | |||
public function getTitle() { | |||
if ($this->items !== false && $this->valid()) { | |||
return @current($this->items)->get_title(); | |||
return htmlspecialchars_decode(@current($this->items)->get_title()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you decoding the title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because some people wanted HTML in the title to be shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it escaped for GitHub spout then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because otherwise, a commit message containing HTML entities would mess up the selfoss stream in the client browser (or be stripped if the tag is not allowed). What we want from commit messages is the raw HTML to be shown in the browser, thus the escaping.
On the contrary, in RSS feeds, we want the HTML to format text in the client browser. Thus, it must be passed as raw HTML the the content loader, filtered by sanitize{Field,Content}() and sent the the client browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSS specs suggest HTML can only appear within description
element. The following feed was broken by the PR:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? From a quick test, the behavior for this feed is the same as before, which was my intention. A few years ago, some people were expecting HTML in the title. No sure what to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the behaviour was the same. And apparently, Feedly and tt-rss do it too. For now we can probably leave it as is
spouts/spout.php
Outdated
@@ -91,14 +91,16 @@ public function getSpoutTitle() { | |||
abstract public function getId(); | |||
|
|||
/** | |||
* returns the current title as string | |||
* returns the current title as string with html special chars decoded if | |||
* applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is it applicable
?
selfoss was checking for items with the same uid in the whole items database, not within the same source. This change searchs for items with the same uid only in the same source. This would have potentially allowed one feed to prevent any new items from being loaded from another feed.
Various fixes detailed in the individual commits.