Skip to content

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

// @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.

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