TheRedisSessionHandler added in Symfony 4.1 was supposed to acceptRedisCluster instances but that possibility was not in the condition, leading to anInvalidArgumentException.

I formatted the object type condition to allow theRedisCluster 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 force-pushed thefix/redis-session-allow-cluster branch fromec7d4aa to0ed7dbdCompare

August 23, 2018 18:23

michaelperrin changed the title~~Allow RedisCluster class for RedisSessionHandler~~[HttpFoundation] Allow RedisCluster class for RedisSessionHandler

Aug 23, 2018

carsonbot added the Status: Needs Work label

Aug 23, 2018

nicolas-grekas requested changes

Aug 23, 2018

View reviewed changes

Copy link

Member

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

		!$redisinstanceof \RedisArray &&
		!$redisinstanceof \RedisCluster &&
		!$redisinstanceof \Predis\Client &&
		!$redisinstanceof 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

ContributorAuthor

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 commented

Aug 23, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

		if (
		!$redisinstanceof \Redis &&
		!$redisinstanceof \RedisArray &&
		!$redisinstanceof \RedisCluster &&

Copy link

ContributorAuthor

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

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Aug 23, 2018

nicolas-grekas approved these changes

Aug 23, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

		!$redisinstanceof \RedisArray &&
		!$redisinstanceof \RedisCluster &&
		!$redisinstanceof \Predis\Client &&
		!$redisinstanceof 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 mentioned this pull request

Aug 24, 2018

[travis] enable Redis cluster#28255

Merged

stof approved these changes

Aug 24, 2018

View reviewed changes

nicolas-grekas added a commit that referenced this pull request

Aug 24, 2018

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 alsoCommits-------f245df4 [travis] enable Redis cluster

nicolas-grekas added this to the4.1 milestone

Aug 24, 2018

Copy link

Member

nicolas-grekas commentedAug 24, 2018

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

michaelperrin force-pushed thefix/redis-session-allow-cluster branch 2 times, most recently fromd10fa94 to08e92bfCompare

August 24, 2018 16:31

Copy link

ContributorAuthor

michaelperrin commentedAug 24, 2018

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 usingRedisSessionHandler.

I didn't get why an extraRedis instance was created to make checks after values are written, instead of using the same Redis connection instance.
The "validator" instance was always using theRedis 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 theredisClient instance, which is of the correct type whatever the test.

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

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

Copy link

Contributor

dkarlovi commentedAug 24, 2018

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.

Copy link

Member

stof commentedAug 24, 2018

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

Copy link

ContributorAuthor

michaelperrin commentedAug 24, 2018

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 newRedisClusterSessionHandlerTest class to make things work.

Copy link

Contributor

dkarlovi commentedAug 24, 2018

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.

Copy link

Member

nicolas-grekas commentedAug 24, 2018

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.

[HttpFoundation] Allow RedisCluster class for RedisSessionHandler

d2ecea0

nicolas-grekas force-pushed thefix/redis-session-allow-cluster branch from08e92bf tod2ecea0Compare

August 26, 2018 08:23

Copy link

Member

nicolas-grekas commentedAug 26, 2018

Thank you@michaelperrin.

nicolas-grekas merged commitd2ecea0 intosymfony:4.1

Aug 26, 2018

nicolas-grekas added a commit that referenced this pull request

Aug 26, 2018

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     <!-- seehttps://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

Copy link

ContributorAuthor