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 issues in HTTP client #106

Merged
merged 2 commits into from
Jun 15, 2024
Merged

Fix issues in HTTP client #106

merged 2 commits into from
Jun 15, 2024

Conversation

MayPaw
Copy link
Collaborator

@MayPaw MayPaw commented Jun 8, 2024

Summary

This pull request introduces fixes to the HTTP client as described in #104.

Major Changes:

  • temporary change of protocol HTTP/1.1 -> HTTP/1.0 - HTTP/1.0 doesn't support Transfer-Encoding: chunked and thus temporarily solves the dechunking problem, until some more robust solution is implemented,
  • additional null-check for Content-Length header, when assigning total bytes size in HTTP client, which otherwise resulted in an PHP Notice: Undefined index: content-length
  • removal of excessive line-ending replacement - the explicit replacement from "\n" to "\r\n" broke sending requests on Windows.

How to Test

After checking out to the feature branch, run blueprint_compiling_simple_progress.php script. The script passes all steps connected with downloading and installing WordPress (but still does not complete).

Notes

  • the blueprint_compiling_simple_progress.php script fails on my machine at step DefineWpConfigConsts, at the moment I'm not sure why, but I'll create a separate issue describing the problem
  • the change of protocol is only temporary and will allow further work on Add e2e tests #82 @reimic

…-ending's replacement, add null check for 'content-length' header (WordPress#104)
@reimic reimic added bug Something isn't working HTTP Client labels Jun 8, 2024
@reimic reimic requested a review from adamziel June 8, 2024 18:19
@reimic
Copy link
Collaborator

reimic commented Jun 8, 2024

Splendid! Welcome aboard, Maya.

Check this out @adamziel

For a bit of housekeeping - this is also relevant to #60

@@ -230,7 +230,7 @@ function stream_http_prepare_request_bytes( $url ) {

// @TODO: Add support for Accept-Encoding: gzip

return str_replace( "\n", "\r\n", $request ) . "\r\n\r\n";
return $request . "\r\n\r\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we'd still get inconsistent newline style depending on where this file was last saved and where is it used. Let's declare each line of the request separately and concatenate them with the \r\n sequence so the result is always the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The newest change introduces single newline style \r\n.

@adamziel adamziel merged commit 239f43e into WordPress:trunk Jun 15, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working HTTP Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants