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

Add support for PSR-18 HTTP clients #777

Merged
merged 42 commits into from
Aug 26, 2023

Conversation

Art4
Copy link
Contributor

@Art4 Art4 commented Jan 27, 2023

Hi all 👋

This is a follow up of #774 and will add support for every PSR-18 HTTP client implementation. 🥳

This PR will fixes #520. I propose this PR for SimplePie 1.9.0, see #731.

@@ -71,8 +73,17 @@ class Sniffer
*
* @param File $file Input file
*/
public function __construct(File $file)
public function __construct(/* File */ $file)
Copy link
Contributor Author

@Art4 Art4 Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the type hint is not a breaking change because we have not released 1.9.0 yet.

@Art4 Art4 marked this pull request as ready for review February 17, 2023 11:40
@Art4
Copy link
Contributor Author

Art4 commented Feb 17, 2023

This PR is ready for review. Because this PR is build on #774 I recommend to merge this PR first. It will fix some BC breaks that comes with #774.

To show only the changes between #774 and #777 you can look at the diff between the branches: Art4/simplepie@split-file-into-client-and-response...Art4:simplepie:add-psr18-http-client-support

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to resolve the question of non-absolute Location header and Redirect tracking.

$requestedUrl = $response->getHeaderLine('Location');

if ($statusCode === 301) {
$permanentUrl = $requestedUrl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some kind of mechanism to not change the permanent URL after temporary redirects (as tested by #819). For example, this is what File does:

$this->permanentUrlMutable = $this->permanentUrlMutable && ($this->status_code == 301 || $this->status_code == 308);

@mblaney
Copy link
Member

mblaney commented Aug 25, 2023

thanks @Art4 this looks good, can we merge and handle the redirect case separately?

@Art4
Copy link
Contributor Author

Art4 commented Aug 25, 2023

@mblaney I resolved all conflicts and fixed all PHPStan errors.

@mblaney mblaney merged commit 8f45077 into simplepie:master Aug 26, 2023
9 checks passed
@Art4 Art4 deleted the add-psr18-http-client-support branch August 26, 2023 09:18
jtojnar added a commit to jtojnar/simplepie that referenced this pull request Nov 6, 2023
…ent-support"

There are correctness issues as well as disorganized commit history.
The fixed changes are re-applied in the commits that follow.
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.

Migration to a HTTP library
3 participants