Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[11.x] Fix Cache component to be aware of phpredis serialization and compression settings #54221

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

TheLevti
Copy link
Contributor

@TheLevti TheLevti commented Jan 16, 2025

Summary

Fix Cache component to be aware of phpredis's serialization and compression settings.

--No breaking changes-- as the Cache component never worked properly with those settings enabled. If those settings are disabled, code path is the same as before.

Detailed Description

Three years ago, support for phpredis serialization and compression settings was added, working seamlessly with the Redis component. However, the Cache component is unaware of these settings and fails when they are set to anything other than NONE.

Issues:

  1. Double Serialization:
    The Cache component serializes/unserializes data before sending it to Redis. When phpredis serialization is enabled, this results in redundant serialization, causing inefficiency. Solution: Skip Cache-level serialization if phpredis serialization is enabled.

  2. EVAL Parameter Mismatch:
    EVAL scripts require parameter values to be prepared client-side, which differs from the serialized/compressed values stored in Redis. Without proper preparation, scripts may fail. Solution: Use phpredis’s internal pack method to ensure EVAL parameters are consistent with stored values.

Evidence

Screenshots demonstrate the issue when running existing tests using MSGPACK (serialization) and ZSTD (compression) enabled. Before the fix, the Cache component incorrectly php-serializes values, leaving them uncompressed and mismatched with stored values. After the fix, the EVAL parameters are smaller, correctly compressed, and match Redis-stored values.

Screenshot from 2025-01-16 22-37-11
Screenshot from 2025-01-16 22-38-05

TheLevti and others added 2 commits January 16, 2025 20:33
- Fix cache component not being able to handle serialized or compressed phpredis connections
@taylorotwell taylorotwell merged commit 4ae015b into laravel:11.x Jan 17, 2025
38 checks passed
@TheLevti TheLevti deleted the feature/improve-redis-cache branch January 17, 2025 17:32
@cyppe
Copy link

cyppe commented Feb 6, 2025

FYI - this was a breaking change for me that uses:

        'serializer'  => Redis::SERIALIZER_IGBINARY,
        'compression' => Redis::COMPRESSION_LZ4,

On sessions and cache since before.

Laravel could no longer unserialize sessions/cached data so I had to flush my redis and loose my sessions to come back online again.

Probably not very common to use this settings. And I have already overcome the trouble. But might be useful to know.

@TheLevti
Copy link
Contributor Author

TheLevti commented Feb 6, 2025

FYI - this was a breaking change for me that uses:

        'serializer'  => Redis::SERIALIZER_IGBINARY,
        'compression' => Redis::COMPRESSION_LZ4,

On sessions and cache since before.

Laravel could no longer unserialize sessions/cached data so I had to flush my redis and loose my sessions to come back online again.

Probably not very common to use this settings. And I have already overcome the trouble. But might be useful to know.

Thanks for the input. Unfortunately this went unnoticed as I missed that multiple other components apparently rely on this Cache component.

It was a change that fixed something that never worked actually. I did not expect that anyone is using the cache component with those settings :(.

I will better check next time where else the cache component is used.

On the bright side you now get properly serialized and compressed results in your redis instance :)

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

Successfully merging this pull request may close these issues.

3 participants