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

Fix case of multiple RSS2.0 enclosures #769

Merged
merged 14 commits into from
Jan 19, 2023

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Dec 31, 2022

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

This line was introduced in simplepie@8a5f601#diff-a710c236cc7775672edbe5840d8fb96b0e91b0d56eff709f6eace2da47c1fe43R349-R359

Seems to be a similar error than simplepie@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.
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
tests/Unit/EnclosureTest.php Outdated Show resolved Hide resolved
Co-authored-by: Artur Weigandt <Art4@users.noreply.github.com>
src/Item.php Outdated Show resolved Hide resolved
@jtojnar
Copy link
Contributor

jtojnar commented Jan 1, 2023

It is not clear from the RSS spec if multiple enclosures are valid but it makes sense to me to err on the side of accepting it since the ambiguity allows feed producers to do this.

https://www.rssboard.org/rss-validator/docs/warning/DuplicateEnclosure.html

Alkarex and others added 2 commits January 2, 2023 09:50
Co-authored-by: Jan Tojnar <jtojnar@gmail.com>
Co-authored-by: Jan Tojnar <jtojnar@gmail.com>
@Alkarex
Copy link
Contributor Author

Alkarex commented Jan 2, 2023

For the record, when using the <media:content> syntax, the specification explicitly accounts for multiple enclosures https://www.rssboard.org/media-rss#media-content
This PR adds tests for both the <media:content> syntax and the RSS 2.0 <content> syntax, and the fix was only needed for the RSS 2.0 syntax.

@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 12, 2023
@mblaney mblaney merged commit 1ef386d into simplepie:master Jan 19, 2023
Art4 added a commit to Art4/simplepie that referenced this pull request Jan 20, 2023
@Alkarex Alkarex deleted the multiple-rss2-enclosures branch January 20, 2023 07:34
mblaney pushed a commit that referenced this pull request Jan 20, 2023
* bump version to 1.8.0

* Update CHANGELOG.md

* Fix version tags in deprecated messages

* fix version in old deprecation messages

* Fix typo

see comment from @jtojnar in #752

* Add comment for DataCache interface

see comment from @jtojnar in #752

* Update CHANGELOG.md for #760, #764 and #765

* Update CHANGELOG.md for #762, #767 and #763

* Update CHANGELOG.md for #768 and #770

* Update release date

* Update CHANGELOG.md for #769 and #771

* Update CHANGELOG.md for #766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants