Skip to content
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

[Messenger] Add missing runtime check for ext redis version #31872

Merged
merged 1 commit into from Jun 5, 2019

Conversation

Projects
None yet
5 participants
@chalasr
Copy link
Member

commented Jun 5, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31791
License MIT
Doc PR n/a

Lower versions lead to segmentation faults.

@chalasr chalasr added this to the 4.3 milestone Jun 5, 2019

@fabpot

fabpot approved these changes Jun 5, 2019

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Build failure is unrelated.

@stof

stof approved these changes Jun 5, 2019

@@ -43,6 +44,10 @@ class Connection
public function __construct(array $configuration, array $connectionCredentials = [], array $redisOptions = [], \Redis $redis = null)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 5, 2019

Member

unrelated, but we missed making this class compatible with other kind of redis instances: RedisArray, RedisCluser, RedisProxy and RedisClusterProxy

This comment has been minimized.

Copy link
@stof

stof Jun 5, 2019

Member

are they compatible with redis streams ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 5, 2019

Member

I think so, because in the end this is just a key. https://redis.io/topics/streams-intro also mentions clusters. But I didn't try, so maybe not. Yet the hardcoded signature would be a blocker for the future.

This comment has been minimized.

Copy link
@chalasr

chalasr Jun 5, 2019

Author Member

I will give it a try and drop the typehint in favor of a check anyways.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Thank you @chalasr.

@fabpot fabpot merged commit 07603da into symfony:4.3 Jun 5, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 5, 2019

bug #31872 [Messenger] Add missing runtime check for ext redis versio…
…n (chalasr)

This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Add missing runtime check for ext redis version

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31791
| License       | MIT
| Doc PR        | n/a

Lower versions lead to segmentation faults.

Commits
-------

07603da [Messenger] Add runtime check for ext redis version

@chalasr chalasr deleted the chalasr:redis-version-check branch Jun 5, 2019

@fabpot fabpot referenced this pull request Jun 6, 2019

Merged

Release v4.3.1 #31901

@weaverryan weaverryan referenced this pull request Jun 12, 2019

Merged

Redis 4.3 is required #11732

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jun 13, 2019

minor #11732 Redis 4.3 is required (weaverryan)
This PR was merged into the 4.3 branch.

Discussion
----------

Redis 4.3 is required

See symfony/symfony#31872

But also the conversation on symfony/messenger@47d2b91

So, I'm not actually sure if that commit is correct and the docs should be updated, or if that commit is wrong. Opening this up to at least have some placeholder to figure that out.

Commits
-------

c76afd3 Redis 4.3 is required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.