Skip to content

fix(#3015137): point the field item at the moved file - #20

Open
Decipher wants to merge 1 commit into
8.x-1.xfrom
feature/3015137-stale-file-object
Open

fix(#3015137): point the field item at the moved file#20
Decipher wants to merge 1 commit into
8.x-1.xfrom
feature/3015137-stale-file-object

Conversation

@Decipher

@Decipher Decipher commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Fixes the 2018 report that File (Field) Paths breaks Focal Point: a focal point set on upload is lost on the first save and correct on the second.

Upstream issue: https://www.drupal.org/i/3015137

Root cause

After moving a file into its token based path, File (Field) Paths left the field item pointing at the pre-move file object. Modules that act later in the same save read the file from the field item, so they read a path that no longer exists.

Focal Point is the visible casualty. It runs right after us on hook_entity_insert(), reads the field item, and saves its crop against the old location. On the second save the file is already in place, so nothing moves and Focal Point reads the right path, which is why the re-save works.

This is not Focal Point specific. Any module reading a file path from the entity in a later hook hits the same thing.

The change

Point the field item at the moved file once the move is finished:

$item = $field->get($delta);
if ($item !== NULL) {
  $item->set('entity', $new_file);
}

It runs after the redirect and directory cleanup, which read the old path on purpose. No dependency on Crop or Focal Point.

An earlier version of this fix only synced the local file variable. That passed a simple save but not the real widget flow, where the upload is staged and reloaded so the field item holds a different File instance than the one being processed. Manual browser testing with Focal Point 2.1.2 and Crop 2.6 caught it. The fix now updates the field item itself, and a kernel test forces that object swap so the gap cannot pass unnoticed again.

Tests

Two kernel tests in FileFieldPathsProcessFileLegacyTest, written before the fix:

  • testMoveUpdatesFileObjectHeldByTheEntity: a later hook reads the moved path.
  • testMoveUpdatesFieldItemWhenReferencedInstanceDiffers: forces the staged-and-reloaded object swap, so a later hook still reads the moved path. Red on the old fix, green on this one.

Local runs on Drupal 11: unit 33/33, kernel 88/88, lint clean with no new PHPStan findings. Also verified end to end in a browser with the Focal Point widget.

What this does not fix

Retroactive update calls filefield_paths_entity_update($entity) directly in Batch\Updater::batchProcess() without re-saving the entity, so focal_point_entity_update() never fires and existing crops are orphaned at their old paths. Only Crop can fix that, by implementing hook_file_move(), which core already invokes from FileRepository::move().

Upstream state

Focal Point https://www.drupal.org/i/3042259 is still Needs Review. MR !61 was opened on 2026-08-01 from tvhung's 2019 patch and has not been touched since, and the newest Focal Point release predates it. The upstream fix has not shipped. The two changes are complementary: theirs reloads the file at the point of use, this one stops handing out a stale reference in the first place.

Credit

  • Nuuou reported it in 2018 with a full reproduction and the correct suspicion that File (Field) Paths was the culprit.
  • tvhung diagnosed the URI change and wrote the original Focal Point patch in 2019.
  • nikitas posted a working custom-module workaround in 2023.
  • justcaldwell confirmed the fix and opened Focal Point MR !61 on 2026-08-01.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 555f6ff3-cfbe-4998-b346-afd3499ab109

📥 Commits

Reviewing files that changed from the base of the PR and between 307022d and 074f811.

📒 Files selected for processing (2)
  • src/Hook/FileFieldPathsProcessFileLegacy.php
  • tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The file move process now tracks each referenced field item, updates the moved file entity, and replaces the field reference when required. Kernel tests cover normal moves, collision-based renaming, and processing with a different File instance.

Changes

File entity synchronization

Layer / File(s) Summary
Update cached file data after moves
src/Hook/FileFieldPathsProcessFileLegacy.php, tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php
The move process captures field deltas, updates the moved file URI and filename, and synchronizes the field-held reference. Kernel tests verify normal moves, collision renaming, persisted values, and distinct file instances.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 074f8

The change synchronizes the in-memory file object after a move so later processing reads the correct URI and filename; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: updating the field item to reference the moved file.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/3015137-stale-file-object

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.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.29%. Comparing base (74954f3) to head (074f811).
⚠️ Report is 2 commits behind head on 8.x-1.x.

Additional details and impacted files
@@             Coverage Diff             @@
##           8.x-1.x      #20      +/-   ##
===========================================
+ Coverage    81.16%   81.29%   +0.12%     
===========================================
  Files           19       19              
  Lines          722      727       +5     
===========================================
+ Hits           586      591       +5     
  Misses         136      136              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Decipher
Decipher force-pushed the feature/3015137-stale-file-object branch from 307022d to 074f811 Compare August 24, 2026 07:15
@Decipher Decipher changed the title fix(#3015137): update the in-memory file object after a move fix(#3015137): point the field item at the moved file Aug 24, 2026
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