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

[10.x] Ensure array cache considers milliseconds #48573

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Sep 28, 2023

With the recent fix to the array cache driver, #48497, it has surfaced another problem with it that was previously hidden by the initial issue.

The array cache driver, unlike the redis cache driver, does not take milliseconds into account.

This PR fixes that.

This is technically a breaking change with the return type changes, but one I think we should make as it is really a bugfix. The array driver should always have used milliseconds.

fixes #48569

@rez1dent3
Copy link
Contributor

/subcribe pr

@driesvints driesvints marked this pull request as draft September 28, 2023 07:34
@driesvints
Copy link
Member

Tests seem to fail here @timacdonald

@rez1dent3
Copy link
Contributor

rez1dent3 commented Sep 28, 2023

Guys, I think we need to revert the changes from the PR and only then working on improvements. It seems to me that the ArrayCache functionality is not working quite correctly right now. I described the problem here: #48569

@taylorotwell Hello. What do you think about this?

@driesvints
Copy link
Member

We've reverted the original PR for now

@rez1dent3
Copy link
Contributor

  - Locking laravel/framework (v10.25.2)

@driesvints I confirm that all tests have begun to pass. Pipeline green.
https://github.com/bavix/laravel-wallet/actions/runs/6321648269

Thank you.

@timacdonald
Copy link
Member Author

timacdonald commented Oct 2, 2023

I've rebased and included the original change here as well.

@timacdonald
Copy link
Member Author

@rez1dent3 I've tested this change against the testsuite passes. I repeated the suite 20 times and it passed everytime (it was flaky with the original fix but without this millisecond fix).

@timacdonald timacdonald marked this pull request as ready for review October 2, 2023 00:26
@taylorotwell taylorotwell merged commit 388c835 into laravel:10.x Oct 2, 2023
20 checks passed
@timacdonald timacdonald deleted the array-cache-milliseconds branch October 2, 2023 00:41
@rez1dent3
Copy link
Contributor

rez1dent3 commented Oct 2, 2023

@timacdonald Yes, this option works correctly. I checked it on my tests.

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.

ArrayCache TTL is less than the specified expire
4 participants