Skip to content

Ignore locale when creating the HTTP version string from a float#339

Merged
rmccue merged 2 commits into
WordPress:masterfrom
Zegnat:fix-decimal-mark
Sep 16, 2019
Merged

Ignore locale when creating the HTTP version string from a float#339
rmccue merged 2 commits into
WordPress:masterfrom
Zegnat:fix-decimal-mark

Conversation

@Zegnat
Copy link
Copy Markdown
Contributor

@Zegnat Zegnat commented Apr 28, 2019

This should fix #335.

Rather than using %f, this should use the non-locale aware %F. That way PHP will always use a period (.) for the decimal separator in the HTTP version, and never fallback to a comma (,).

I wasn’t sure how to add a test for this, as I’d want to intercept the socket message somehow. But to illustrate the issue:

php > echo setlocale(LC_NUMERIC, 0);
C
php > echo sprintf("%s %s HTTP/%.1f\r\n", 'GET', '/', 1.1);
GET / HTTP/1.1
php > echo setlocale(LC_NUMERIC, 'de_DE');
de_DE
php > echo sprintf("%s %s HTTP/%.1f\r\n", 'GET', '/', 1.1);
GET / HTTP/1,1
php > echo sprintf("%s %s HTTP/%.1F\r\n", 'GET', '/', 1.1);
GET / HTTP/1.1

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #339 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #339   +/-   ##
=========================================
  Coverage     92.11%   92.11%           
  Complexity      760      760           
=========================================
  Files            21       21           
  Lines          1762     1762           
=========================================
  Hits           1623     1623           
  Misses          139      139
Impacted Files Coverage Δ Complexity Δ
library/Requests/Transport/fsockopen.php 94.18% <100%> (ø) 68 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf35eb2...e52c155. Read the comment docs.

@Zegnat
Copy link
Copy Markdown
Contributor Author

Zegnat commented Sep 16, 2019

Not sure why #335 was closed as a support issue while the bug identified in there, and patched here, is still there.

Remerged with master as per #346 (comment) in the hope to get all tests to pass and get this PR accepted. Something still seems wrong with Travis? Seeing errors connecting to localhost, even from cURL when this patch only touches the fsocket code.

Let me know what I can do to get this resolved!

@rmccue
Copy link
Copy Markdown
Collaborator

rmccue commented Sep 16, 2019

Reopened #335, apologies.

The tests are likely failing due to transient issues; the nature of working with servers is that occasionally things fail for no reason. I've restarted those tests.

@rmccue
Copy link
Copy Markdown
Collaborator

rmccue commented Sep 16, 2019

Tests are now passing after restarting, thanks!

@rmccue rmccue merged commit e4fe0eb into WordPress:master Sep 16, 2019
@Zegnat
Copy link
Copy Markdown
Contributor Author

Zegnat commented Sep 16, 2019

Many thanks for merging! 🎊

@jrfnl
Copy link
Copy Markdown
Member

jrfnl commented Aug 30, 2021

I've opened PR #521 to add a regression test for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Always get "Bad request (400)"

4 participants