Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Lock][Messenger] Fix precedence of DSN options for 5.1#37269
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJun 14, 2020
There are some failures to take care of. |
b409d15 toc4942b8Compare| parse_str($parsedUrl['query'],$query); | ||
| } | ||
| $this->options['collection'] =$this->options['collection'] ??$query['collection'] ??null; | ||
| $this->options['collection'] =$query['collection'] ??$this->options['collection'] ??null; |
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.
Isn't this a BC Break? someone's application could start writing to the wrong collection if they are specifying both for some reason and they differ. Also there's the path in the uri which should take precedence over$options['database']
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.
I would rather we deal withLock\Store\MongoDbStore in#37218
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.
see#37218 (comment) about BC vs bugfix
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.
there's the path in the uri which should take precedence over $options['database']
@jderusse can you have a look at this, also in the other PR?
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.
good catch. Also fix precedenceof credentials in that PR
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.
did not found precendence in#37268@nicolas-grekas . Did you have a specific thing in mind?
132b5ea to9670e9fComparenicolas-grekas commentedJun 18, 2020
Thank you@jderusse. |
… (jderusse)This PR was merged into the 5.1 branch.Discussion----------[Messenger] Fix invalid option sslmode in AmazonSqs bridge| Q | A| ------------- | ---| Branch? | 5.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | />| License | MIT| Doc PR | /The sslmode option allows people to use AmazonSqs with non-offical endpoint like the [asyncaws/testing-sqs](https://hub.docker.com/r/asyncaws/testing-sqs) docker imageBy fixing precedence of DNS options in#37269 I introduced a bug that trigger an exception `Unknown option found: [sslmode]`. I apologize for thisThis PR adds `sslmode` in list of allowed optionsCommits-------afbd51b Fix invalid option sslmode
Uh oh!
There was an error while loading.Please reload this page.
This PR fix précédence of DSN options over constructor options in all component on branch 5.1