feat(#3608361): pass field delta in token data for [file:delta] support#11
Open
Decipher wants to merge 1 commit into
Open
feat(#3608361): pass field delta in token data for [file:delta] support#11Decipher wants to merge 1 commit into
Decipher wants to merge 1 commit into
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 (21)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughThe legacy file field path processor now iterates field items by delta, adds delta to token data, and skips non-file entities. Test support adds a ChangesFile Delta Token Support
Separate-process Test Attributes
Explicit Guard Conditions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Hook/FileFieldPathsProcessFileLegacy.php (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider checking against
FileInterfaceinstead of the concreteFileclass.The new guard is functionally correct, but binding to the concrete
Drupal\file\Entity\Fileclass instead ofDrupal\file\FileInterfaceis slightly less robust if a site swaps in a custom bundle class for thefileentity type that doesn't extendFile(though it would still implementFileInterface). Since this pattern is already used elsewhere in the file (e.g. the@var \Drupal\file\Entity\File $original_filedocblock), this is a minor stylistic nit rather than a functional issue.♻️ Optional refactor to check against the interface
-use Drupal\file\Entity\File; +use Drupal\file\FileInterface; ... - $file = $field_item->entity; - if (!$file instanceof File) { + $file = $field_item->entity; + if (!$file instanceof FileInterface) { continue; }Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Hook/FileFieldPathsProcessFileLegacy.php` at line 16, The new file-type guard is too tightly coupled to the concrete Drupal\file\Entity\File class; switch the type check in FileFieldPathsProcessFileLegacy to use Drupal\file\FileInterface instead. Update the relevant instanceof guard(s) in this hook so they accept any file entity implementation, while leaving the existing behavior unchanged. Refer to the FileFieldPathsProcessFileLegacy logic and its current File import when making the replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Hook/FileFieldPathsProcessFileLegacy.php`:
- Line 16: The new file-type guard is too tightly coupled to the concrete
Drupal\file\Entity\File class; switch the type check in
FileFieldPathsProcessFileLegacy to use Drupal\file\FileInterface instead. Update
the relevant instanceof guard(s) in this hook so they accept any file entity
implementation, while leaving the existing behavior unchanged. Refer to the
FileFieldPathsProcessFileLegacy logic and its current File import when making
the replacement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a5b77aa-d681-4303-9b33-fa13b34fdc1a
📒 Files selected for processing (3)
src/Hook/FileFieldPathsProcessFileLegacy.phptests/modules/filefield_paths_test/filefield_paths_test.moduletests/src/Functional/FileFieldPathsGeneralTest.php
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.x-1.x #11 +/- ##
===========================================
+ Coverage 79.72% 79.85% +0.12%
===========================================
Files 17 17
Lines 666 670 +4
===========================================
+ Hits 531 535 +4
Misses 135 135 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Hook/FieldConfigEditForm.php (1)
60-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit parentheses for clarity.
The condition relies on
&&binding tighter than||(!class_exists($class) || ($class !== FileFieldItemList::class && !is_subclass_of(...))). Logic checks out against the test case for unsupported classes, but the implicit precedence is easy to misread as a bug at a glance.♻️ Suggested clarification
- if (!class_exists($class) || $class !== FileFieldItemList::class && !is_subclass_of($class, FileFieldItemList::class)) { + if (!class_exists($class) || ($class !== FileFieldItemList::class && !is_subclass_of($class, FileFieldItemList::class))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Hook/FieldConfigEditForm.php` around lines 60 - 63, The conditional in FieldConfigEditForm’s class check is correct but hard to read because it relies on implicit operator precedence. Update the if statement to use explicit parentheses around the FileFieldItemList class comparison and is_subclass_of check so the intended grouping is obvious when reading or reviewing the Hook/FieldConfigEditForm logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Hook/FieldConfigEditForm.php`:
- Around line 60-63: The conditional in FieldConfigEditForm’s class check is
correct but hard to read because it relies on implicit operator precedence.
Update the if statement to use explicit parentheses around the FileFieldItemList
class comparison and is_subclass_of check so the intended grouping is obvious
when reading or reviewing the Hook/FieldConfigEditForm logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6553f0c7-ca16-4440-8d2b-2a32d70f7704
📒 Files selected for processing (3)
src/Hook/FieldConfigEditForm.phpsrc/Hook/FileFieldPathsProcessFileLegacy.phpsrc/Utility/FieldConfigEditFormHandler.php
✅ Files skipped from review due to trivial changes (1)
- src/Utility/FieldConfigEditFormHandler.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Hook/FileFieldPathsProcessFileLegacy.php
Decipher
force-pushed
the
feature/3608361-field_delta
branch
from
July 6, 2026 04:41
9fdaf10 to
f14a625
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
[file:delta]token (added in Field Tokens 2.0.x, #2386655) returns the zero-based position of a file within a multi-value field. However, File (Field) Paths iterates over files using->referencedEntities()which discards position information, so[file:delta]always resolves to empty.Changes
referencedEntities()loop with direct field item iteration to expose the delta index'delta' =>in token datainstanceof Fileguard for safetyRelated
Summary by CodeRabbit
New Features
file:deltatoken support for multi-value file upload path generation, assigning each uploaded file a deterministic zero-based delta suffix.Bug Fixes
Tests
file:deltaon unlimited-cardinality multivalue fields.