-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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] Reduce the number of queries with Cache::many
and Cache::putMany
methods in the database driver
#52209
[11.x] Reduce the number of queries with Cache::many
and Cache::putMany
methods in the database driver
#52209
Conversation
…se store cache driver
The tests seem to be broken in |
* | ||
* @return array | ||
*/ | ||
public function many(array $keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://redberry.international/laravel-10-features-and-updates/#mcetoc_1hfrpmpr11v
please put : array as return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and remove the dockblock @return pls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep the same method signature as the interface we're implementing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are implementing an interface I agree, but I see this as a new function that is not in the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param int $seconds | ||
* @return bool | ||
*/ | ||
public function putMany(array $values, $seconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analog pls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Store multiple items in the cache for a given number of seconds.
*/
public function putMany(array $values, int $seconds): bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I'd rather keep the same method signature as the interface we're implementing. Plus, the type of the $seconds variable cannot be enforced because of the interface definition.
@tonysm This seems interesting. Is the idea to store multiple models with the |
@francoism90 no, it's about caching view partials, and being able to retrieve all fragments in a single query is essential there. See the Rails Guide on this, it's a similar idea, but using my own Blade components or directives to achieve the same result. |
Changelog
many
andputMany
methods in theDatabaseStore
cache driver. The previous implementation was doing a database query for each key, but now it does only 1 query for all (2 if it finds expired keys)I was exploring fragment caching on my views... then found out the
many
method of the database cache driver was doing a database query for each cache key. The new implementation does only one query for all keys and then builds the results from what it finds. Additionally, it deletes the expired keys it finds in a single query as well.The
putMany
had a similar story: it was doing a database query for each key. Now, it only does oneupsert
.I made the
get
andput
methods use themany
andput
ones behind the scenes so we can rely on the same implementation. This step is optional and we can revert the commit, if y'all don't want that.