Skip to content

[Cache] Support Redis cluster connections with predis/predis:^2.0 #49801

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
Apr 19, 2023

Conversation

mfettig
Copy link
Contributor

@mfettig mfettig commented Mar 24, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

In v2 of predis/predis some classes related to cluster connections were reorganized. This PR expands the checks in RedisTrait to also work with the new version of Predis.

Previous tests appear to cover this change, and I didn't see any updates needed to documentation, etc. but please let me know if I missed anything. I have been running this locally for a couple of days and while my usage has been limited it has been working well so far

I did change the composer constraint for predis/predis from tilde to caret which I was a bit hesitant about, but they should be functionally equivalent in this case and it looked like the caret constraint was more standard. Let me know if that is not an appropriate change and I will switch back to the tilde constraint.

@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

@nicolas-grekas
Copy link
Member

Thanks for the PR. Can you have a look at failures? They look related to me.
About the deprecation, Predis2 should add @return foo annotation to the methods that trigger them. Would you mind sending them a PR to do so?

@mfettig mfettig force-pushed the predis2-support branch 2 times, most recently from b93a8d2 to 52ee088 Compare March 30, 2023 15:35
@mfettig
Copy link
Contributor Author

mfettig commented Mar 30, 2023

@nicolas-grekas yes, I didn't really know about the integration tests until they failed and they caught that the connection parameters had changed in Predis v2. I updated the test so the PredisClusterSessionHandlerTest should pass now. I also rebased my branch again on 6.3

There is now a problem with the integration test Notifier/Bridge/SimpleTextin/Tests/SimpleTextinTransportFactoryTest.php which looks like its overriding methods but not keeping the static keyword. Not sure if a change there would be appropriate in this branch, but it is keeping the integration test from running fully

The other problem I see is Psalm failing on classes that don't exist.
1: Class, interface or enum named Predis\Connection\Aggregate\RedisCluster does not exist
2: Class, interface or enum named Relay\Relay does not exist

For item 1 I guess that is because I tried to keep support for Predis v1, but only v2 is installed at the time the test is ran. Is there a way to handle that or can only one version be supported due to the class changes?

For item 2 I'm not familiar with Relay or how it's being tested. Do you have any recommendations for that error?

@stof
Copy link
Member

stof commented Mar 30, 2023

For item 1 I guess that is because I tried to keep support for Predis v1, but only v2 is installed at the time the test is ran. Is there a way to handle that or can only one version be supported due to the class changes?

Let it complain about that. It won't do it again in the future, as we use the target branch as baseline for the reporting, so we report only new issues.

@mfettig
Copy link
Contributor Author

mfettig commented Apr 4, 2023

Freshly rebased on the 6.3 branch. The failure in SimpleTextinTransportFactoryTest has been resolved so the integration tests were able to run fully

Integration test results still have the the deprecation notices for Countable::count() and IteratorAggregate::getIterator(), but I did look at predis\predis and it looks like the maintainers have addressed that for now with #[ReturnTypeWillChange]
https://github.com/predis/predis/blob/v2.x/src/Connection/Cluster/RedisCluster.php#L594
https://github.com/predis/predis/blob/v2.x/src/Connection/Cluster/RedisCluster.php#L603

Psalm checks report on the non-existent classes for Predis v1 and Relay as mentioned before

AppVeyor shows an error on Symfony\Component\VarDumper\Tests\Caster\ExceptionCasterTest::testFlattenException, which I don't believe is related to the changes made

Please let me know if there is anything more I should do for this PR

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted predis/predis#1265 to fix the deprecation notices.

@nicolas-grekas
Copy link
Member

Thank you @mfettig.

@nicolas-grekas nicolas-grekas merged commit 3dfec48 into symfony:6.3 Apr 19, 2023
nicolas-grekas added a commit that referenced this pull request May 2, 2024
This PR was merged into the 5.4 branch.

Discussion
----------

[Cache] Fix support for predis/predis:^2.0

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

This PR backports #49801 to 5.4.
That's the best way to fix the remaining deprecations.

Commits
-------

5ab2d90 [Cache] Fix support for predis/predis:^2.0
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.

4 participants