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

Fix course structure ordering cache #4236

Merged
merged 3 commits into from
Jul 5, 2021
Merged

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Jul 1, 2021

Fixes #3891

Changes proposed in this Pull Request

  • It fixes the issue related to cache when ordering the course structure.
    • Notice that it only happens when using memcached for object cache. With Redis, using the plugin "Redis Object Cache" I couldn't reproduce the issue.
  • The chosen strategy was to avoid cache in some specific cases while getting the structure for edit. It was done by adding a new conditional to the WHERE query. It seems the way that solves for any extension. My first option was trying to clear some cache objects, but in the plugin I used, for example, it had some specific names, so we couldn't clear it in a generic way for any object cache extension.

Testing instructions

You can configure your local env (2 first steps), or you can test it in Pressable or an Ephemeral site.

  • Configure and start memcached.
  • Configure the object cache in WordPress to use the memcached. I used the plugin Docket Cache for this.
  • Create a course with modules and lessons (use lessons inside modules and lessons without modules to make sure all scenarios are working properly).
  • Reorder the lessons, save the post, and make sure the lessons continue in the correct place (see the issue for more details).
  • Go to WP admin > Lessons > Order Lessons.
  • Select your course.
  • Reorder the lessons, and make sure it works properly.

renatho added 2 commits July 1, 2021 15:15
A cache error happened when using memcached to store object cache.
So it avoids the cache when using extension for object cache.
@renatho renatho added this to the 3.11.2 milestone Jul 1, 2021
@renatho renatho requested review from jom and donnapep July 1, 2021 18:31
@renatho renatho self-assigned this Jul 1, 2021
jom
jom previously approved these changes Jul 5, 2021
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well! I had one tiny comment, but not a big deal at all if you think you'd prefer merging as-is.

* @return string Where with extra condition to avoid cache.
*/
public function filter_no_cache_where( $where ) {
return $where . ' AND ' . time() . ' = ' . time();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would ever happen, but I wonder if it is a tiny-tiny-tiny bit safer to set time() to a variable here. I mean, we'd have to be really unlucky and I doubt PHP would ever do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course! Fixed here: c0fffe1

@renatho renatho requested a review from jom July 5, 2021 20:06
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@renatho renatho merged commit ac64e72 into master Jul 5, 2021
@renatho renatho deleted the fix/structure-ordering-cache branch July 5, 2021 21:05
@donnapep donnapep changed the title Fix structure ordering cache Fix course structure ordering cache Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module lesson order updates not persisted with object cache
2 participants