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

[11.x] fix: remove use of Redis::COMPRESSION_ZSTD_MIN #51346

Merged
merged 1 commit into from
May 8, 2024

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented May 8, 2024

Hello!

Related Issue: phpredis/phpredis#2487
php: 8.2
phpredis: 6.0.2

When running the tests locally I would receive ~40 failures for Undefined constant Redis::COMPRESSION_ZSTD_MIN. After a lot of debugging/recompiling phpredis/git bisecting I finally discovered that the constant was removed between phpredis 5.7.3 and 6.0.0 (COMPRESSION_ZSTD_MAX and COMPRESSION_ZSTD_DEFAULT are still defined).

Given that this constant is only referenced in the tests and other tests cases cover the max and default compression constants, I just opted to remove the references to this particular constant.

Thanks!

@taylorotwell taylorotwell merged commit 2057d55 into laravel:11.x May 8, 2024
30 checks passed
@vlakoff
Copy link
Contributor

vlakoff commented May 16, 2024

The constant has been brought back upstream, see phpredis/phpredis#2490. Of course, it will need a new phpredis release, to even just start propagating.

For the (long) time being, we might want to restore the unit tests that has been removed here, enclosing them with
if (defined(Redis::COMPRESSION_ZSTD_MIN)) { /* ... */ }. Even better, if the constant is missing, we may use the value 1 instead, which should be a safe fallback.

That being said, the tests with Redis::COMPRESSION_ZSTD_MIN and with Redis::COMPRESSION_ZSTD_MAX are identical, so I don't know if it is useful to keep both. Though, nowadays Redis::COMPRESSION_ZSTD_MIN may be negative (see phpredis/phpredis#2490 (comment)), thus maybe an integration test for such value might be relevant.

@driesvints
Copy link
Member

Thanks @vlakoff. We can indeed restore the tests when the new version comes out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants