Fixed 'ImageHandler' and 'DatetimeHandler' compound parser shape.#370
Conversation
…ow-function return types.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdd AbstractHandler::normalise() and update handlers (Datetime, Image, Text variants) to consume canonical record-list shapes; expand() implementations now return newly built record arrays. Unit and kernel tests converted to data-provider-driven forms and adjusted to assert the compound-parser record-list shape. ChangesField handler normalization and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.php (1)
77-83: ⚡ Quick winAdd one assertion for the numeric-index compound fallback path.
This test validates
['value' => ...], butexpand()also supports[0 => ...]. Adding that case here will lock in both accepted parser shapes.🤖 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 `@tests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.php` around lines 77 - 83, Update the testExpandHandlesCompoundParserShapeEmpty test to also assert the numeric-index compound fallback path: call $this->createHandler('datetime')->expand with elements that use key 0 (e.g. [[0 => ''], [0 => NULL]]) and assert the result is [NULL, NULL]; this ensures both supported parser shapes ('value' and numeric index 0) are covered for the expand() method.
🤖 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.
Inline comments:
In `@src/Drupal/Driver/Core/Field/DatetimeHandler.php`:
- Around line 27-33: The loop in DatetimeHandler that builds $formatted
reindexes the array and drops original delta keys; change the foreach over
$values to preserve keys (e.g. foreach ($values as $delta => $value)) and assign
formatted results back using that same key (set $formatted[$delta] =
$this->formatDateValue(...)) so DatetimeHandler and formatDateValue keep
incoming delta-keyed compound shapes intact.
---
Nitpick comments:
In `@tests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.php`:
- Around line 77-83: Update the testExpandHandlesCompoundParserShapeEmpty test
to also assert the numeric-index compound fallback path: call
$this->createHandler('datetime')->expand with elements that use key 0 (e.g. [[0
=> ''], [0 => NULL]]) and assert the result is [NULL, NULL]; this ensures both
supported parser shapes ('value' and numeric index 0) are covered for the
expand() method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2807be7f-da88-4c2f-8b06-ac40c5584cbb
📒 Files selected for processing (7)
src/Drupal/Driver/Core/Field/DatetimeHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phptests/Drupal/Tests/Driver/Kernel/Core/Field/DatetimeHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/ImageHandlerKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.php
| foreach ($values as $value) { | ||
| // EntityFieldParser emits compound-mode deltas as '['value' => '...']' | ||
| // arrays. Sibling DaterangeHandler accepts the same shape; mirror it | ||
| // here so 'datetime' fields work uniformly with the parser. | ||
| $date = is_array($value) ? ($value['value'] ?? $value[0] ?? NULL) : $value; | ||
| $formatted[] = $this->formatDateValue($date, $site_timezone, $storage_timezone); | ||
| } |
There was a problem hiding this comment.
Preserve incoming delta keys when formatting records.
$formatted[] reindexes results and discards original delta keys. Since this handler now accepts delta-keyed compound shapes, keep keys stable during transformation.
Suggested fix
- foreach ($values as $value) {
+ foreach ($values as $delta => $value) {
// EntityFieldParser emits compound-mode deltas as '['value' => '...']'
// arrays. Sibling DaterangeHandler accepts the same shape; mirror it
// here so 'datetime' fields work uniformly with the parser.
$date = is_array($value) ? ($value['value'] ?? $value[0] ?? NULL) : $value;
- $formatted[] = $this->formatDateValue($date, $site_timezone, $storage_timezone);
+ $formatted[$delta] = $this->formatDateValue($date, $site_timezone, $storage_timezone);
}🤖 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/Drupal/Driver/Core/Field/DatetimeHandler.php` around lines 27 - 33, The
loop in DatetimeHandler that builds $formatted reindexes the array and drops
original delta keys; change the foreach over $values to preserve keys (e.g.
foreach ($values as $delta => $value)) and assign formatted results back using
that same key (set $formatted[$delta] = $this->formatDateValue(...)) so
DatetimeHandler and formatDateValue keep incoming delta-keyed compound shapes
intact.
There was a problem hiding this comment.
Declining this one. Every sibling handler in src/Drupal/Driver/Core/Field/ reindexes via $arr[]:
FileHandler::expand()→$files[] = [...]ImageHandler::expand()→$expanded[] = [...]DaterangeHandler::expand()→$result[] = [...]LinkHandler::expand()→$links[] = $linkNameHandler::expand()→$names[] = ...AddressHandler::expand()→$result[] = ...
DatetimeHandler now matches that pattern (the whole point of this change was to mirror DaterangeHandler). Preserving incoming delta keys here would make DatetimeHandler the odd one out without buying anything observable: EntityFieldParser only emits numerically-keyed records (0, 1, 2…) and Drupal's entity field storage re-indexes on save, so the keys are never carried through to the stored entity.
If we ever decide that handlers should preserve delta keys, that's a project-wide change across all eight handlers, not a one-off here.
There was a problem hiding this comment.
@AlexSkrypnyk, that's a well-reasoned argument — the consistency with all eight sibling handlers and the fact that Drupal re-indexes on save makes preserving delta keys here genuinely redundant. Acknowledged, withdrawing the suggestion.
(╯°□°）╯ 🐇 (graceful retreat)
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: jhedstrom/DrupalDriver PR: 370
File: src/Drupal/Driver/Core/Field/DatetimeHandler.php:27-33
Timestamp: 2026-05-18T10:25:22.749Z
Learning: In `src/Drupal/Driver/Core/Field/` (DrupalDriver project, PHP), all field handler `expand()` methods consistently reindex their output arrays using `$arr[] = ...` (append syntax). This is intentional and project-wide: EntityFieldParser only emits numerically-keyed (0, 1, 2…) records, and Drupal's entity field storage re-indexes on save. Suggestions to preserve incoming delta keys in any individual handler should be declined as the pattern is uniform across all handlers (FileHandler, ImageHandler, DaterangeHandler, DatetimeHandler, LinkHandler, NameHandler, AddressHandler, etc.).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Code coverage (threshold: 95%) Per-class coverage |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/Drupal/Driver/Core/Field/ImageHandler.php`:
- Around line 29-31: Check for and validate the required target_id before
calling resolveExistingFile/uploadAndSave in the loop that processes $records
(the foreach in expand()); specifically ensure $record has a non-empty
'target_id' (e.g., isset/array_key_exists and not empty) and if missing or
invalid throw a clear exception (e.g., InvalidArgumentException) or return a
meaningful error rather than passing an invalid path into resolveExistingFile()
or uploadAndSave(); update the logic around resolveExistingFile($file_path) and
uploadAndSave($file_path) to only run when $file_path is present and valid and
reference the expand(), normalise(), resolveExistingFile, and uploadAndSave
symbols when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8774b9be-649c-4cb2-95b2-de3bd60ed200
📒 Files selected for processing (17)
.github/workflows/ci.ymlsrc/Drupal/Driver/Core/Field/AbstractHandler.phpsrc/Drupal/Driver/Core/Field/DatetimeHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phpsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phptests/Drupal/Tests/Driver/Kernel/Core/Field/DatetimeHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/ImageHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/ImageHandlerReuseKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextWithSummaryHandlerTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Drupal/Driver/Core/Field/DatetimeHandler.php
1 participant
Summary
Field handlers gain a shared
normalise()helper onAbstractHandlerthat converts any loose input shape a caller naturally produces — a bare scalar, a list of scalars, a single keyed record, a list of records, or a mixed list of scalars and records — into a canonical list of records. Callers never need to know whether a field is single- or multi-column, never need to type column names liketarget_idorvaluefor the simple case, and the handler always returns a uniform list of records that Drupal entity storage accepts.ImageHandler::expand()andDatetimeHandler::expand()now usenormalise()instead of in-line shape sniffing; the three text handlers (TextHandler,TextLongHandler,TextWithSummaryHandler) were migrated fromimplements FieldHandlerInterfacetoextends AbstractHandlerso they get the same helper.The original bug —
ImageHandler::expand()reading$values[0]as a string when the parser handed it[['target_id' => 'foo.jpg', ...]]and triggeringTypeError— is fixed as a consequence. The branch also fixes theDrupalExtension smokeworkflow that kept breaking wheneverdrupalextensionbumped itsminimum-stabilityfloor.Consumers (drupal-extension's
EntityFieldParser,DrushDriver, etc.) can now pass whatever they have without inspecting the field's column structure first. That work happens in the consumer packages.Changes
src/Drupal/Driver/Core/Field/AbstractHandler.php— addedprotected function normalise(mixed $values): array. Discovers the main property via$this->fieldInfo->getMainPropertyName(). Accepts five shapes (bare scalar, list of scalars, single record, list of records, mixed list); throwsInvalidArgumentExceptionfor the ambiguous "positional plus named keys at the top level" shape (e.g.['/path', 'alt' => 'A']) rather than silently picking an interpretation. Subclasses overridenormalise()only when they have custom shorthand (e.g. a futureNameHandler"Family, Given" string).src/Drupal/Driver/Core/Field/ImageHandler.php—expand()now calls$this->normalise($values)and iterates the canonical records. Always returns a list of records (no flat-single-record return, no shape sniffing).src/Drupal/Driver/Core/Field/DatetimeHandler.php—expand()now calls$this->normalise($values)and iterates the canonical records. Returns a list of['value' => …]records with each value formatted for storage.src/Drupal/Driver/Core/Field/TextHandler.php,TextLongHandler.php,TextWithSummaryHandler.php— migrated fromimplements FieldHandlerInterfacetoextends AbstractHandler.expand()delegates tonormalise(). Production wiring is unchanged (Core::getFieldHandler()already passes the stub/entity_type/field_name constructor args); test setup updated to follow thenewInstanceWithoutConstructor()+ reflection-injectedfieldInfopattern.tests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.php— new. Single test methodtestNormalisedriven by a 16-row data provider. 12 happy-path rows cover every accepted shape against multiple main-property names (target_id,value,uri); 4 throw rows cover the ambiguous-key cases (numeric-0 + named extras, gappy numeric + named, ordering variants). Each row carries(input, main_property, expected_output, expected_exception_class, expected_exception_message_fragment)— the test branches on whether an exception class was supplied. A test-local final subclassNormaliseTestHandlerexists in the same file solely to provide the abstract surface so reflection can invoke the inheritednormalise().tests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.php— refactored. Two data-provider tests (testExpandUploadsFile,testExpandReusesManagedFile) now cover every loose shape a caller can naturally pass: bare scalar path, list of paths, single record, list of records, multi-record, plus the same shape variations for the reuse-managed-file path.createHandler()injects afieldInfostub returning'target_id'.tests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.php—testExpandPreservesEmptyValuesAsNullis now data-driven (5 shape variants);createHandler()stubs bothgetSetting('datetime_type')andgetMainPropertyName().tests/Drupal/Tests/Driver/Unit/Core/Field/TextHandlerTest.php,TextLongHandlerTest.php,TextWithSummaryHandlerTest.php— updated to construct the handler vianewInstanceWithoutConstructor()+ reflection-injectedfieldInfo, matching the pattern used by other handler tests.Kernel tests (
ImageHandlerKernelTest.php,ImageHandlerReuseKernelTest.php,DatetimeHandlerKernelTest.php) — inputs use the canonical record shape, which the newnormalise()returns unchanged..github/workflows/ci.yml—DrupalExtension smokejob: the path repo fordrupal/drupal-driveris now declared at version3.0.99(stable) instead of3.0.99-alpha. The previous alpha-suffixed declaration broke every timedrupalextensionbumped itsminimum-stabilityfloor (alpha → beta → RC). Declaring stable satisfies any floor up to and including stable, decoupling this workflow from the consumer's stability progression.Before / After
Summary by CodeRabbit
Release Notes
Refactor
Tests
Chores