Skip to content

[Cache] Fix for RedisAdapter without auth parameter #48816

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
merged 1 commit into from
Dec 29, 2022
Merged

[Cache] Fix for RedisAdapter without auth parameter #48816

merged 1 commit into from
Dec 29, 2022

Conversation

rikvdh
Copy link
Contributor

@rikvdh rikvdh commented Dec 29, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #48813
License MIT
Doc PR x

Compatibility with Redis without auth was broken by #48711, this change fixes this.
This applies for all versions (6.x as well).

@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 6.3 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

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 29, 2022

I checked the errors from fabbot, but it seems to change switch-case constructions elsewhere in the file. Although being perhaps better according PSR, not intending on more changes than neccecairy.

@ThomasTr
Copy link
Contributor

Your changes works for me symfony v.6.2.3

@nicolas-grekas nicolas-grekas changed the title Fix for caching without auth parameter, broken by #48711, fix for #48813 Fix for RedisAdapter without auth parameter Dec 29, 2022
@carsonbot carsonbot changed the title Fix for RedisAdapter without auth parameter [Cache] Fix for RedisAdapter without auth parameter Dec 29, 2022
@chalasr
Copy link
Member

chalasr commented Dec 29, 2022

A test case would be useful.

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 29, 2022

@chalasr I thought about it. But I didn't know where to start without all kinds of complicated Redis spin-up stuff. But to prevent regression in the future it would be useful. It would probably require mocking a lot of stuff.

@nicolas-grekas
Copy link
Member

There's already a test case as this fixes our integration tests.

@chalasr
Copy link
Member

chalasr commented Dec 29, 2022

Thanks @rikvdh.

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.

5 participants