Introduce a caching layer in bu_navigation_get_posts
#48
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
September 26, 2019 11:47
This opts to create a new cache group - `bu-navigation-persistent` - due to the `bu-navigation` group being set as non-persistent (see https://github.com/bu-ist/bu-navigation/blob/master/bu-navigation.php#L86). A copy of core's `last_changed` cache value is stored in the new cache group, and will leveraged for resetting the cache of the query results.
added 4 commits
September 26, 2019 12:28
The previous approach had a significant flaw in that it could potentially never update the cache value for some calls to `bu_navigation_get_posts`, as the now-removed BU Navigation-specific `last_changed` key would be updated the first time the function was called.
Sorry...
acketon
requested review from
jayshreepraveen,
jdub233,
toddmilliken,
hirozed and
alana29s
and removed request for
jayshreepraveen
toddmilliken
approved these changes
Oct 9, 2019
includes/library.php
Outdated
| @@ -428,6 +428,27 @@ function bu_navigation_get_posts( $args = '' ) { | |||
| ); | |||
| $r = wp_parse_args( $args, $defaults ); | |||
|
|
|||
| // Get the `last_changed` key from the WP core `posts` cache group. | |||
| // This key is updated by core in `clean_post_cache`. | |||
| $last_changed = wp_cache_get( 'last_changed', 'posts' ); | |||
There was a problem hiding this comment.
I think we can do something like wp_cache_get_last_changed( 'posts' )? https://developer.wordpress.org/reference/functions/wp_cache_get_last_changed/
More or less the same amount of typing 😆
includes/library.php
Outdated
|
|
||
| // If WP's `last_changed` key has no value, set it using the current time. | ||
| if ( ! $last_changed ) { | ||
| wp_cache_set( 'last_changed', microtime(), 'posts' ); |
There was a problem hiding this comment.
Actually, the above function would mean you wouldn't have to do this if im reading the source code correctly for wp_cache_get_last_changed
There was a problem hiding this comment.
That looks right to me, too! Thanks for the suggestion!
Uses the working setup from bu-custom-roles which was pulled from latest wp scaffold plugin
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://trello.com/c/eU9qfTAa/30-ca-6-bu-navigation
This approach leverages keys stored in a new cache group -
bu-navigation-persistent- asbu-navigationis set as non-persistent.A copy of the
last_changedcache key value from WP core is stored along with the results of the query made in thebu_navigation_get_postsfunction. This is stored with a key that uses the arguments passed to the function, JSON encoded and wrapped inmd5, so that multiple unique calls to the function will retrieve their respective results.If the
last_changedvalue stored with the results of the query don't match the current WP core value oflast_changed, the cache for that call is reset.WP core's
last_changedkey is also reset whenever changes to the menu are saved.I think this approach makes sense, but I appreciate that my understanding of the plugin as a whole may not be complete enough, and I could be missing something.