Skip to content

[4.2] Redis cache#38490

Merged
roland-d merged 6 commits intojoomla:4.2-devfrom
alikon:patch-10
Aug 18, 2022
Merged

[4.2] Redis cache#38490
roland-d merged 6 commits intojoomla:4.2-devfrom
alikon:patch-10

Conversation

@alikon
Copy link
Copy Markdown
Contributor

@alikon alikon commented Aug 17, 2022

Pull Request for Issue #38473 .

Summary of Changes

deal with INFinite 😄
introduced by #37457

Testing Instructions

use redis Cache Handler

Actual result BEFORE applying this Pull Request

Redis::setex(): Argument #2 ($expire) must be of type int, float given

Expected result AFTER applying this Pull Request

works as before

@roland-d
Copy link
Copy Markdown
Contributor

@Stuartemk could you give this a test please and see if it resolves your issue?

@RodHub
Copy link
Copy Markdown

RodHub commented Aug 17, 2022

@roland-d I had the same issue and it's solved with this fix.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38490.

@RodHub
Copy link
Copy Markdown

RodHub commented Aug 17, 2022

I have tested this item ✅ successfully on f1a8fd2

Issue is gone after replacing RedisStorage.php with the fixed version.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38490.

@roland-d
Copy link
Copy Markdown
Contributor

@alikon How do I get the lifetime to be infinite?

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 17, 2022

@roland-d check linked PR
@alikon Probably we better change INF to a 1 year 31557600 (or a couple years):

// Media version cache never expire
$cache->setLifeTime(INF);

It will be more safe than hacking every potential issue with Cache drivers.

You will update or I do PR?
Set it to 10 years 315576000 😉

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Aug 17, 2022

in redis term if something "never expire" the there is no need to setex() just store it set()

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 17, 2022

in redis term if something "never expire" the there is no need to setex() just store it set()

Yeah, but we also better cover our backs.
Because other drivers may have similar issue, they may expect integer for lifetime, while PHP INF is float.

@alikon alikon requested a review from laoneo as a code owner August 17, 2022 15:18
@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Aug 17, 2022

@Fedik changed INF to 315576000

@roland-d
Copy link
Copy Markdown
Contributor

@alikon the RedisStorageCache file should not show a change anymore and it looks like it has a tab/space too many. Could you check please?

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 17, 2022

I have tested this item ✅ successfully on abadf05

Cannot do a real test, but on review looks good. Changing INF Float to Integer for a lifetime value


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38490.

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 17, 2022

@RodHub can you please do one more test? Thanks in advance

@fancyFranci
Copy link
Copy Markdown
Contributor

fancyFranci commented Aug 18, 2022

The code comment before the changed line does not fit any more. Could you please edit it and explain the 10 years in the comment? :)

@zero-24 zero-24 added this to the Joomla 4.2.1 milestone Aug 18, 2022
The comment is not applicable anymore
@roland-d
Copy link
Copy Markdown
Contributor

RTC

@roland-d roland-d merged commit d6357ce into joomla:4.2-dev Aug 18, 2022
@alikon alikon deleted the patch-10 branch August 23, 2022 15:32
heelc29 added a commit to heelc29/joomla-cms that referenced this pull request Nov 20, 2022
@n3t n3t mentioned this pull request Jan 25, 2023
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.

7 participants