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

[ReutersBridge] Change timestamp, add new feed, add alt text to image. #2150

Merged
merged 17 commits into from
Jul 20, 2021
Merged

[ReutersBridge] Change timestamp, add new feed, add alt text to image. #2150

merged 17 commits into from
Jul 20, 2021

Conversation

csisoap
Copy link
Contributor

@csisoap csisoap commented Jun 7, 2021

  • Change timestamp for each article from modified date to published date.
  • Add new 'Fact Check' feed.
  • Add support for Twitter, Youtube, Instagram embed.
  • Add alt text for each images (not include those chart/graphic).

csisoap and others added 6 commits March 4, 2021 19:06
- Add new wireitem template type and change the code on line 233 to use the extracted data instead of original one.
- Refactor some codebase.
- Change timestamp for each article from modified date to published date.
- Add new 'Fact Check' feed.
- Add support for Twitter embed.
- Add alt text for each images (not include those chart/graphic).
- Change timestamp for each article from modified date to published date.
- Add new 'Fact Check' feed.
- Add support for Twitter embed.
- Add alt text for each images (not include those chart/graphic).
bridges/ReutersBridge.php Outdated Show resolved Hide resolved
@em92
Copy link
Contributor

em92 commented Jun 12, 2021

I suggest to use multi line strings instead of those indents. Here is an example

$item['content'] = <<<EOD
<p>Media Type: {$result->attr['data-mediatype']}<br>
Collection: <a href="{$collectionLink}">{$collectionTitle}</a></p>
EOD;

@em92
Copy link
Contributor

em92 commented Jul 8, 2021

Just checked those embed iframes. How do you use them? They don't seem to be convenient to use.
Capture 2

@csisoap
Copy link
Contributor Author

csisoap commented Jul 9, 2021

Just checked those embed iframes. How do you use them? They don't seem to be convenient to use.
Capture 2

It's missing the iframe closing tag. That's weird, i remember that add them.

@em92
Copy link
Contributor

em92 commented Jul 12, 2021

It's missing the iframe closing tag. That's weird, i remember that add them.

I don't think problem is about closing tags. Actually iframes are escaped:

protected function sanitizeHtml($html)
{
$html = str_replace('<script', '<&zwnj;script', $html); // Disable scripts, but leave them visible.
$html = str_replace('<iframe', '<&zwnj;iframe', $html);
$html = str_replace('<link', '<&zwnj;link', $html);
// We leave alone object and embed so that videos can play in RSS readers.
return $html;
}

@csisoap
Copy link
Contributor Author

csisoap commented Jul 13, 2021

It's missing the iframe closing tag. That's weird, i remember that add them.

I don't think problem is about closing tags. Actually iframes are escaped:

protected function sanitizeHtml($html)
{
$html = str_replace('<script', '<&zwnj;script', $html); // Disable scripts, but leave them visible.
$html = str_replace('<iframe', '<&zwnj;iframe', $html);
$html = str_replace('<link', '<&zwnj;link', $html);
// We leave alone object and embed so that videos can play in RSS readers.
return $html;
}

I think what make iframe not working is this: <&zwnj;iframe instead of <iframe.
When use Feed Reader like TT-RSS, they cut it out iframe on feeds from RSS-Bridge, but allow iframe from others.

@em92
Copy link
Contributor

em92 commented Jul 20, 2021

Ok, I will accept it as is. Good job!

@em92 em92 merged commit cabf7a7 into RSS-Bridge:master Jul 20, 2021
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.

2 participants