Skip to content

fix(Cache): use __ci_value field in PredisHandler::increment() and decrement() - #10758

Open
banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/predis-increment-decrement
Open

banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/predis-increment-decrement

Conversation

@banghamid

Copy link
Copy Markdown
Contributor

Description
PredisHandler::save() and PredisHandler::get() store and read cached values in a Redis hash using the '__ci_type' and '__ci_value' fields. However, PredisHandler::increment() and PredisHandler::decrement() were still calling hincrby() on the legacy 'data' field instead of '__ci_value'.

As a result, calling increment() or decrement() modified the unused 'data' hash field while leaving '__ci_value' unchanged, causing subsequent calls to get() to return the stale value.

This PR:

  • Updates PredisHandler::increment() and PredisHandler::decrement() to target the '__ci_value' hash field (consistent with RedisHandler::increment()).
  • Adds testIncrementAndDecrement() to PredisHandlerTest.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@carson-codeigniter4 carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 10, 2026
@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Thank you @banghamid for spotting this and submitting a clean fix!

Review Summary: Approve

  • Correctness: PredisHandler::save() and PredisHandler::get() have always operated on the '__ci_type' and '__ci_value' hash fields. Targeting '__ci_value' in increment() and decrement() aligns PredisHandler with both its own read/write contract and RedisHandler::increment().
  • Backward Compatibility: Preserves existing method signatures, visibility, and return types on the develop branch.
  • Tests: The added testIncrementAndDecrement() serves as a proper regression test (fails on the previous implementation and passes with this fix).

Optional suggestion (nitpick / non-blocking)

Both increment() and decrement() return the newly computed value (int). In testIncrementAndDecrement(), you might consider also asserting the return value directly on at least one call to ensure the return contract is validated alongside get():

$this->assertSame(135, $this->handler->decrement('counter', 20));
$this->assertSame(135, $this->handler->get('counter'));

@banghamid
banghamid requested a review from gr8man November 11, 2026 18:39
@banghamid

Copy link
Copy Markdown
Contributor Author

thanks @gr8man! good catch, makes sense to check the return value directly too.

just pushed an update to testIncrementAndDecrement() to assert the returned int from increment() and decrement() as well.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants