Refactored handler pipeline to template-method: single 'expand()', internal 'normalise()' and 'doExpand()'.#372
Merged
Conversation
Contributor
|
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 (4)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Code coverage (threshold: 95%) Per-class coverage |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/AddressHandler.php`:
- Around line 52-60: In expand(), the code uses
reset($this->fieldConfig->getSettings()['available_countries']) which returns
FALSE when available_countries is empty; update expand(array $records): array to
guard that case: fetch $available =
$this->fieldConfig->getSettings()['available_countries'] (or cast to array),
check if non-empty before using reset(), and if empty set a sensible default
(e.g. empty string or null) for $record['country_code'] instead of boolean false
so country_code is always a string/nullable value; modify the logic inside
expand() accordingly to use the guarded value.
In `@src/Drupal/Driver/Core/Field/FileHandler.php`:
- Around line 22-24: The loop in FileHandler.php dereferences
$record['target_id'] without validation which can cause undefined-key notices;
update the foreach that builds $file_path to first check that 'target_id' exists
and is non-empty (e.g. isset and non-empty string after casting/trimming) and if
not throw a clear InvalidArgumentException (or a domain-specific exception)
before calling resolveExistingFile or uploadAndSave; ensure
resolveExistingFile($file_path) and uploadAndSave($file_path) are only invoked
with a validated string to fail fast and avoid downstream file-read errors.
In `@src/Drupal/Driver/Core/Field/ImageHandler.php`:
- Around line 25-30: The validation currently checks for NULL/empty but doesn't
handle a missing 'target_id' key; update the guard in ImageHandler so you first
verify the key exists (e.g. using array_key_exists('target_id', $record) or
!isset($record['target_id'])) and then check for null/empty, and throw the same
InvalidArgumentException referencing $this->mainProperty when the key is absent
or invalid; after that safely cast $record['target_id'] to string and continue
to call $this->resolveExistingFile($file_path) ??
$this->uploadAndSave($file_path).
In `@src/Drupal/Driver/Core/Field/SupportedImageHandler.php`:
- Around line 22-28: The code dereferences $record['target_id'] without
validation and throws a generic exception on read failure; update the block in
SupportedImageHandler (the code using $record['target_id'], $file_path and
file_get_contents) to first validate that $record['target_id'] exists and is
non-empty (throw a clear exception referencing missing target_id), then check
file existence/readability (e.g., is_readable($file_path)) before calling
file_get_contents, and if reading fails include the actual $file_path (and any
underlying error details) in the thrown Exception to improve diagnostics.
🪄 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: b58555e2-7686-4a1c-a1ca-cd537e778fce
📒 Files selected for processing (45)
src/Drupal/Driver/Alias/RolesAlias.phpsrc/Drupal/Driver/Core/Core.phpsrc/Drupal/Driver/Core/Field/AbstractHandler.phpsrc/Drupal/Driver/Core/Field/AddressHandler.phpsrc/Drupal/Driver/Core/Field/BooleanHandler.phpsrc/Drupal/Driver/Core/Field/DaterangeHandler.phpsrc/Drupal/Driver/Core/Field/DatetimeHandler.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/EmbridgeAssetItemHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceRevisionsHandler.phpsrc/Drupal/Driver/Core/Field/FieldHandlerInterface.phpsrc/Drupal/Driver/Core/Field/FileHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phpsrc/Drupal/Driver/Core/Field/LinkHandler.phpsrc/Drupal/Driver/Core/Field/ListHandlerBase.phpsrc/Drupal/Driver/Core/Field/NameHandler.phpsrc/Drupal/Driver/Core/Field/OgStandardReferenceHandler.phpsrc/Drupal/Driver/Core/Field/SmartdateHandler.phpsrc/Drupal/Driver/Core/Field/SupportedImageHandler.phpsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phpsrc/Drupal/Driver/Core/Field/TimeHandler.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AddressHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/BooleanHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DaterangeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldHandlerUnitTestBase.phptests/Drupal/Tests/Driver/Unit/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/LinkHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/NameHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/SmartdateHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/SupportedImageHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextWithSummaryHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TimeHandlerTest.php
AlexSkrypnyk
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/Drupal/Driver/Core/Field/FileHandler.php (1)
18-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead the main property with
??before validating it.If a keyed record omits
$this->mainProperty, line 19 raises an undefined-array-key notice before yourInvalidArgumentExceptioncan fire. Malformed file records should fail cleanly here.Suggested fix
foreach ($records as &$record) { - if ($record[$this->mainProperty] === NULL || $record[$this->mainProperty] === '') { + $target = $record[$this->mainProperty] ?? NULL; + if ($target === NULL || $target === '') { throw new \InvalidArgumentException(sprintf('%s field "%s" must not be NULL or empty.', $this->getFieldLabel(), $this->mainProperty)); } - $record[$this->mainProperty] = (string) $record[$this->mainProperty]; + $record[$this->mainProperty] = (string) $target; }🤖 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/FileHandler.php` around lines 18 - 24, The loop over $records reads $record[$this->mainProperty] directly which can trigger an undefined-array-key notice; first read the value with the null-coalescing operator (e.g. $value = $record[$this->mainProperty] ?? null) and validate $value for NULL or empty before throwing the \InvalidArgumentException (use $this->getFieldLabel() and $this->mainProperty in the message as before); then set $record[$this->mainProperty] = (string)$value so missing keys are handled cleanly and the exception only fires for malformed file records.src/Drupal/Driver/Core/Field/SupportedImageHandler.php (1)
17-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep
normalise()public and restore the guarded main-property read.Core now calls
handler->normalise()directly, so narrowing this override toprotectedbreaks the new contract. The direct$record[$this->mainProperty]access also reintroduces the undefined-key path instead of consistently throwing the intendedInvalidArgumentExceptionfor malformed records.Proposed fix
- protected function normalise(mixed $values): array { + public function normalise(mixed $values): array { $records = parent::normalise($values); foreach ($records as &$record) { - if ($record[$this->mainProperty] === NULL || $record[$this->mainProperty] === '') { + $value = $record[$this->mainProperty] ?? NULL; + if ($value === NULL || $value === '') { throw new \InvalidArgumentException(sprintf('Supported image field "%s" must not be NULL or empty.', $this->mainProperty)); } - $record[$this->mainProperty] = (string) $record[$this->mainProperty]; + $record[$this->mainProperty] = (string) $value; }#!/bin/bash # Verify the normalise() visibility contract across the handler hierarchy. rg -nP '\b(public|protected)\s+function\s+normalise\s*\(' \ src/Drupal/Driver/Core/Field/AbstractHandler.php \ src/Drupal/Driver/Core/Field/SupportedImageHandler.php # Show the interface declaration that Core is expected to call. rg -nP '\bfunction\s+normalise\s*\(' \ src/Drupal/Driver/Core/Field/FieldHandlerInterface.php🤖 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/SupportedImageHandler.php` around lines 17 - 25, The override of normalise() in SupportedImageHandler is incorrectly declared protected and directly indexes $record[$this->mainProperty], risking undefined-key access; change the method signature back to public to match Core's contract (public function normalise(mixed $values): array) and, inside the loop after calling parent::normalise($values), replace direct indexing with a guarded read that checks array_key_exists($this->mainProperty, $record) and that the value is not null/empty, throwing the existing InvalidArgumentException when the key is missing or the value is NULL/empty, then cast the value to string and assign back to $record[$this->mainProperty].
🤖 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/AddressHandler.php`:
- Around line 30-40: The current branch naively expands any top-level list into
multiple records, which splits positional addresses like ['John','Smith','123
Main St'] into three deltas; change the logic in AddressHandler so that when
array_is_list($values) you first detect whether the list is actually a single
positional address (e.g., elements are scalars or each element is itself a
positional list) vs a list of associative record arrays, and only iterate into
multiple records when the elements are associative record arrays; otherwise call
$this->normaliseDelta($values, $visible_fields) once and return that single
record. Use the existing normaliseDelta method and the $visible_fields variable
to decide (for example, check is_array($el) && !array_is_list($el) or presence
of string keys on elements) to distinguish positional single-address lists from
lists of records.
In `@src/Drupal/Driver/Core/Field/DaterangeHandler.php`:
- Around line 22-40: The code in DaterangeHandler that normalises $values treats
any PHP list as multiple records and thus rejects the shorthand single-record
positional form [start, end]; modify the normalisation logic around $values so
that if $values is an array_is_list and its elements are scalars (or not arrays)
— e.g. count($values) === 2 and !is_array($values[0]) — you wrap it as a single
record (i.e. $values = [$values]) before iterating; keep the existing behaviour
for a list of record arrays and for non-list inputs so the foreach building
$records with keys 'value' and 'end_value' still works.
In `@src/Drupal/Driver/Core/Field/EntityReferenceHandler.php`:
- Around line 18-23: The loop in EntityReferenceHandler that coerces digit-only
string labels into integers (the block modifying $record[$this->mainProperty]
when is_string($lookup) && ctype_digit($lookup)) causes numeric-looking labels
like "123" to be treated as pre-resolved IDs and thus skips the lookup and
bundle/not-found checks; remove this coercion (or preserve the original string
type) so that lookup logic still runs for digit-only labels and ensure the same
change is applied to the similar block around the other occurrence (lines that
check ctype_digit) to avoid bypassing label resolution and validation.
In `@src/Drupal/Driver/Core/Field/EntityReferenceRevisionsHandler.php`:
- Around line 20-25: The integer fast-path in the foreach (casting
$record[$this->mainProperty] to int when is_string && ctype_digit) can create
dangling revision refs; instead of coercing early, remove or guard the cast so
numeric-looking labels still go through the normal validation/load path: defer
any integer conversion for $this->mainProperty until after you validate the
entity/bundle and successfully load the target revision (the code paths around
the query/bundle validation and revision load that write target_revision_id must
be reached); if the revision/entity cannot be loaded, do not write a numeric
target_revision_id (leave original value or set to null) and ensure the
subsequent logic that sets target_revision_id only runs on successful loads
(affecting the code that references $records, $this->mainProperty,
target_revision_id and the revision load/validation logic).
In `@src/Drupal/Driver/Core/Field/FieldHandlerInterface.php`:
- Around line 7-9: Update the FieldHandlerInterface class-level docblock to
describe the two-stage expansion pipeline: explain that implementations must
first accept raw input via normalise(mixed $values) to convert and validate
values into a canonical intermediate form, and then perform batch expansion via
doExpand(array $records) which takes the normalized records and returns
expanded/denormalized results; mention the contract guarantees (e.g., normalise
is called per-value before doExpand, doExpand may operate on arrays/batches and
should be idempotent) and reference the method names normalise and doExpand so
implementers can locate the expected flow.
In `@src/Drupal/Driver/Core/Field/LinkHandler.php`:
- Around line 45-79: The handler misinterprets a top-level positional tuple like
['Title','entity:node/1','foo=bar'] as multiple URI-only records; add a
disambiguation before the current normalization so that when $values is a list
(array_is_list($values)), has more than one item, and its elements are all
scalars (none are arrays), treat it as a single positional record by wrapping
it: $values = [$values]; update LinkHandler::the processing block (the code
around the array_is_list check and foreach over $values) and add tests covering
scalar-only list tuples vs multi-value scalar URI lists.
In `@src/Drupal/Driver/Core/Field/TimeHandler.php`:
- Around line 22-27: The code in TimeHandler.php currently pushes
strtotime((string)$value) - $midnight into $seconds even when strtotime returns
FALSE, producing large negative values; update the handling around the strtotime
call in the method (look for the $seconds array, $value and $midnight usages) to
first call strtotime((string)$value), check for strict !== FALSE, and only
subtract $midnight and push to $seconds when valid; for invalid/false results
reject or surface the error (throw/return/validation failure) instead of writing
the erroneous computed value.
---
Duplicate comments:
In `@src/Drupal/Driver/Core/Field/FileHandler.php`:
- Around line 18-24: The loop over $records reads $record[$this->mainProperty]
directly which can trigger an undefined-array-key notice; first read the value
with the null-coalescing operator (e.g. $value = $record[$this->mainProperty] ??
null) and validate $value for NULL or empty before throwing the
\InvalidArgumentException (use $this->getFieldLabel() and $this->mainProperty in
the message as before); then set $record[$this->mainProperty] = (string)$value
so missing keys are handled cleanly and the exception only fires for malformed
file records.
In `@src/Drupal/Driver/Core/Field/SupportedImageHandler.php`:
- Around line 17-25: The override of normalise() in SupportedImageHandler is
incorrectly declared protected and directly indexes
$record[$this->mainProperty], risking undefined-key access; change the method
signature back to public to match Core's contract (public function
normalise(mixed $values): array) and, inside the loop after calling
parent::normalise($values), replace direct indexing with a guarded read that
checks array_key_exists($this->mainProperty, $record) and that the value is not
null/empty, throwing the existing InvalidArgumentException when the key is
missing or the value is NULL/empty, then cast the value to string and assign
back to $record[$this->mainProperty].
🪄 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: 4587f5ab-6033-4991-a9b9-e39777c450f6
📒 Files selected for processing (34)
src/Drupal/Driver/Core/Field/AbstractHandler.phpsrc/Drupal/Driver/Core/Field/AddressHandler.phpsrc/Drupal/Driver/Core/Field/BooleanHandler.phpsrc/Drupal/Driver/Core/Field/DaterangeHandler.phpsrc/Drupal/Driver/Core/Field/DatetimeHandler.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceRevisionsHandler.phpsrc/Drupal/Driver/Core/Field/FieldHandlerInterface.phpsrc/Drupal/Driver/Core/Field/FileHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phpsrc/Drupal/Driver/Core/Field/LinkHandler.phpsrc/Drupal/Driver/Core/Field/ListHandlerBase.phpsrc/Drupal/Driver/Core/Field/NameHandler.phpsrc/Drupal/Driver/Core/Field/SmartdateHandler.phpsrc/Drupal/Driver/Core/Field/SupportedImageHandler.phpsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phpsrc/Drupal/Driver/Core/Field/TimeHandler.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldHandlerRegistryKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/CoreFieldHandlerLookupTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AddressHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldHandlerUnitTestBase.phptests/Drupal/Tests/Driver/Unit/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/NameHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/SupportedImageHandlerTest.phptests/fixtures/ConsumerProject/Driver/Field/StringLongHandler.phptests/fixtures/ConsumerProject/Driver/Field/TextLongHandler.php
✅ Files skipped from review due to trivial changes (1)
- src/Drupal/Driver/Core/Field/AbstractHandler.php
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Drupal/Driver/Core/Field/DefaultHandler.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/FieldHandlerUnitTestBase.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/AddressHandlerTest.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/NameHandlerTest.php
AlexSkrypnyk
changed the title
…ternal 'normalise()' and 'doExpand()'.
AlexSkrypnyk
changed the title
AlexSkrypnyk
force-pushed
the
feature/normalise-public
branch
from
b3e410f to
85989ca
Compare
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
…ive guards on reference handlers, NULL load throw.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/Driver/Core/Field/NameHandler.php (1)
58-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unsupported list item types instead of silently dropping them.
At Line 58-67, non-string/non-array items are ignored. That can silently lose records and mask invalid input; throw an exception for unsupported element types.
Suggested fix
foreach ($values as $value) { if (is_string($value)) { $names[] = $this->normaliseString($value, $enabled); continue; } if (is_array($value)) { $names[] = $this->normaliseArray($value, $enabled); + continue; } + + throw new \InvalidArgumentException(sprintf( + 'Name field record must be a string or array; got %s.', + get_debug_type($value), + )); }🤖 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/NameHandler.php` around lines 58 - 67, The loop in NameHandler that iterates over $values currently drops items that are neither strings nor arrays, which can silently lose data; update the foreach block (the loop that calls normaliseString() and normaliseArray()) to throw an InvalidArgumentException for any unsupported item type instead of ignoring it, including in the exception message the offending item's type (and optionally its index or key) and the field/context so callers can diagnose invalid input; keep calls to normaliseString() and normaliseArray() as-is for string/array items.
🤖 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/Alias/AuthorAlias.php`:
- Around line 85-87: Validate the resolved user id returned by $user->id()
before coercing to int: ensure it's numeric and greater than zero (e.g.,
is_numeric and > 0) and only then call $stub->setValue('uid', (int)
$user->id()); otherwise avoid assigning the uid (or raise/return an explicit
error) so you don't silently convert invalid values to 0; update the logic in
AuthorAlias.php around the $stub->setValue('uid', ...) call to perform this
guard.
In `@src/Drupal/Driver/Core/Alias/ParentTermAlias.php`:
- Around line 117-119: Guard against invalid tid before casting in
ParentTermAlias: verify the $tid returned by your lookup is a numeric positive
ID (e.g., is_numeric and > 0) before calling $stub->setValue('parent', (int)
$tid); if the check fails, do not set the parent value and instead handle the
error (throw/return/skip) so you don't silently convert unexpected strings to
0—update the logic around the lookup and the $stub->setValue('parent', (int)
$tid) call to perform this validation.
In `@src/Drupal/Driver/Core/Field/AddressHandler.php`:
- Around line 41-43: The foreach over $values in AddressHandler is passing
non-array/non-string items into normaliseDelta which can cause malformed
records; update the loop that builds $records to validate each $value is an
array or string before calling AddressHandler::normaliseDelta (use
is_array/is_string), and if not, throw an InvalidArgumentException (include the
offending value/index and a clear message referencing the field being processed)
so callers get a clear error instead of silently producing bad records.
In `@tests/Drupal/Tests/Driver/Unit/Core/Field/FieldHandlerUnitTestBase.php`:
- Around line 59-63: The test currently always suppresses PHP warnings by
calling @$handler->expand($input), which can hide issues on non-exception rows;
update the test in FieldHandlerUnitTestBase so that you only prepend the
error-suppression operator when the current test row expects an exception (e.g.,
check the row's metadata / a flag like 'exception' or similar used by the data
provider), otherwise call $handler->expand($input) without '@', and keep
asserting the exception behavior unchanged for exception rows.
---
Outside diff comments:
In `@src/Drupal/Driver/Core/Field/NameHandler.php`:
- Around line 58-67: The loop in NameHandler that iterates over $values
currently drops items that are neither strings nor arrays, which can silently
lose data; update the foreach block (the loop that calls normaliseString() and
normaliseArray()) to throw an InvalidArgumentException for any unsupported item
type instead of ignoring it, including in the exception message the offending
item's type (and optionally its index or key) and the field/context so callers
can diagnose invalid input; keep calls to normaliseString() and normaliseArray()
as-is for string/array items.
🪄 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: 03be3628-6512-49d8-8c87-2e080af638a5
⛔ Files ignored due to path filters (1)
tests/fixtures/files/fixture.binis excluded by!**/*.bin
📒 Files selected for processing (54)
src/Drupal/Driver/Alias/RolesAlias.phpsrc/Drupal/Driver/Core/Alias/AuthorAlias.phpsrc/Drupal/Driver/Core/Alias/ParentTermAlias.phpsrc/Drupal/Driver/Core/Field/AbstractHandler.phpsrc/Drupal/Driver/Core/Field/AddressHandler.phpsrc/Drupal/Driver/Core/Field/BooleanHandler.phpsrc/Drupal/Driver/Core/Field/DaterangeHandler.phpsrc/Drupal/Driver/Core/Field/DatetimeHandler.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/EmbridgeAssetItemHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceRevisionsHandler.phpsrc/Drupal/Driver/Core/Field/FieldHandlerInterface.phpsrc/Drupal/Driver/Core/Field/FileHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phpsrc/Drupal/Driver/Core/Field/LinkHandler.phpsrc/Drupal/Driver/Core/Field/ListFloatHandler.phpsrc/Drupal/Driver/Core/Field/ListHandlerBase.phpsrc/Drupal/Driver/Core/Field/NameHandler.phpsrc/Drupal/Driver/Core/Field/OgStandardReferenceHandler.phpsrc/Drupal/Driver/Core/Field/SmartdateHandler.phpsrc/Drupal/Driver/Core/Field/SupportedImageHandler.phpsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phpsrc/Drupal/Driver/Core/Field/TimeHandler.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldHandlerRegistryKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/CoreFieldHandlerLookupTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/AddressHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/BooleanHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DaterangeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldHandlerUnitTestBase.phptests/Drupal/Tests/Driver/Unit/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/LinkHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListFloatHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListIntegerHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ListStringHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/NameHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/SmartdateHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/SupportedImageHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TextWithSummaryHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/TimeHandlerTest.phptests/fixtures/ConsumerProject/Driver/Field/StringLongHandler.phptests/fixtures/ConsumerProject/Driver/Field/TextLongHandler.php
💤 Files with no reviewable changes (1)
- tests/Drupal/Tests/Driver/Unit/Core/Field/ListHandlerTest.php
✅ Files skipped from review due to trivial changes (4)
- src/Drupal/Driver/Core/Field/EmbridgeAssetItemHandler.php
- src/Drupal/Driver/Core/Field/OgStandardReferenceHandler.php
- src/Drupal/Driver/Core/Field/FieldHandlerInterface.php
- src/Drupal/Driver/Core/Field/AbstractHandler.php
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php
- src/Drupal/Driver/Core/Field/DefaultHandler.php
- src/Drupal/Driver/Alias/RolesAlias.php
- src/Drupal/Driver/Core/Field/TextLongHandler.php
- src/Drupal/Driver/Core/Field/TextWithSummaryHandler.php
- tests/Drupal/Tests/Driver/Kernel/Core/Field/FieldHandlerRegistryKernelTest.php
- src/Drupal/Driver/Core/Field/DatetimeHandler.php
- src/Drupal/Driver/Core/Field/ListHandlerBase.php
- tests/fixtures/ConsumerProject/Driver/Field/TextLongHandler.php
- src/Drupal/Driver/Core/Field/TimeHandler.php
- src/Drupal/Driver/Core/Field/TextHandler.php
- src/Drupal/Driver/Core/Field/SupportedImageHandler.php
- src/Drupal/Driver/Core/Field/LinkHandler.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/AbstractHandlerNormaliseTest.php
- src/Drupal/Driver/Core/Field/SmartdateHandler.php
- src/Drupal/Driver/Core/Field/BooleanHandler.php
- src/Drupal/Driver/Core/Field/FileHandler.php
… guard, scoped test warning suppression.
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.
Summary
Refactors the field handler pipeline to a template-method shape and collapses every per-handler unit test to a single
testExpand()against the public method.FieldHandlerInterfacekeeps one public method:expand(mixed $values): array.AbstractHandler::expand()is nowfinaland runs$this->doExpand($this->normalise($values)).normalise()isprotected(default lives onAbstractHandler; subclasses override only for shorthand parsing).doExpand()isabstract protectedand only ever sees canonical records. The discipline is enforced by the base class rather than by callers remembering to callnormalise()first - there is no public path that bypasses it.Changes
Interface and base class
FieldHandlerInterfacedeclares onlyexpand(mixed $values): array.AbstractHandler::expand()isfinal; body is one line.AbstractHandler::normalise()isprotected. Default shape: folds scalars into[mainProperty => value]records, rejects mixed positional/named keys at the top level, rejects records missing the main property.AbstractHandler::doExpand()isabstract protected.Core::expandEntityFields()One call per field:
$stub->setValue($field, $handler->expand($value));.Per-handler updates
Every concrete handler renames the previous
public function expand(array $records)toprotected function doExpand(array $records). Validation and casting move intonormalise():FileHandler/ImageHandler/SupportedImageHandler: NULL/emptytarget_idrejection and(string)cast live innormalise().doExpand()only does file resolution / upload.EntityReferenceHandler/EntityReferenceRevisionsHandler:doExpand()skips the entity-storage round-trip when the lookup is already anint. Numeric-string lookups go through the OR-condition (id_key = X OR label_key = 'X') so digit-only labels still resolve.LinkHandler/NameHandler/SmartdateHandlerkeep theirnormalise()overrides for shorthand parsing.AddressHandler/DaterangeHandlerdistinguish a single positional record from a list of records via anarray_is_list && is_array($values[0])check, so['John', 'Doe']and['2026-07-15T09:00:00', '2026-07-15T17:00:00']resolve as one record rather than multi-delta.TimeHandlerthrowsInvalidArgumentExceptionwhenstrtotime()returnsFALSE.Reference handlers consistently use
$this->mainPropertyinstead of the literal'target_id'.Aliases
AuthorAliascasts the resolved uid tointso the int-only fast path inEntityReferenceHandler::doExpand()applies.ParentTermAliascasts the resolved tid tointfor the same reason.RolesAliasunwrapstarget_idfrom the records thatEntityReferenceHandlernow produces, so the post-create role assignment still works afterrolesflows through the pipeline.Tests
FieldHandlerUnitTestBasedeclares a singletestExpand()driven by an abstractdataProviderExpand(). Every concrete handler test extends it and supplies onlycreateHandler()and the data rows.ListHandlerTestsplit intoListStringHandlerTest,ListIntegerHandlerTest,ListFloatHandlerTest- one allowed-values configuration per file.FileHandlerTest,ImageHandlerTest,SupportedImageHandlerTestuse onesetUp()that primes a universal Drupal container (URI-to-File-id index for the reuse branch, fresh File fromfile.repositoryfor the upload branch). Unit-test rows exercise the upload, reuse, and rejection paths against a checked-in fixture attests/fixtures/files/fixture.bin.EntityReferenceHandlerTest,EntityReferenceRevisionsHandlerTestuse onesetUp()priming anentity_type.managerwhose query stub matches a label-to-id index. Data rows cover label match, int passthrough, label miss.[input, expected, exception_class, exception_message]shape.tests/fixtures/files/sample.txt(FileHandler) andtests/fixtures/files/sample.jpg(ImageHandler, SupportedImageHandler).AbstractHandlerNormaliseTestretained - it exercises the basenormalise()against a genericNormaliseTestHandlervia reflection (the protected method is not on the public contract).Comments
Class docblocks reduced to a single descriptive sentence. Method docblocks no longer describe how other methods relate to the one being documented.
Before / After
Public surface
Test layout
Summary by CodeRabbit