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.

Sign up for GitHub

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

Jump to bottom

[HttpClient][WebProfilerBundle] Do not generate cURL command when files are uploaded #52472

Merged
merged 1 commit into from Nov 6, 2023
Merged

[HttpClient][WebProfilerBundle] Do not generate cURL command when files are uploaded #52472

merged 1 commit into from Nov 6, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Nov 6, 2023 •
edited

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #51366
License MIT

I also removed @requires extension openssl annotations since that does not seem to be the case since #45729.

Failures in AppVeyor occur because double quotes are missing around --data-raw values. Possibly related to #52429.

@carsonbot carsonbot added this to the 6.3 milestone Nov 6, 2023
@nicolas-grekas
Copy link
Member

Up to fix the quote issue in the same PR for tests?

@MatTheCat
Copy link
Contributor Author

@nicolas-grekas I'm not sure what should be done regarding these double quotes since it seems to be Windows-specific 🤔

@nicolas-grekas
Copy link
Member

I think we should just adjust the expected output depending on Windows or not. Failing tests should pass.

@nicolas-grekas
Copy link
Member

Or maybe @alexandre-daubois can provide the fix for the tests?

@nicolas-grekas
Copy link
Member

Thank you @MatTheCat.

@nicolas-grekas nicolas-grekas merged commit b8bba36 into symfony:6.3 Nov 6, 2023
7 of 9 checks passed
@MatTheCat MatTheCat deleted the ticket_51366 branch November 6, 2023 17:59
@MatTheCat
Copy link
Contributor Author

I'll look into it tomorrow if @alexandre-daubois hasn't in the meantime.

@alexandre-daubois alexandre-daubois mentioned this pull request Nov 6, 2023
Merged
@alexandre-daubois
Copy link
Contributor

alexandre-daubois commented Nov 6, 2023 •
edited

Thank you for proposing your help Mathieu! Trying to understand what I missed in my PR 😄

@MatTheCat
Copy link
Contributor Author

From what I saw (did not investigate further), HttpClientDataCollector::escapePayload returns different strings whether on Unix or Windows, but HttpClientDataCollector::provideCurlRequests expects the Unix return. That would explain why passing tests fail on AppVeyor.

nicolas-grekas added a commit that referenced this pull request Nov 7, 2023
@nicolas-grekas
minor #52473 [HttpKernel] Fix quotes expectations in tests (alexandre…
c752607 
…-daubois)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Fix quotes expectations in tests

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Following #52472 (comment)

On Windows, quotes are only added when necessary. This means that for a value like `foobar`, no quotes are added. I updated the test so the first dataset shows the escaping whereas the second updated dataset shows that it is expected that no quotes are added around data.

Commits
-------

dcc465f [HttpKernel] Fix quotes expectations in tests
This was referenced Nov 10, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review WebProfilerBundle
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MatTheCat @nicolas-grekas @alexandre-daubois @carsonbot