Skip to content

[HttpFoundation] Allow RedisCluster class for RedisSessionHandler #28251

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 26, 2018
Merged

[HttpFoundation] Allow RedisCluster class for RedisSessionHandler #28251

merged 1 commit into from
Aug 26, 2018

Conversation

michaelperrin
Copy link
Contributor

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

The RedisSessionHandler added in Symfony 4.1 was supposed to accept RedisCluster instances but that possibility was not in the condition, leading to an InvalidArgumentException.

I formatted the object type condition to allow the RedisCluster instance value and also added a test for it.

A double space formatting issue has been fixed too (which is included in the same commit, I can change that if needed).

I could not add a test using RedisCluster, as the installed Redis instance on Travis doesn't use clusters.

@michaelperrin michaelperrin force-pushed the fix/redis-session-allow-cluster branch from ec7d4aa to 0ed7dbd Compare August 23, 2018 18:23
@michaelperrin michaelperrin changed the title Allow RedisCluster class for RedisSessionHandler [HttpFoundation] Allow RedisCluster class for RedisSessionHandler Aug 23, 2018
nicolas-grekas
nicolas-grekas requested changes Aug 23, 2018
View reviewed changes
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.

Thanks

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php
!$redis instanceof \RedisArray &&
!$redis instanceof \RedisCluster &&
!$redis instanceof \Predis\Client &&
!$redis instanceof RedisProxy
Copy link
Member

Choose a reason for hiding this comment

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

Missing \, same in the docblock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you talk about the RedisProxy class? That one is imported at the top of the class.

Copy link
Member

Choose a reason for hiding this comment

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

Right sorry!

michaelperrin
michaelperrin commented Aug 23, 2018
View reviewed changes
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php
if (
!$redis instanceof \Redis &&
!$redis instanceof \RedisArray &&
!$redis instanceof \RedisCluster &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas Just to make things easier to see, this is the condition I added

nicolas-grekas
nicolas-grekas approved these changes Aug 23, 2018
View reviewed changes
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php
!$redis instanceof \RedisArray &&
!$redis instanceof \RedisCluster &&
!$redis instanceof \Predis\Client &&
!$redis instanceof RedisProxy
Copy link
Member

Choose a reason for hiding this comment

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

Right sorry!

@nicolas-grekas nicolas-grekas mentioned this pull request Aug 24, 2018
Merged
nicolas-grekas added a commit that referenced this pull request Aug 24, 2018
@nicolas-grekas
minor #28255 [travis] enable Redis cluster (nicolas-grekas)
a682051 
This PR was merged into the 3.4 branch.

Discussion
----------

[travis] enable Redis cluster

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Will allow testing #28251 also

Commits
-------

f245df4 [travis] enable Redis cluster
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Aug 24, 2018
@nicolas-grekas
Copy link
Member

@michaelperrin I added a Redis cluster in #28255. Can you please rebase and add a test case by taking inspiration from this PR?

@michaelperrin michaelperrin force-pushed the fix/redis-session-allow-cluster branch 2 times, most recently from d10fa94 to 08e92bf Compare August 24, 2018 16:31
@michaelperrin
Copy link
Contributor Author

Thank you very much @nicolas-grekas, that is indeed useful to have a Redis cluster for tests.

I updated tests in order to use it for tests using RedisSessionHandler.

I didn't get why an extra Redis instance was created to make checks after values are written, instead of using the same Redis connection instance.
The "validator" instance was always using the Redis class connecting to a given host (thus not working when using RedisCluster).

To make everything work, I removed the validator in tests and used instead the redisClient instance, which is of the correct type whatever the test.

Maybe you or @dkarlovi will know purpose of the validator Redis instance.

Could you make sure that my changes are alright then? If everything looks fine, I will squash commits into one.

@dkarlovi
Copy link
Contributor

Validator is supposed to be used to connect to the test version of Redis and confirm the intended action happened regardless of the adapter used. Probably shouldn't be changed, but you be the judge, I'm on mobile.

@stof
Copy link
Member

stof commented Aug 24, 2018

@dkarlovi but shouldn't this redisClient be used to connect to redis ? That way, it works for RedisCluster too (otherwise, you would connect to the wrong redis)

@michaelperrin
Copy link
Contributor Author

Thanks @dkarlovi for your insight :) I better understand now why you added this other instance.
I would say that it may not be necessary as it is like testing if the Redis libraries work correctly, but I would prefer to be sure.

If we decide it is better to keep the "validator" instance, I will override the other methods in the new RedisClusterSessionHandlerTest class to make things work.

@dkarlovi
Copy link
Contributor

The idea is to have a stand-alone instance which independently confirms the intended action happened. Otherwise the adapter is testing itself and we're supposed to trust it implicitly.

@nicolas-grekas
Copy link
Member

it is like testing if the Redis libraries work correctly

I agree with this statement, so I'm fine with a single Redis connection personally.

@nicolas-grekas nicolas-grekas force-pushed the fix/redis-session-allow-cluster branch from 08e92bf to d2ecea0 Compare August 26, 2018 08:23
@nicolas-grekas
Copy link
Member

Thank you @michaelperrin.

@nicolas-grekas nicolas-grekas merged commit d2ecea0 into symfony:4.1 Aug 26, 2018
nicolas-grekas added a commit that referenced this pull request Aug 26, 2018
@nicolas-grekas
bug #28251 [HttpFoundation] Allow RedisCluster class for RedisSession…
aa8b9c0 
…Handler (michaelperrin)

This PR was squashed before being merged into the 4.1 branch (closes #28251).

Discussion
----------

[HttpFoundation] Allow RedisCluster class for RedisSessionHandler

| Q             | A
| ------------- | ---
| Branch?       | 4.1 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |    <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |

The `RedisSessionHandler` added in Symfony 4.1 was supposed to accept `RedisCluster` instances but that possibility was not in the condition, leading to an `InvalidArgumentException`.

I formatted the object type condition to allow the `RedisCluster` instance value and also added a test for it.

A double space formatting issue has been fixed too (which is included in the same commit, I can change that if needed).

I could not add a test using RedisCluster, as the installed Redis instance on Travis doesn't use clusters.

Commits
-------

d2ecea0 [HttpFoundation] Allow RedisCluster class for RedisSessionHandler
@michaelperrin
Copy link
Contributor Author

Thanks @nicolas-grekas for the merge and the help!
Looking forward to next Symfony (minor) release :)

@dkarlovi
Copy link
Contributor

Nice work @michaelperrin! 👍

@michaelperrin
Copy link
Contributor Author

@dkarlovi It is based on your work, so thanks to you ;)

@fabpot fabpot mentioned this pull request Aug 28, 2018
Merged
@xabbuh xabbuh added the Cache label Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

Assignees
No one assigned
Labels
Cache Status: Reviewed
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
6 participants
@michaelperrin @nicolas-grekas @dkarlovi @stof @xabbuh @carsonbot