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

Set phpredis multi() mode parameter #1412

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Set phpredis multi() mode parameter #1412

merged 1 commit into from
Dec 20, 2019

Conversation

fbnfgc
Copy link
Contributor

@fbnfgc fbnfgc commented Dec 12, 2019

Set the default function parameter to avoid potential issues when a Redis proxy is used instead of the internal class.

This issue has been discussed in phpredis/phpredis#1476 (comment) and snc/SncRedisBundle#442.

This may be Symfony specific, but I'd be happy to hear your thoughts on this issue.

@Seldaek
Copy link
Owner

Seldaek commented Dec 19, 2019

This is fine I guess, but should be rebased on 1.x branch as the problem exists there too.

@Seldaek Seldaek added this to the 1.x milestone Dec 19, 2019
@Seldaek Seldaek added the Bug label Dec 19, 2019
@fbnfgc fbnfgc changed the base branch from master to 1.x December 19, 2019 13:50
@fbnfgc
Copy link
Contributor Author

fbnfgc commented Dec 19, 2019

Just cherry-picked the commit and applied it to the 1.x branch.

Tests now fails but it shouldn't.

1) Monolog\Handler\RedisHandlerTest::testRedisHandleCapped
Error: Undefined class constant 'MULTI'

I need to dig into, some tests were skipped because phpredis is not available on the CI.

@Seldaek
Copy link
Owner

Seldaek commented Dec 19, 2019

Yeah the extension isn't loaded I believe in the tests.. Either mark more tests as skipped or maybe fix the patch so it uses defined('Redis::MULTI') ? Redis::MULTI : 1 or whatever the value of Redis::MULTI is when the ext is loaded..

@fbnfgc
Copy link
Contributor Author

fbnfgc commented Dec 19, 2019

Thanks, fixed as suggested.

@Seldaek
Copy link
Owner

Seldaek commented Dec 20, 2019

Thanks

@Seldaek Seldaek merged commit d25bb38 into Seldaek:1.x Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants