-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Fixed query string parsing #41427
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
[Messenger] Fixed query string parsing #41427
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
This seems to broke some parameters with special characters including |
@YaFou |
fd74d28
to
786c074
Compare
@YaFou I thought I needed to test the parsing process, which can be complicated, so I divided it into classes and wrote tests for them. |
No, I mean special characters that use |
@YaFou |
...fony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/AmazonSqsQueryStringParserTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Bridge/AmazonSqs/Transport/AmazonSqsQueryStringParser.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Bridge/AmazonSqs/Transport/AmazonSqsQueryStringParser.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, you need to rebase the branch to 5.2 because it is a bug fix.
6bf1062
to
4e17e3f
Compare
@YaFou |
4bf3d92
to
22d9281
Compare
22d9281
to
3e6e7b6
Compare
@derrabus |
We should avoid doing custom encoding for DSNs. I'm thus 👎 |
Okay. 👍
I tried it this way. Is this behavior expected?
|
I'm also 👎 on this as this is not necessarily a bug. PHP treats query strings as url-form-encoded. There + means space. |
…cess_key and secret_key (77web) This PR was merged into the 5.2 branch. Discussion ---------- [Messenger] add description to amazon-sqs-transport's access_key and secret_key Added description according to reviews to this PR symfony/symfony#41427 Commits ------- e10d284 add description to amazon-sqs-transport's access_key and secret_key
Hi!
I love Symfony, it's a very useful framework.
While using it, I found a bug and would like to make a pull request.
The secret_key issued by Amazon SQS may contain a "+".
Before the fix, when you parse
A+a+A
, there was a problem that you cannot access Amazon SQS because of the unintended change as follows.(This is the change that happens when using
parse_str()
.)Run parse_str()
before
after
In this pull request, we will fix it to use the exact secret_key by changing it to not use
parse_str()
.