Skip to content

Add inline object caching to HPPS repositories#7904

Merged
donnapep merged 27 commits into
trunkfrom
add/hpps-inline-caching
Mar 6, 2026
Merged

Add inline object caching to HPPS repositories#7904
donnapep merged 27 commits into
trunkfrom
add/hpps-inline-caching

Conversation

@donnapep

@donnapep donnapep commented Feb 26, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Adds inline object caching to all six HPPS (High-Performance Progress Storage) Tables_Based repositories, improving read performance by avoiding repeated database queries for the same progress/submission data within a request.

Architecture:

  • Cache_Prefix trait — Shared cache infrastructure with prefix-rotation for group invalidation (modeled after WooCommerce's CacheNameSpaceTrait). Uses wp_cache_add() + re-read to mitigate thundering herd on prefix initialization.
  • Inline caching — Read-through pattern directly in repository methods (get(), get_all()). Write methods (create(), save(), delete()) invalidate individual entries; bulk deletes rotate the group prefix.
  • Feature flag — All caching gated by Progress_Storage_Settings::is_cache_enabled(), which defaults to true when HPPS tables are active. Filterable via sensei_hpps_cache_enabled. Memoized to prevent mid-request instability.
  • Sentinel pattern'__not_found__' sentinel distinguishes "cached null" from "cache miss", preventing repeated DB queries for non-existent entities.
  • find() cache warming — After DB queries, find() warms individual caches for returned objects without caching the query itself.

Repositories cached:

Repository Cache Group Key Format
Course Progress sensei_course_progress {course_id}_{user_id}
Lesson Progress sensei_lesson_progress {lesson_id}_{user_id}
Quiz Progress sensei_quiz_progress {quiz_id}_{user_id}
Submissions sensei_quiz_submissions {quiz_id}_{user_id}
Answers sensei_quiz_answers {submission_id}
Grades sensei_quiz_grades {submission_id}

Not cached (by design): has(), find() query results, count(), get_question_ids() — complex/cross-table queries where invalidation would be fragile, or lightweight queries where caching adds unnecessary overhead.

Testing Instructions

Prerequisites: Enable HPPS: WP Admin → Sensei → Settings → Experimental → set progress storage to Custom tables. Install Query Monitor for query inspection.

1. No stale data after completing a lesson

  • As a student, open a lesson page and note the progress status
  • Complete the lesson
  • Verify the progress updates immediately on the next page load (both the lesson page and the course page) — no stale state

2. Quiz submission flow

  • Start a quiz, submit answers, and complete it
  • Verify submission, answers, and grades display correctly on the results page
  • Retake the quiz if retakes are enabled — verify previous results are replaced, not stale

3. Query reduction with persistent object cache (optional, requires Redis or Memcached)

  • With a persistent object cache configured (e.g., Redis Object Cache plugin — requires the phpredis or Predis PHP extension), visit a course page as a student
  • Open Query Monitor → Queries and note queries to wp_sensei_lms_progress
  • Reload the same page — progress queries should be significantly reduced or absent on the second load, since data is now served from the persistent cache

New/Updated Hooks

  • sensei_hpps_cache_enabled (filter) — Controls whether HPPS repository caching is active. Receives a boolean (default: result of is_tables_repository()). Return false to disable caching.

🤖 Generated with Claude Code

donnapep and others added 11 commits February 26, 2026 14:54
Introduces a shared trait mirroring WooCommerce's CacheNameSpaceTrait
that provides prefix-based cache group invalidation. This enables
rotating a prefix key to make all cached entries in a group unreachable
without requiring wp_cache_flush_group(), which is not universally
available.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a filterable feature flag that gates all HPPS cache operations.
Defaults to enabled when tables-based storage is active, controllable
via the sensei_hpps_cache_enabled filter.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds read-through object caching with sentinel values to the course,
lesson, and quiz progress Tables_Based repositories. Refactors has()
to delegate to get() for cache reuse. find() warms individual caches
via wp_cache_set_multiple(). Bulk deletes use prefix rotation for
group invalidation. Filter cleanup moved to tearDown() to prevent
leakage on assertion failure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds read-through object caching to the submission, answer, and grade
Tables_Based repositories. Submissions cache get() with per-key
invalidation on save/delete. Answers and grades cache get_all() by
submission_id, with cache deletion on create/delete_all/save_many.
Filter cleanup moved to tearDown() to prevent leakage on assertion
failure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Prefix trait

- Extract '__not_found__' magic string to $cache_not_found static property
  for centralized sentinel management across all consuming repositories
- Replace wp_cache_set with wp_cache_add + re-read in get_cache_prefix()
  to mitigate thundering herd when the prefix key is evicted
- Add @SInCE 4.24.0 tags to all three trait methods

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache the filter result in a static property so is_cache_enabled()
returns a consistent value throughout the request. Add
reset_cache_enabled() for test teardown.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use trait sentinel property instead of magic string
- Skip cache population on failed DB insert (id=0)
- Remove duplicate filter from has() since get() already filters
- Hoist is_cache_enabled() call out of find() loop
- Add @SInCE 4.24.0 to CACHE_GROUP constants
- Fix @intenal typos and wrong constructor names in docblocks

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use trait sentinel property instead of magic string
- Skip cache invalidation on failed DB insert
- Add @SInCE 4.24.0 to CACHE_GROUP constants
- Add missing @return tag to get_answer_ids_by_submission_id()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests for answer create cache invalidation, grade create and
save_many cache invalidation, and has() delegation to get() for
lesson/quiz progress. Add reset_cache_enabled() calls in tearDown
to prevent filter leakage between tests. Rename $array parameter
to $data to satisfy PHPCS naming rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add native string type to $cache_not_found, add false fallback in
get_cache_prefix() re-read, add sentinel overwrite tests for all 3
progress repos, add is_cache_enabled() unit tests, and add
failed-insert-no-cache test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 26, 2026 20:39
@donnapep donnapep self-assigned this Feb 26, 2026
@donnapep donnapep added this to the 4.26.0 milestone Feb 26, 2026
@donnapep donnapep marked this pull request as draft February 26, 2026 20:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Introduces request-scoped object caching for HPPS (tables-based) progress/submission repositories to reduce repeated DB reads within a request, with a shared cache-prefix invalidation mechanism and a feature-flag gate.

Changes:

  • Added Cache_Prefix trait to support cache-group invalidation via prefix rotation plus a not-found sentinel.
  • Implemented inline read-through caching + targeted invalidation in tables-based repositories (course/lesson/quiz progress, submissions, answers, grades).
  • Added unit/integration tests for cache enablement, memoization, invalidation behavior, and prefix logic.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
includes/internal/trait-cache-prefix.php New shared trait for prefix-based cache namespacing and group invalidation.
includes/internal/services/class-progress-storage-settings.php Adds memoized is_cache_enabled() feature flag with sensei_hpps_cache_enabled filter and test reset helper.
includes/internal/student-progress/course-progress/repositories/class-tables-based-course-progress-repository.php Adds inline caching for get() and warming in find(), plus invalidation on writes/deletes.
includes/internal/student-progress/lesson-progress/repositories/class-tables-based-lesson-progress-repository.php Same inline caching + invalidation for lesson progress repository.
includes/internal/student-progress/quiz-progress/repositories/class-tables-based-quiz-progress-repository.php Same inline caching + invalidation for quiz progress repository.
includes/internal/quiz-submission/submission/repositories/class-tables-based-submission-repository.php Adds caching/invalidation for submissions get() and write paths.
includes/internal/quiz-submission/answer/repositories/class-tables-based-answer-repository.php Adds caching/invalidation for answers get_all() and write paths.
includes/internal/quiz-submission/grade/repositories/class-tables-based-grade-repository.php Adds caching/invalidation for grades get_all() and write paths.
tests/unit-tests/internal/test-class-cache-prefix.php New tests covering cache prefix generation and invalidation behavior.
tests/unit-tests/internal/services/test-class-progress-storage-settings.php Tests new cache-enabled flag defaults, filter override, and memoization/reset.
tests/unit-tests/internal/student-progress/course-progress/repositories/test-class-tables-based-course-progress-repository.php Adds caching/invalidation tests for course progress repository.
tests/unit-tests/internal/student-progress/lesson-progress/repositories/test-class-tables-based-lesson-progress-repository.php Adds caching/invalidation tests for lesson progress repository.
tests/unit-tests/internal/student-progress/quiz-progress/repositories/test-class-tables-based-quiz-progress-repository.php Adds caching/invalidation tests for quiz progress repository.
tests/unit-tests/internal/quiz-submission/submission/repositories/test-class-tables-based-submission-repository.php Adds caching/invalidation tests for submission repository.
tests/unit-tests/internal/quiz-submission/answer/repositories/test-class-tables-based-answer-repository.php Adds caching/invalidation tests for answer repository.
tests/unit-tests/internal/quiz-submission/grade/repositories/test-class-tables-based-grade-repository.php Adds caching/invalidation tests for grade repository.
changelog/add-hpps-inline-caching Changelog entry for the performance change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

donnapep and others added 11 commits February 26, 2026 15:47
The get_all() methods in answer and grade repositories return [] for
empty results. Since [] !== false, empty arrays can be cached directly
without the __not_found__ sentinel, which is only needed for methods
that return null.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reintroduce the sensei_*_progress_has_*_id filters in all three
progress repository has() methods — these are public hooks since 4.23.1
used by WPML and other integrations. Removing them was a
backward-incompatible regression.

Fix CacheDisabled_DoesNotCache tests in all 6 repositories to assert
on the cache prefix marker key instead of an un-prefixed key, which
would always return false regardless of whether caching occurred.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… trait

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The cache group already contains 'sensei_', so the prefix key was
producing 'sensei_sensei_...' unnecessarily.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Also add AGENTS.md rule requiring assertion messages when a test has
multiple assertions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace get() delegation with a direct SELECT COUNT(*) query in
course, lesson, and quiz tables-based progress repositories. This
avoids fetching all columns, hydrating full entity objects, and
populating the cache when callers only need a boolean existence check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove testHas_CacheEnabled_DelegatesToGet from all three progress
repository test files since has() no longer delegates to get().
Reorganize tests so all tests for each method (unit + cache) are
grouped together instead of having cache tests in a separate section.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert read-caching tests (ReturnsCachedValueOnSecondCall,
CachesNullAsNotFound/CachesEmptyResult, CreateOverwritesNotFoundSentinel,
WarmsIndividualCaches) from integration tests using global $wpdb to unit
tests using mocked wpdb with expects() assertions. This proves the second
call actually hits cache instead of DB, which the previous tests could not
verify. Also removes redundant get() calls in invalidation tests where
create() already warms the cache via write-through caching.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@donnapep donnapep marked this pull request as ready for review March 2, 2026 20:36
@donnapep donnapep added the Hooks This change adds or modifies one or more hooks. label Mar 2, 2026
@donnapep

donnapep commented Mar 2, 2026

Copy link
Copy Markdown
Member Author

@merkushin This one adds caching.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread includes/internal/trait-cache-prefix.php
Comment thread includes/internal/trait-cache-prefix.php
@merkushin

Copy link
Copy Markdown
Contributor

Overall, the solution is straightforward but fine.

The original idea was to have caching wrappers: simpler logic, separate concern.

donnapep and others added 3 commits March 3, 2026 12:02
…Memcached keys

microtime() returns "0.12345600 1712345678" which contains a space.
While most WordPress Memcached drop-ins sanitize keys, not all do.
Stripping the space defensively ensures valid keys with any backend.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensures filtered values are cast to int before use in cache keys and
queries, matching the pattern used in all other repositories.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces inline cache key construction with a private method per
repository, making the key format a single source of truth.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@donnapep

donnapep commented Mar 3, 2026

Copy link
Copy Markdown
Member Author

The original idea was to have caching wrappers: simpler logic, separate concern.

@merkushin The first approach I took was to use caching decorators, but I didn't like the number of extra files it added, plus there were issues with flushing the cache so I decided to pivot to Woo's approach.

@merkushin

Copy link
Copy Markdown
Contributor

plus there were issues with flushing the cache

Do you remember what the issue was? (My personal interest.)

@donnapep

donnapep commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

Do you remember what the issue was? (My personal interest.)

Claude wanted to use wp_cache_flush_group which wouldn't always work depending on the type of object caching. I wish I hadn't deleted that branch. 🙁

@mdawaffe mdawaffe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable to me. One question inline about group invalidation, but I don't think it's a blocker.

Comment on lines +334 to +336
if ( Progress_Storage_Settings::is_cache_enabled() ) {
self::invalidate_cache_group( self::CACHE_GROUP );
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these ->delete_for_{object}() and ->delete_for_user() methods common operations? Invalidating the entire cache just to delete one user's data could be painful for some sites.

Other possibilities:

  • SELECT to get IDs then DELETE either in one transaction (atomic but more expensive), or in separate queries (racey but cheap).
  • DELETE … RETURNING: just what we need but not portable.
  • "Nested" prefixes so that we could invalidate the whole outer group or just individual nested ones. This is sort of a pain and doesn't actually solve the whole problem: there are multiple relevant ways to nest: group -> {object}_id, or group -> user_id. Picking one doesn't help the invalidation of the other, so it's only useful if one is common and the other is rare. This feels like too much complexity to me.

I may have talked myself into this simpler invalidate-all approach :) Are there sites that are currently using the custom tables? If so, they don't have any cacheing anyway so even a cacheing strategy with "inefficient" invalidations is an improvement, right?

@donnapep donnapep Mar 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Are these ->delete_for_{object}() and ->delete_for_user() methods common operations?

They're not. It should be quite rare which is why invalidating the entire cache seems OK. Of course, people do unexpected things so I can't say this with any kind of certainty. 😅

Are there sites that are currently using the custom tables?

Some, but likely not that many since this feature is labelled as experimental and we didn't promote it widely due to gaps like the one this PR addresses.

* @param {bool} $enabled Whether caching is enabled.
* @return {bool} Whether caching should be enabled.
*/
self::$cache_enabled = (bool) apply_filters( 'sensei_hpps_cache_enabled', self::is_tables_repository() );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No action needed, but the WP Cache API is a little annoying here. I could see a site wanting a persistent cache (like Memcached) for most of their site but to only use a in-memory (not persisted) cache for Sensei. That's possible, but it requires different tweaking depending on what WP Cache dropin the site has installed, so it isn't anything we can do in the plugin.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could reconsider this in the future if we get reports about people wanting to do this.

donnapep and others added 2 commits March 6, 2026 12:32
…positories

Cache invalidation in create(), save_many(), and delete_all() now applies
the same filter as get_all() when computing cache keys, ensuring consistent
invalidation when filters (e.g. WPML) remap submission IDs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@donnapep donnapep merged commit c32ee1d into trunk Mar 6, 2026
23 checks passed
@donnapep donnapep deleted the add/hpps-inline-caching branch March 6, 2026 18:30
@donnapep donnapep added the HPPS label Mar 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Hooks This change adds or modifies one or more hooks. HPPS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants