Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpClient] Fix over-encoding of URL parts to match browser's behavior#49299

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:5.4fromnicolas-grekas:hc-url-dec
Feb 10, 2023

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#48315
LicenseMIT
Doc PR-

Yes, the RFC says these chars should be url-encoded:

[ ] \ ^ ` { | }

But in practice, browser's don't encode them, and some servers don't expect them to be encoded either.
Seehttps://stackoverflow.com/questions/2366260/whats-valid-and-whats-not-in-a-uri-query for some pointers.

billhance reacted with heart emoji
@carsonbotcarsonbot added this to the5.4 milestoneFeb 8, 2023
@nicolas-grekasnicolas-grekas changed the titleRemove unused data provider[HttpClient] Fix over-encoding of URL parts to match browser's behaviorFeb 8, 2023
@kwisatz
Copy link

kwisatz commentedMar 29, 2023
edited
Loading

For what it's worth, this is breaking an API client to WooCommerce for us.

With 5.4.20

> GET /wp-json/wc/v3/orders?status%5B0%5D=processing&status%5B1%5D=completed&after=2023-03-29T10%3A35%3A09%2B02%3A00&before=2023-03-29T11%3A04%3A18%2B02%3A00 HTTP/2Host: gazon-naturel-en-rouleau.frpragma: No-cacheaccept:*/*authorization: Basic**********user-agent: Symfony HttpClient/Curlaccept-encoding: gzip* old SSL session ID is stale, removing* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!< HTTP/2 200

With 5.4.21

> GET /wp-json/wc/v3/orders?status[0]=processing&status[1]=completed&after=2023-03-29T10:35:09+02:00&before=2023-03-29T11:02:48+02:00 HTTP/2Host: gazon-naturel-en-rouleau.frpragma: No-cacheaccept:*/*authorization: Basic*************user-agent: Symfony HttpClient/Curlaccept-encoding: gzip* old SSL session ID is stale, removing* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!< HTTP/2 400

This is how the request is constructed:

$response =$client->request('GET',$this->currentShop['endpoint'],    ['auth_basic' => [$this->currentShop['api_key'],$this->currentShop['api_secret']        ],'headers' => ['Pragma' =>'No-cache',        ],'query' => ['status' => ['processing','completed'],'after' =>$dates['after']->modify('-5 minutes')->format('c'),'before' =>$dates['before']->modify('-5 minutes')->format('c'),        ],'on_progress' =>function ()use ($downloadProgress):void {$downloadProgress->advance();        },    ]);

@nicolas-grekas
Copy link
MemberAuthor

Which part/character? You can know by generating the query string on your own to test.

@kwisatz
Copy link

It's the plus signs here.
Guessing the problem is that un-encoded plus signs stand for space, but here, they stand for timezone offsets. So they must be encoded to plus signs, not spaces.

@nicolas-grekas
Copy link
MemberAuthor

OK, so this is#49579, fixed in the next release.

@kwisatz
Copy link

kwisatz commentedMar 29, 2023 via email

Indeed, I had seen that ticket actually but not made the connection, sorry.

@Kozzi11
Copy link

Kozzi11 commentedJun 15, 2023
edited
Loading

This has broken our aplication should not we stick confrontant to RFC?

@chalasr
Copy link
Member

@Kozzi11 Please open a new issue if you think this introduced a bug.
Also note that unlike your application, Symfony is mostly non-profit work so please use a more relaxed language.

Kozzi11 reacted with thumbs up emoji

@Kozzi11
Copy link

Kozzi11 commentedJun 15, 2023
edited
Loading

@Kozzi11 Please open a new issue if you think this introduced a bug. Also note that unlike your application, Symfony is mostly non-profit work so please use a more relaxed language.

@chalasr Yeah you are right, I appologize for that. I have created Issue#50670 and PR#50671

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

Reviewers

@OskarStarkOskarStarkAwaiting requested review from OskarStarkOskarStark is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas@kwisatz@Kozzi11@chalasr@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp