Skip to content

Return effective feed URL when asking for non-permanent subscribe_url#627

Merged
mblaney merged 3 commits intosimplepie:masterfrom
jtojnar:redirect-subscribe-url
Oct 7, 2019
Merged

Return effective feed URL when asking for non-permanent subscribe_url#627
mblaney merged 3 commits intosimplepie:masterfrom
jtojnar:redirect-subscribe-url

Conversation

@jtojnar
Copy link
Member

@jtojnar jtojnar commented Oct 4, 2019

subscribe_url method supports two modes: permanent and non-permanent. And while the URL for the first one is obtained from the File after it has been fetched, discounting auto-discovery, the latter one remains the same from the time set_feed_url was called.

Because even the permanent mode concerns itself with redirection (although limited to a potential 301 as a first link in the chain), I would expect the non-permanent one to care doubly so.

This patch enables just that – after the feed is fetched, url property is updated, regardless of auto-discovery being done.

Fixes: #2

@mblaney
Copy link
Member

mblaney commented Oct 7, 2019

hi @jtojnar thanks for this, I notice you've added $suite->addTestSuite('SubscribeUrlTest'); to AllTests.php were you planning on adding another file?

@jtojnar jtojnar force-pushed the redirect-subscribe-url branch from d85ff41 to a802603 Compare October 7, 2019 08:02
@jtojnar
Copy link
Member Author

jtojnar commented Oct 7, 2019

Right, forgot to stage a new file again. I really need to start using something like https://github.com/hyperfekt/git-stagelight

@mblaney
Copy link
Member

mblaney commented Oct 7, 2019

@jtojnar your new test raises a notice, would you mind looking in to that too?

No tests really tried to parse this before but the prolog was invalid:

https://www.w3.org/TR/REC-xml/#NT-XMLDecl
@jtojnar jtojnar force-pushed the redirect-subscribe-url branch from a802603 to 50a26be Compare October 7, 2019 11:23
Atom feeds should have a namespace declared.
@jtojnar jtojnar force-pushed the redirect-subscribe-url branch from 50a26be to 0e0846b Compare October 7, 2019 11:29
@jtojnar
Copy link
Member Author

jtojnar commented Oct 7, 2019

Fixed the MockSimplePie_File data to be valid.

@jtojnar jtojnar force-pushed the redirect-subscribe-url branch from 0e0846b to f97ef52 Compare October 7, 2019 11:36
@jtojnar

This comment has been minimized.

`subscribe_url` method supports two modes: permanent and non-permanent.
And while the URL for the first one is obtained from the `File` after
it has been fetched, discounting auto-discovery, the latter one remains
the same from the time `set_feed_url` was called.

Because even the permanent mode concerns itself with redirection (although
limited to a potential 301 as a first link in the chain), I would expect
the non-permanent one to care doubly so.

This patch enables just that – after the feed is fetched, url property
is updated, regardless of auto-discovery being done.
@jtojnar jtojnar force-pushed the redirect-subscribe-url branch from f97ef52 to 59149ef Compare October 7, 2019 11:54
@mblaney
Copy link
Member

mblaney commented Oct 7, 2019

Nice one @jtojnar thanks for the extra work! :-)

@mblaney mblaney merged commit 5a3b23a into simplepie:master Oct 7, 2019
@jtojnar jtojnar deleted the redirect-subscribe-url branch October 8, 2019 00:11
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.

SimplePie::subscribe_url() should follow permanent redirects

2 participants