Skip to content

[Messenger] Fix Redis messenger scheme comparison #52930

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

Merged

Conversation

freswa
Copy link

@freswa freswa commented Dec 7, 2023

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

Commit 3380518 introduced a new Redis Sentinel DSN for the redis messenger transport which uses a scheme syntax like redis:?host[rs:1234]&host[rs2:1234].
Though, the coresponding factory only supports schemes which start with redis:// or rediss:// which renders the redis sentinel features for the messenger unusable. This commit fixes the supported schemes by removing the // portion of them.

fixes #52899

@carsonbot
Copy link

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:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title fix redis messenger transport bridge supports [Messenger] fix redis messenger transport bridge supports Dec 8, 2023
@OskarStark OskarStark changed the title [Messenger] fix redis messenger transport bridge supports [Messenger] Fix Redis messenger scheme comparison Dec 8, 2023
@xabbuh
Copy link
Member

xabbuh commented Dec 9, 2023

Can you please add a test (for supports() as well as for createTransport())?.

@OskarStark
Copy link
Contributor

I pushed some tests

@OskarStark OskarStark force-pushed the fix-redis-sentinel-messenger-transport branch from 963129f to 1c821bc Compare December 9, 2023 11:41
@OskarStark OskarStark force-pushed the fix-redis-sentinel-messenger-transport branch 2 times, most recently from 68c69cd to 20846e2 Compare December 9, 2023 11:44
@freswa freswa force-pushed the fix-redis-sentinel-messenger-transport branch 2 times, most recently from 6441d08 to 7477e5f Compare December 12, 2023 13:50
@nicolas-grekas
Copy link
Member

Please squash your PR and/or allow edits from maintainers.

Commit 3380518 introduced a new Redis Sentinel DSN for the redis messenger transport which uses a scheme syntax like
`redis:?host[rs:1234]&host[rs2:1234]`.
Though, the coresponding factory only supports schemes which start with `redis://` or `rediss://` which renders the redis sentinel features for the messenger
unusable. This commit fixes the supported schemes by removing the `//` portion of them.

fixes symfony#52899
@freswa freswa force-pushed the fix-redis-sentinel-messenger-transport branch from 59ab0f9 to 6f4e0a4 Compare January 2, 2024 13:11
@freswa
Copy link
Author

freswa commented Jan 2, 2024

Squashed.

@nicolas-grekas
Copy link
Member

Thank you @freswa.

@nicolas-grekas nicolas-grekas merged commit 712ccf5 into symfony:6.4 Jan 2, 2024
This was referenced Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants