Added a dedicated field value-shape classifier gating the default handler, and deprecated pass-through handlers.#386
Merged
Conversation
…ions. 'DefaultHandler' now inspects a field's stored (non-computed) property definitions and relays the normalised records verbatim only when every stored property is a plain scalar the caller authors as-is. A property that needs a translation the default cannot perform - an entity-reference target, a 'datetime_iso8601'/'duration_iso8601' string, or a complex/nested value - makes the field ineligible, and expand() throws a message naming the field and the offending property. Detection is proactive rather than try/catch because those translations fail silently (a label persisted as a bogus id, a timezone-shifted date). Removed the pure pass-through handlers 'TextHandler', 'TextLongHandler', 'TextWithSummaryHandler', and 'ColorFieldTypeHandler': their field types are plain scalars the default now absorbs, as is any future contrib field of the same shape. 'date_recur' keeps its handler because its 'value'/'end_value' columns are 'datetime_iso8601' (inherited from 'daterange'), structurally identical to fields that need timezone conversion, so the default rejects it and the verbatim pass-through must be declared by a handler. 'FieldTypeCoverageKernelTest' classifies via the shared 'DefaultHandler::unsupportedPropertyReason()', and the text/color kernel round-trips are consolidated into 'DefaultHandlerKernelTest'.
'expectExceptionMessage()' keeps only its last call, so the earlier wrapper-text assertion was silently dropped. Combining both fragments into a single 'expectExceptionMessageMatches()' verifies the message as a whole.
Contributor
📝 WalkthroughWalkthroughThis PR introduces ChangesDefaultHandler classification and enforcement
Estimated code review effort: 4 (Complex) | ~60 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 |
|
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.
🧹 Nitpick comments (2)
tests/fixtures/ConsumerProject/Driver/Field/DatetimeHandler.php (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a less loaded timestamp for the marker constant.
The
MARKERvalue2001-09-11T08:46:00matches the well-known time of the September 11 attacks. Functionally it works fine as a distinct sentinel, but picking an arbitrary/less symbolically-charged timestamp (e.g.1999-12-31T23:59:59) avoids any unintended association in test fixture data.🤖 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/fixtures/ConsumerProject/Driver/Field/DatetimeHandler.php` around lines 20 - 29, The DatetimeHandler MARKER constant uses a timestamp with an unintended real-world association; replace the current sentinel in DatetimeHandler::MARKER with a neutral ISO 8601 value that is still clearly distinct from the input date, and keep the rest of the handler behavior unchanged so the fixture continues proving the custom handler ran.src/Drupal/Driver/Core/Field/README.md (1)
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpelling variant inconsistency: "normalize" vs "normalise".
Line 40 in this same doc uses "normalised"; line 56 switches to "normalize".
✏️ Proposed fix
-Internally, they normalize a scalar to `[$scalar]` before returning the storage +Internally, they normalise a scalar to `[$scalar]` before returning the storage🤖 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/README.md` around lines 53 - 58, The README has a spelling-variant inconsistency in the cardinality section: the term used in this passage should match the earlier “normalised” wording already used in the same document. Update the wording in the Cardinality description to use the same British spelling throughout, and keep the reference consistent with the surrounding Field README text.Source: Linters/SAST tools
🤖 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/Drupal/Driver/Core/Field/README.md`:
- Around line 53-58: The README has a spelling-variant inconsistency in the
cardinality section: the term used in this passage should match the earlier
“normalised” wording already used in the same document. Update the wording in
the Cardinality description to use the same British spelling throughout, and
keep the reference consistent with the surrounding Field README text.
In `@tests/fixtures/ConsumerProject/Driver/Field/DatetimeHandler.php`:
- Around line 20-29: The DatetimeHandler MARKER constant uses a timestamp with
an unintended real-world association; replace the current sentinel in
DatetimeHandler::MARKER with a neutral ISO 8601 value that is still clearly
distinct from the input date, and keep the rest of the handler behavior
unchanged so the fixture continues proving the custom handler ran.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 089f5b6f-de09-4a15-987f-d36efed9593d
📒 Files selected for processing (20)
src/Drupal/Driver/Core/Field/ColorFieldTypeHandler.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/README.mdsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phptests/Drupal/Tests/Driver/Kernel/Core/Field/ColorFieldTypeHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/CustomCoreKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/DefaultHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldHandlerRegistryKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/TextHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/TextWithSummaryHandlerKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/ColorFieldTypeHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.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/fixtures/ConsumerProject/Driver/Field/DatetimeHandler.php
💤 Files with no reviewable changes (12)
- tests/Drupal/Tests/Driver/Unit/Core/Field/TextWithSummaryHandlerTest.php
- tests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/TextHandlerTest.php
- src/Drupal/Driver/Core/Field/TextHandler.php
- tests/Drupal/Tests/Driver/Kernel/Core/Field/ColorFieldTypeHandlerKernelTest.php
- src/Drupal/Driver/Core/Field/TextWithSummaryHandler.php
- tests/Drupal/Tests/Driver/Kernel/Core/Field/TextWithSummaryHandlerKernelTest.php
- tests/Drupal/Tests/Driver/Kernel/Core/Field/TextHandlerKernelTest.php
- src/Drupal/Driver/Core/Field/TextLongHandler.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.php
- src/Drupal/Driver/Core/Field/ColorFieldTypeHandler.php
- tests/Drupal/Tests/Driver/Unit/Core/Field/ColorFieldTypeHandlerTest.php
'DefaultHandler::unsupportedPropertyReason()' now rejects only the two shapes the generic type system exposes - an entity-reference target ('DataReferenceTargetDefinition') and a complex or nested value ('ComplexDataDefinitionInterface'/'ListDataDefinitionInterface'). It enumerates no field-type or data-type strings, so it never rejects a field for being a datetime, boolean, or list; those are scalars it relays as-is.
Timezone-aware conversion stays in the dedicated 'DatetimeHandler', 'DaterangeHandler', and 'DateRecurHandler' where it belongs, rather than being detected by the default. Updated the unit test and the README classification policy to match.
…ated pass-through handlers. The property-shape classification - rejecting an entity-reference target or a complex/nested value - now lives in 'FieldClassifier::fieldDefaultExpandReason()' rather than inside the default handler. 'Core::getFieldHandler()' consults it before falling back to 'DefaultHandler' and throws the actionable message, so 'DefaultHandler' is a pure pass-through again. The classifier enumerates no field-type or data-type strings; value translation stays in the dedicated handlers. Restored 'TextHandler', 'TextLongHandler', 'TextWithSummaryHandler', and 'ColorFieldTypeHandler' as '@deprecated' for removal in 4.0.0. Their columns are plain scalars the generic default now relays, but they stay registered and functional for consumers that extend or reference them; on removal their types fall through to 'DefaultHandler' unchanged. Added a kernel test proving Core rejects an unhandled entity-reference field at resolution, and pointed the coverage test and unit tests at the shared classifier method.
AlexSkrypnyk
changed the title
…ction. The 'can the default relay this field' decision is a value-shape question - scalar vs entity-reference vs complex - which the README calls orthogonal to the F-row (origin/storage) axis 'FieldClassifier' owns. It now lives in its own 'FieldShapeClassifier'/'FieldShapeClassifierInterface' with two predicates, 'fieldIsEntityReference()' and 'fieldIsComplexValue()', mirroring 'FieldClassifier''s factory and version-override discipline via 'Core::createFieldShapeClassifier()'. 'Core::getFieldHandler()' consults the shape classifier before falling back to 'DefaultHandler' and throws when the field is an entity reference or a complex/nested value. 'FieldClassifier' is reverted to the nine F-row predicates only - the misplaced classification method is gone from it. Added 'FieldShapeClassifierTest'; 'FieldTypeCoverageKernelTest' and the Core enforcement (proved by 'DefaultHandlerEnforcementKernelTest') consult the new classifier.
AlexSkrypnyk
changed the title
The 'driver_field_test' fixture module - wired in through a composer path repository, the same mechanism as the DrupalExtension-smoke leg - ships two custom field types the driver has no handler for: 'driver_test_scalar' (plain-scalar columns) and 'driver_test_reference' (an entity-reference target column). It stands in for any contrib module that introduces its own field type. 'CustomModuleFieldKernelTest' drives both through the real 'Core' with every built-in handler registered: the scalar type rides 'DefaultHandler' and round-trips through real storage, while the reference type is refused at handler resolution with the actionable 'register a dedicated handler' exception. This replaces the contrived 'HandlerlessCore'-based enforcement test with a realistic end-to-end demonstration.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Drupal/Tests/Driver/Unit/Core/Field/FieldShapeClassifierTest.php (1)
56-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the
ListDataDefinitionInterfacebranch.
testFieldIsComplexValue()only exercises theComplexDataDefinitionInterfacebranch (viaMapDataDefinition).FieldShapeClassifier::fieldIsComplexValue()also returnsTRUEforListDataDefinitionInterfaceproperties, but that branch has no test.Suggested addition
public function testFieldIsComplexValue(): void { $classifier = new FieldShapeClassifier(); $this->assertTrue($classifier->fieldIsComplexValue($this->storageWithProperties([ 'value' => DataDefinition::create('string'), 'options' => MapDataDefinition::create(), ]))); + // A list-typed property is also complex. + $this->assertTrue($classifier->fieldIsComplexValue($this->storageWithProperties([ + 'value' => DataDefinition::create('string'), + 'items' => ListDataDefinition::create('string'), + ]))); + // Plain scalars are not complex. $this->assertFalse($classifier->fieldIsComplexValue($this->storageWithProperties([ 'value' => DataDefinition::create('string'), 'format' => DataDefinition::create('string'), ]))); }🤖 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/FieldShapeClassifierTest.php` around lines 56 - 69, Add test coverage in testFieldIsComplexValue for the ListDataDefinitionInterface path in FieldShapeClassifier::fieldIsComplexValue, since the current assertions only verify the ComplexDataDefinitionInterface case with MapDataDefinition. Extend the existing test to build storageWithProperties using a list-type property definition and assert that fieldIsComplexValue returns true for that shape, while keeping the existing scalar false case as-is.
🤖 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 `@tests/Drupal/Tests/Driver/Unit/Core/Field/FieldShapeClassifierTest.php`:
- Around line 56-69: Add test coverage in testFieldIsComplexValue for the
ListDataDefinitionInterface path in FieldShapeClassifier::fieldIsComplexValue,
since the current assertions only verify the ComplexDataDefinitionInterface case
with MapDataDefinition. Extend the existing test to build storageWithProperties
using a list-type property definition and assert that fieldIsComplexValue
returns true for that shape, while keeping the existing scalar false case as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ea3170f-3159-4cab-90ac-121ad07db12c
📒 Files selected for processing (14)
composer.jsonsrc/Drupal/Driver/Core/Core.phpsrc/Drupal/Driver/Core/CoreInterface.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/FieldShapeClassifier.phpsrc/Drupal/Driver/Core/Field/FieldShapeClassifierInterface.phpsrc/Drupal/Driver/Core/Field/README.mdtests/Drupal/Tests/Driver/Kernel/Core/Field/CustomModuleFieldKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldShapeClassifierTest.phptests/fixtures/modules/driver_field_test/composer.jsontests/fixtures/modules/driver_field_test/driver_field_test.info.ymltests/fixtures/modules/driver_field_test/src/Plugin/Field/FieldType/DriverTestReferenceItem.phptests/fixtures/modules/driver_field_test/src/Plugin/Field/FieldType/DriverTestScalarItem.php
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/modules/driver_field_test/composer.json
- tests/fixtures/modules/driver_field_test/driver_field_test.info.yml
- src/Drupal/Driver/Core/Field/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.php
- src/Drupal/Driver/Core/Field/DefaultHandler.php
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
When a field type has no dedicated handler,
Coreroutes it through a value-shape classifier before falling back toDefaultHandler. Because a field's value shape (scalar vs entity-reference vs complex) is orthogonal to the origin/storage F-row axis thatFieldClassifierowns, this is a separate, focused abstraction -FieldShapeClassifierInterface/FieldShapeClassifier- built with the same factory and version-override discipline asFieldClassifier(Core::createFieldShapeClassifier(), overridable by aCore{N}\Field\FieldShapeClassifier).Core::getFieldHandler()asks the shape classifier two predicates -fieldIsEntityReference()andfieldIsComplexValue()- and throws an actionable exception when either is true, because those shapes cannot be authored as a plain scalar (an entity-reference id the author cannot know; a complex/nested value with no scalar form). The classifier reads only the stored (non-computed) property definitions and enumerates no field-type or data-type strings, so datetime strings, booleans, and list keys are plain scalars the default relays verbatim - their translation stays in dedicated handlers.Detection is proactive rather than try/catch, because both shapes fail silently (a label persisted as a bogus id, a nested value flattened), so the field is refused at handler resolution rather than after a corrupt save.
FieldClassifieris unchanged - it remains exactly the nine F-row predicates.Deprecations
TextHandler,TextLongHandler,TextWithSummaryHandler, andColorFieldTypeHandlerhandle field types whose columns are plain scalars the generic default now relays, so they are redundant. They remain registered and functional but are@deprecatedfor removal indrupal-driver:4.0.0, kept for consumers that extend or reference them. On removal, their field types fall through toDefaultHandlerwith no behavioural change.Changes
FieldShapeClassifierInterface/FieldShapeClassifier- a dedicated value-shape classifier withfieldIsEntityReference()andfieldIsComplexValue()predicates over a field's stored (non-computed) property definitions.Core::createFieldShapeClassifier()/getFieldShapeClassifier()(declared onCoreInterface), mirroring the F-row classifier's factory + lazy-getter + version-override pattern.Core::getFieldHandler()consults the shape classifier when it would fall back toDefaultHandlerand throws an exception naming the field, type, entity type, bundle, and why the default cannot relay it.DefaultHandlerto a pure pass-through; the old single-columngetColumns()guard is gone.TextHandler,TextLongHandler,TextWithSummaryHandler, andColorFieldTypeHandler@deprecatedfor 4.0.0.FieldShapeClassifierTestandDefaultHandlerEnforcementKernelTest; pointedFieldTypeCoverageKernelTestat the shape classifier.FieldClassifierand its test are untouched.Before / After
Summary by CodeRabbit
New Features
Bug Fixes
Chores