test(#3616606): cover the active updating skip path - #25
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe hook now resolves an entity’s original revision once with Drupal-version compatibility. Rector excludes this legacy access from automated rewriting. A new kernel test verifies file movement when active updating is disabled or enabled. ChangesActive updating behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds focused coverage for the active-updating behavior and keeps existing files in place when updating is disabled without introducing new file-operation capabilities. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.x-1.x #25 +/- ##
========================================
Coverage 83.39% 83.39%
========================================
Files 20 20
Lines 783 783
========================================
Hits 653 653
Misses 130 130 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a5c2a2c to
6b97eb7
Compare
6b97eb7 to
e2afb3f
Compare
e2afb3f to
75b9e99
Compare
Refs https://www.drupal.org/project/filefield_paths/issues/3616606
Summary
Test-only. Adds coverage for the
active_updatingskip path, which nothing exercised before, and records that the reported bug does not reproduce.Why there is no fix here
dshields reports that files are renamed on every save even with
active_updatingoff, and attributes it toproperty_exists($entity, 'original')missing a magic property on Drupal 10. That mechanism does not hold on any supported core:originalis a real dynamic property set byEntityStorageBase::doPreSave(), andproperty_exists()returns TRUE for those.originalbecame magic, but the code takes thegetOriginal()branch there, and core clears the original only after invoking the update hook.Even the released
isset($entity->original)form is sound on 11.2+, because core's__isset()special-casesoriginal.Verification
testFileIsNotMovedOnUpdateWhenActiveUpdatingIsOffperforms the reported sequence: save with a file, change the path pattern, save again. The file stays put.testFileIsMovedOnUpdateWhenActiveUpdatingIsOnis the control. Without it, the first test could pass simply because the hook never ran.The issue needs the reporter's core version and entity type before it can go further.
Summary by CodeRabbit
Bug Fixes
Tests