Skip to content

Fix #18191: Read tag timestamps once per request when reusable#20898

Open
WarLikeLaux wants to merge 2 commits into
yiisoft:masterfrom
WarLikeLaux:fix-tagdependency-reusable
Open

Fix #18191: Read tag timestamps once per request when reusable#20898
WarLikeLaux wants to merge 2 commits into
yiisoft:masterfrom
WarLikeLaux:fix-tagdependency-reusable

Conversation

@WarLikeLaux

Copy link
Copy Markdown
Contributor
Q A
Is bugfix? ✔️
New feature?
Tests added? ✔️
Breaks BC?
Fixed issues #18191

What does this PR do?

TagDependency ignored the inherited reusable flag: its isChanged() override and generateDependencyData() both call getTimestamps(), which queried the cache backend on every call. So even with reusable => true, reusing one dependency for many cache reads in a single request hit the backend each time.

getTimestamps() now caches the fetched tag timestamps per request in a static property when reusable is set, so each tag is read from the backend only once. touchKeys() refreshes that cache for keys it writes, so an invalidate() in the same request is still detected. resetReusableData() clears it alongside the base data.

With reusable => false (the default) the method still queries the backend on every call, so existing behavior is unchanged.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9216c764-fc94-40e5-b9e9-f61fa8ecf203

📥 Commits

Reviewing files that changed from the base of the PR and between fa383c8 and 524d173.

📒 Files selected for processing (1)
  • framework/CHANGELOG.md
📜 Recent review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: WarLikeLaux
Repo: yiisoft/yii2 PR: 20900
File: framework/CHANGELOG.md:19-19
Timestamp: 2026-05-29T04:35:28.008Z
Learning: In the `yiisoft/yii2` repository, `framework/CHANGELOG.md` entries should reference the pull request number, not necessarily the original issue number. The original fixed issue may be tracked separately in the PR description Q/A table under “Fixed issue”.
📚 Learning: 2026-05-29T04:35:28.008Z
Learnt from: WarLikeLaux
Repo: yiisoft/yii2 PR: 20900
File: framework/CHANGELOG.md:19-19
Timestamp: 2026-05-29T04:35:28.008Z
Learning: In the `yiisoft/yii2` repository, `framework/CHANGELOG.md` entries should reference the pull request number, not necessarily the original issue number. The original fixed issue may be tracked separately in the PR description Q/A table under “Fixed issue”.

Applied to files:

  • framework/CHANGELOG.md
🔇 Additional comments (1)
framework/CHANGELOG.md (1)

19-21: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Tag dependency timestamp handling now honors the reusable flag, reducing redundant cache reads when checking dependency validity multiple times within a single request.
  • Tests

    • Added tests covering reusable vs non-reusable tag behavior, including that reusable timestamps are fetched only once per request and that invalidation within the same request is still detected.

Walkthrough

TagDependency now implements per-request timestamp caching to honor the reusable flag. The change introduces a private static cache store, updates getTimestamps() to conditionally retrieve and cache timestamps, modifies touchKeys() to update the cache, and adds tests and a changelog entry.

Changes

Per-request tag timestamp caching

Layer / File(s) Summary
Per-request timestamp cache implementation
framework/caching/TagDependency.php
TagDependency adds a private static $_reusableTimestamps storage keyed by cache key. getTimestamps() now fetches only missing timestamps into the reusable store and assembles results from cached values when reuse is enabled, or fetches all timestamps directly when disabled. touchKeys() updates the reusable cache, and resetReusableData() clears it alongside the parent reset.
Test infrastructure and validation
tests/framework/caching/TagDependencyTest.php
Test suite adds a setUp() override that resets reusable state before each test and a createCountingCache() helper that records cache reads. Three new test methods validate that reusable dependencies fetch timestamps once per request, non-reusable dependencies fetch on each call, and reusable dependencies detect tag invalidation within the same request.
Release notes
framework/CHANGELOG.md
Changelog entry documents bug fix #20898 noting that TagDependency now honors the reusable flag for per-request timestamp caching.

Poem

🐰 I hop through timestamps, snug and neat,
One fetch per request keeps my paws light on feet,
Touch nudges the store, reset clears the trail,
Tests count the reads and prove the tale,
A rabbit's small cheer for caching done right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: implementing per-request timestamp caching for TagDependency when the reusable flag is set, directly addressing issue #18191.
Description check ✅ Passed The description clearly explains the problem (TagDependency ignoring the reusable flag), the solution (per-request timestamp caching), and how the changes maintain backward compatibility.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.45%. Comparing base (d77f4bb) to head (524d173).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##             master   #20898   +/-   ##
=========================================
  Coverage     80.44%   80.45%           
- Complexity    11540    11547    +7     
=========================================
  Files           374      374           
  Lines         30235    30250   +15     
=========================================
+ Hits          24323    24338   +15     
  Misses         5912     5912           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant