Better enclosures by Alkarex · Pull Request #4944 · FreshRSS/FreshRSS

Alkarex · 2022-12-14T22:45:08Z

#fix #4702
Improvement of #2898

#fix FreshRSS#4702 Improvement of FreshRSS#2898

And use <figure> for enclosures

math-GH · 2022-12-30T12:54:29Z

@Alkarex am I allowed to add my changes, that moves the HTML from content() to the .phtml? Or should I wait till this PR is merged and will open an additional PR later?

Alkarex · 2022-12-30T12:58:55Z

@Alkarex am I allowed to add my changes, that moves the HTML from content() to the .phtml? Or should I wait till this PR is merged and will open an additional PR later?

Do you mean #4943 ?

Alkarex · 2022-12-30T12:59:37Z

I have just spotted a couple of bugs in SimplePie that needs fixing for this PR to work with various encodings

math-GH · 2022-12-30T13:01:10Z

@Alkarex am I allowed to add my changes, that moves the HTML from content() to the .phtml? Or should I wait till this PR is merged and will open an additional PR later?

Do you mean #4943 ?

#4943 was just the proof of concept.

I have just spotted a couple of bugs in SimplePie that needs fixing for this PR to work with various encodings

ok, then it will make sense to wait till this PR is done.

Encoding errors in enclosure links

lib/SimplePie/SimplePie/Enclosure.php

Alkarex · 2022-12-30T13:21:52Z

lib/SimplePie/SimplePie/Item.php

+				}
+				else
+				{
+					$thumbnail['url'] = $this->sanitize($thumbnail['url'], SIMPLEPIE_CONSTRUCT_IRI);


The thumbnail URL was previously returned unsanitized and unescaped, as opposed to other URLs returned by SimplePie

TODO: provide a base as parameter for absolutize_url to work. Check that other enclosure URLs are correct in this regard.

Upstream PR: simplepie/simplepie#770

Alkarex · 2022-12-30T13:27:59Z

@math-GH Tests very much welcome already.

I need to look into another SimplePie potential issue with a lack of absolutize_url for the thumbnail URL, but I will continue a bit later

SimplePie only returned the first enclosure when using the RSS 2.0 syntax for enclosures, instead of returning them all. Please merge simplepie#768 first. Downtream PR: FreshRSS/FreshRSS#4944

Add missing sanitize and absolute_url on thumbnail URL. SimplePie normally sanitizes all URLs, but the thumbnail URL was not processed. Related to simplepie#768 Downstream PR FreshRSS/FreshRSS#4944

math-GH · 2023-01-05T18:06:40Z

This PR breaks the multi enclosures of Atom feeds.

Example feed: https://github.com/math-GH/testfeed/blob/main/Atomfeed-with-enclosure.xml
This 1 article has 4 enclosure images.

main branch: all 4 images will be shown
this PR: only 1 image will be shown

Alkarex · 2023-01-05T18:34:56Z

This PR breaks the multi enclosures of Atom feeds.

Example feed: https://github.com/math-GH/testfeed/blob/main/Atomfeed-with-enclosure.xml This 1 article has 4 enclosure images.

main branch: all 4 images will be shown this PR: only 1 image will be shown

@math-GH This is because the HTML content already contains those images, in order to avoid duplicates (cf. #1668 )

https://github.com/FreshRSS/FreshRSS/pull/4944/files#diff-318f03d8ac66816058c42f26b8ed4c5ff71306b6250d7096ad25e0a15813e8efR125-R127

The four enclosures are correctly stored in database and provided again e.g. via API or RSS view

{
	"enclosures": [{
			"url": "https://0.gravatar.com/avatar/347dfeae267d8d9b7c8bf959b3b97baa?s=96&amp;d=identicon&amp;r=G",
			"title": "tomsgedankenblog",
			"medium": "image"
		}, {
			"url": "https://agileverwaltungorg.files.wordpress.com/2022/10/grafik-4.png?w=605",
			"medium": "image"
		}, {
			"url": "https://agileverwaltungorg.files.wordpress.com/2022/10/grafik-3.png?w=605",
			"medium": "image"
		}, {
			"url": "https://agileverwaltungorg.files.wordpress.com/2022/10/grafik-5.png?w=605",
			"medium": "image"
		}
	]
}

Avoid generating all enclosures if not used

Clients are typically not showing the enclosures to the users (tested with News+, FeedMe, Readrops, Fluent Reader Lite)

math-GH · 2023-01-06T16:09:52Z

@Alkarex That is a great improvement ❤️

Alkarex · 2023-01-06T18:53:33Z

Let's give it a try 🚀

math-GH · 2023-01-06T23:03:00Z

This PR breaks the multi enclosures of Atom feeds.
Example feed: https://github.com/math-GH/testfeed/blob/main/Atomfeed-with-enclosure.xml This 1 article has 4 enclosure images.
main branch: all 4 images will be shown this PR: only 1 image will be shown

@math-GH This is because the HTML content already contains those images, in order to avoid duplicates (cf. #1668 )

I do not know if it is a bug or improvement or a feature:
The thumbnail is now the blue avatar, that is the first enclosure.
What would make more sense?
To take the first enclosure as thumbnail? Or take the first image in the article?

Alkarex · 2023-01-08T13:41:12Z

I do not know if it is a bug or improvement or a feature: The thumbnail is now the blue avatar, that is the first enclosure. What would make more sense? To take the first enclosure as thumbnail? Or take the first image in the article?

@math-GH Compared to before, the sole difference should be that we now support the dedicated thumbnail RSS element, which we then use as first priority. Before this PR, we were already searching for the first image in enclosures and then in the HTML body.

* No URL Decode for enclosure links This line was introduced in 8a5f601#diff-a710c236cc7775672edbe5840d8fb96b0e91b0d56eff709f6eace2da47c1fe43R349-R359 Seems to be a similar error than efb1d8e Having an `urldecode` returns some potentially invalid URLs, while SimplePie does not do that for other returned URLs Downstream PR FreshRSS/FreshRSS#4944 P.S. There are a few other bugs related to enclosure links, and I will try to address some of them in distinct PRs. * Add tests * Simplify tests * Whitespace * PHP 7.2 compat * Update tests/Unit/EnclosureTest.php Co-authored-by: Jan Tojnar <jtojnar@gmail.com> Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

* Sanitize thumbnail URL Add missing sanitize and absolute_url on thumbnail URL. SimplePie normally sanitizes all URLs, but the thumbnail URL was not processed. Related to #768 Downstream PR FreshRSS/FreshRSS#4944 * Update tests syntax

* No URL Decode for enclosure links This line was introduced in 8a5f601#diff-a710c236cc7775672edbe5840d8fb96b0e91b0d56eff709f6eace2da47c1fe43R349-R359 Seems to be a similar error than efb1d8e Having an `urldecode` returns some potentially invalid URLs, while SimplePie does not do that for other returned URLs Downstream PR FreshRSS/FreshRSS#4944 P.S. There are a few other bugs related to enclosure links, and I will try to address some of them in distinct PRs. * Add tests * Simplify tests * Fix case of multiple RSS2.0 enclosures SimplePie only returned the first enclosure when using the RSS 2.0 syntax for enclosures, instead of returning them all. Please merge #768 first. Downtream PR: FreshRSS/FreshRSS#4944 * Whitespace * PHP 7.2 compat * PHP 7.2 compat * Update tests/Unit/EnclosureTest.php Co-authored-by: Artur Weigandt <Art4@users.noreply.github.com> * Update tests/Unit/EnclosureTest.php Co-authored-by: Jan Tojnar <jtojnar@gmail.com> * Update src/Item.php Co-authored-by: Jan Tojnar <jtojnar@gmail.com> * Update tests syntax Co-authored-by: Artur Weigandt <Art4@users.noreply.github.com> Co-authored-by: Jan Tojnar <jtojnar@gmail.com>

* Better enclosures #fix FreshRSS#4702 Improvement of FreshRSS#2898 * A few fixes * Better enclosure titles * Improve thumbnails * Implement thumbnail for HTML+XPath * Avoid duplicate enclosures #fix FreshRSS#1668 * Fix regex * Add basic support for media:credit And use <figure> for enclosures * Fix link encoding + simplify code * Fix some SimplePie bugs Encoding errors in enclosure links * Remove debugging syslog * Remove debugging syslog * SimplePie fix multiple RSS2 enclosures #fix FreshRSS#4974 * Improve thumbnails * Performance with yield Avoid generating all enclosures if not used * API keep providing enclosures inside content Clients are typically not showing the enclosures to the users (tested with News+, FeedMe, Readrops, Fluent Reader Lite) * Lint * Fix API output enclosure * Fix API content strcut * API tolerate enclosures without a type

Better enclosures

de9a2e3

#fix FreshRSS#4702 Improvement of FreshRSS#2898

Alkarex added this to the 1.21.0 milestone Dec 14, 2022

Alkarex marked this pull request as draft December 14, 2022 22:45

Alkarex mentioned this pull request Dec 14, 2022

Help needed: $test2 does not work #4943

Closed

Alkarex added 4 commits December 18, 2022 12:16

A few fixes

88ca236

Better enclosure titles

d33c1c0

Improve thumbnails

f284d2c

Implement thumbnail for HTML+XPath

2e895cd

Alkarex marked this pull request as ready for review December 26, 2022 21:55

Alkarex added 2 commits December 29, 2022 16:50

Avoid duplicate enclosures

4f1d070

#fix FreshRSS#1668

Merge branch 'edge' into better-enclosures

ca83d9e

Alkarex mentioned this pull request Dec 29, 2022

Duplicate images #1668

Closed

Alkarex linked an issue Dec 29, 2022 that may be closed by this pull request

Duplicate images #1668

Closed

Alkarex added 3 commits December 29, 2022 17:10

Fix regex

d8f1779

Add basic support for media:credit

f687d45

And use <figure> for enclosures

Fix link encoding + simplify code

01d093e

Alkarex added the SimplePie 🍰 label Dec 30, 2022

Fix some SimplePie bugs

0917644

Encoding errors in enclosure links

Alkarex commented Dec 30, 2022

View reviewed changes

lib/SimplePie/SimplePie/Enclosure.php Show resolved Hide resolved

Alkarex commented Dec 30, 2022

View reviewed changes

Alkarex added 3 commits December 30, 2022 14:23

Remove debugging syslog

32ffe61

Remove debugging syslog

14e3d80

Merge branch 'edge' into better-enclosures

aad87d6

This was referenced Dec 30, 2022

[BUG] Enclosures: Youtube: Thumbnail + download icon, that is just a link #4970

Closed

[BUG] Enclosure: Image will not shown, if there are other types of enclosures #4973

Closed

Alkarex mentioned this pull request Dec 31, 2022

No URL Decode for enclosure links simplepie/simplepie#768

Merged

Alkarex mentioned this pull request Dec 31, 2022

Fix case of multiple RSS2.0 enclosures simplepie/simplepie#769

Merged

Alkarex mentioned this pull request Dec 31, 2022

Sanitize thumbnail URL simplepie/simplepie#770

Merged

Alkarex added 2 commits January 5, 2023 15:02

Improve thumbnails

c5c09ea

Merge branch 'edge' into better-enclosures

a905b26

Alkarex added 6 commits January 5, 2023 20:23

Performance with yield

8f8e547

Avoid generating all enclosures if not used

API keep providing enclosures inside content

4262b5c

Clients are typically not showing the enclosures to the users (tested with News+, FeedMe, Readrops, Fluent Reader Lite)

Lint

8d10bc7

Fix API output enclosure

92be57c

Fix API content strcut

9d1f7f9

API tolerate enclosures without a type

038f7ce

Alkarex added the Important 🔥 label Jan 6, 2023

Alkarex mentioned this pull request Jan 6, 2023

[Feature] Enclosures as part of greader API #4702

Closed

Alkarex merged commit 8f9c414 into FreshRSS:edge Jan 6, 2023

Alkarex deleted the better-enclosures branch January 6, 2023 18:54

math-GH mentioned this pull request Feb 5, 2023

Improved: Enclosures: HTML from Entry.php to enclosures.phtml #5072

Draft

2 tasks

math-GH mentioned this pull request Mar 5, 2023

[BUG] Preview images from YouTube feeds no longer working after updating to 1.21 #5170

Closed

bwees mentioned this pull request Aug 3, 2023

[BUG] Entry content is being appended after video players from extensions #5563

Open

Uh oh!

Conversation

Alkarex commented Dec 14, 2022

Uh oh!

math-GH commented Dec 30, 2022

Uh oh!

Alkarex commented Dec 30, 2022

Uh oh!

Alkarex commented Dec 30, 2022

Uh oh!

math-GH commented Dec 30, 2022

Uh oh!

Uh oh!

Alkarex Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex Dec 31, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex commented Dec 30, 2022

Uh oh!

math-GH commented Jan 5, 2023

Uh oh!

Alkarex commented Jan 5, 2023

Uh oh!

math-GH commented Jan 6, 2023

Uh oh!

Alkarex commented Jan 6, 2023

Uh oh!

math-GH commented Jan 6, 2023

Uh oh!

Alkarex commented Jan 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants