Restructured cores into 'Core/' with version-override lookup chain.#338
Merged
AlexSkrypnyk merged 6 commits intomasterfrom Apr 17, 2026
Merged
Restructured cores into 'Core/' with version-override lookup chain.#338AlexSkrypnyk merged 6 commits intomasterfrom
AlexSkrypnyk merged 6 commits intomasterfrom
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors the DrupalDriver codebase from a version-specific namespace structure ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DrupalDriver
participant CoreResolver
participant Core99
participant Core
Client->>DrupalDriver: setCoreFromVersion(99)
DrupalDriver->>CoreResolver: Build lookup chain for v99→10
CoreResolver->>CoreResolver: Generate candidates: Core99, Core98...Core10, Core
loop Try each candidate
CoreResolver->>CoreResolver: class_exists(Drupal\Driver\Core99\Core)?
alt Class exists
CoreResolver->>Core99: Instantiate Core99
Core99-->>DrupalDriver: Core99 instance (✓)
else Try next version
CoreResolver->>CoreResolver: class_exists(Core98)?
end
end
sequenceDiagram
participant Test
participant AbstractCore
participant FieldResolver
participant Core99Handler
participant DefaultHandler
Test->>AbstractCore: getFieldHandler(field_file, entity_type)
AbstractCore->>FieldResolver: Build lookup chain for v99→10
FieldResolver->>FieldResolver: Try Core99\Field\FileHandler
alt v99 override exists
FieldResolver->>Core99Handler: Instantiate Core99\Field\FileHandler
Core99Handler-->>Test: Handler instance (Core99Handler with MARKER)
else v99 doesn't exist
FieldResolver->>FieldResolver: Try Core98...Core10
else Fallback to default
FieldResolver->>DefaultHandler: Instantiate Core\Field\FileHandler
DefaultHandler-->>Test: Handler instance (Default)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)PHPStan was skipped because the config uses disallowed 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: 35%) 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: 3
🧹 Nitpick comments (10)
phpcs.xml (1)
19-19: Pattern won't cover version-override core directories.
src/Drupal/Driver/Core/*.phpmatches only files directly underCore/(e.g.,Core.php,AbstractCore.php). Per-version override directories introduced by this PR's pattern (Core12/, etc.) won't be covered if/when they also need to set$_SERVERduring bootstrap, and will tripDrupal.Semantics.RemoteAddress.RemoteAddress. No overrides ship here so it's not blocking, but consider broadening now to avoid surprises later.♻️ Suggested broadening
- <exclude-pattern>src/Drupal/Driver/Core/*.php</exclude-pattern> + <exclude-pattern>src/Drupal/Driver/Core*/*.php</exclude-pattern>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpcs.xml` at line 19, The current phpcs exclude pattern "src/Drupal/Driver/Core/*.php" only matches files directly under Core/ and will miss per-version override directories like Core12/, so update the exclude-pattern in phpcs.xml to broadly match Core and any versioned Core directories (e.g., use a glob that covers Core*/** or Core*/**/*.php) so files under Core12/, Core13/, etc. are also excluded from Drupal.Semantics.RemoteAddress.RemoteAddress checks.src/Drupal/Driver/Core/Field/TimeHandler.php (1)
5-9: Namespace migration looks correct.Optional nit: the class docblock still says "Time field handler for Drupal 8" (line 8), which is now misleading given the handler lives under the version-agnostic
Core\Fieldnamespace and is the default across D10/11/12. Same applies toNameHandler,LinkHandler, andAddressHandler. Consider a sweep to drop the "for Drupal 8" phrasing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Core/Field/TimeHandler.php` around lines 5 - 9, Update the docblock for the TimeHandler class (and similarly for NameHandler, LinkHandler, AddressHandler) to remove the phrase "for Drupal 8" and replace it with a version-agnostic description such as "Time field handler" (or "Field handler for core" / "Default time field handler") so the comment accurately reflects that these handlers live under Core\Field and apply to D10/11/12; edit the docblock above the TimeHandler class declaration to change the string and keep the existing short description style.doc/extending.rst (1)
70-70: Nit: double space.
"``Core`` and ``Field\*Handler``"contains a stray double space betweenCoreandand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/extending.rst` at line 70, Fix the stray double space in the documentation line by replacing the two spaces between the symbols so it reads "**Default implementation:** ``Core`` and ``Field*Handler``"; update the text that references the Core and Field*Handler symbols to use a single space between ``Core`` and "and" to remove the double-space typo.src/Drupal/Driver/Core/AbstractCore.php (2)
51-63: Confirm the0sentinel contract.Returning
0here plus the$n >= 10loop guard ingetFieldHandler()is an intentional "skip the versioned walk for the default Core" trick. Please add a matching note inCoreInterface(or wherever the contract is surfaced) so that anyone adding a newCore{N}\Coreremembers to overridegetVersion()— otherwise their versioned field-handler directory will never be consulted, silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Core/AbstractCore.php` around lines 51 - 63, Add a documentation note to the CoreInterface explaining the sentinel contract: the default AbstractCore::getVersion() returns 0 to skip versioned handler lookup (the getFieldHandler() loop only iterates when $n >= 10), so any versioned core classes (e.g., Core{N}\Core implementations) must override getVersion() to return their major version number; update CoreInterface's docblock to describe this behavior and the requirement to override getVersion() to ensure versioned field-handler directories are considered.
68-91: Lookup chain is correct; minor dedup/perf opportunity.The two
forloops building$candidatesand$default_candidatesare structurally identical. You can flatten into a single interleaved list (per-version type handler → per-version default) or two small helpers, which also removes thearray_mergecall:♻️ Example simplification
- $candidates = []; - for ($n = $version; $n >= 10; $n--) { - $candidates[] = sprintf('\\Drupal\\Driver\\Core%d\\Field\\%sHandler', $n, $camelized_type); - } - $candidates[] = sprintf('\\Drupal\\Driver\\Core\\Field\\%sHandler', $camelized_type); - - $default_candidates = []; - for ($n = $version; $n >= 10; $n--) { - $default_candidates[] = sprintf('\\Drupal\\Driver\\Core%d\\Field\\DefaultHandler', $n); - } - $default_candidates[] = DefaultHandler::class; - - foreach (array_merge($candidates, $default_candidates) as $class) { + $candidates = []; + for ($n = $version; $n >= 10; $n--) { + $candidates[] = sprintf('\\Drupal\\Driver\\Core%d\\Field\\%sHandler', $n, $camelized_type); + } + $candidates[] = sprintf('\\Drupal\\Driver\\Core\\Field\\%sHandler', $camelized_type); + for ($n = $version; $n >= 10; $n--) { + $candidates[] = sprintf('\\Drupal\\Driver\\Core%d\\Field\\DefaultHandler', $n); + } + $candidates[] = DefaultHandler::class; + + foreach ($candidates as $class) { if (class_exists($class)) { return new $class($entity, $entity_type, $field_name); } } throw new \RuntimeException(sprintf('No field handler found for type "%s".', $camelized_type));Secondary concern:
getFieldHandler()is on the hot path ofexpandEntityFields(), which calls it per field. Every miss triggers an autoloader lookup. In practice the range is 10–12 so this is negligible today, but if a per-(version,type) cache is cheap to add you’ll insulate yourself from future growth.Also: the final
throwis effectively unreachable becauseDefaultHandler::classis imported at the top and thus always resolvable — happy to keep it as defensive code, just noting it won’t be covered by any realistic test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Core/AbstractCore.php` around lines 68 - 91, getFieldHandler builds two identical per-version loops ($candidates and $default_candidates) causing redundant work and repeated autoloader lookups; refactor by constructing a single interleaved candidate list (for each version push the type handler then the version DefaultHandler, then fall back to global DefaultHandler::class) or extract a small helper to produce candidates so you can remove array_merge, and add a simple static or private cache inside getFieldHandler keyed by version+camelized type to remember the resolved handler class (store the class name, not an instance) so subsequent calls do a direct instantiation instead of re-trying class_exists for each candidate; keep the final RuntimeException as defensive code and continue to instantiate the resolved class with new $class($entity, $entity_type, $field_name).tests/Drupal/Tests/Driver/CoreLookupTest.php (2)
41-47: Fallback test depends onDrupal\Driver\Core50\Corenever existing.
testLookupFallsBackToDefault()uses50as the "no override" sentinel. This is fine today, but an innocuous future fixture namedCore50would silently break the test. Using an extreme/obviously-fake version like999(paired with a comment) or introducing a constant next to the fixture directory would remove that foot-gun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/CoreLookupTest.php` around lines 41 - 47, The test testLookupFallsBackToDefault() relies on createDriverWithVersion(50) assuming no Core50 fixture exists; change the sentinel to an obviously fake/extreme version (e.g. 999) or introduce a named constant (e.g. NO_OVERRIDE_VERSION) near the test/fixture configuration and use that constant in createDriverWithVersion(), then run setCoreFromVersion() and assertInstanceOf(Core::class, $driver->getCore()) as before so the test no longer depends on the accidental absence of a real Core50 fixture.
61-75: Reflection onreadonlyprops works here, but fragile ifDrupalDriveris ever reshuffled.
newInstanceWithoutConstructor()leavesdrupalRoot/uriuninitialized, soReflectionProperty::setValue()is permitted on thereadonlydeclarations — that’s the only reason this works. If someone later moves either property out ofDrupalDriver(or gives it a default), the reflection path will start throwingError: Cannot modify readonly property.Consider either:
- A small factory hook in
DrupalDriver(e.g.,static createForTesting(int $version, string $root): self) that the test can call instead of poking private state, or- A brief comment on the helper documenting the
newInstanceWithoutConstructor()+ readonly dependency so future maintainers understand why this is written the way it is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/CoreLookupTest.php` around lines 61 - 75, The test helper createDriverWithVersion relies on reflection/newInstanceWithoutConstructor to set readonly properties (drupalRoot and uri) which is fragile; add a small public static factory on DrupalDriver (e.g., DrupalDriver::createForTesting(int $version, string $drupalRoot, string $uri = 'default')) that constructs a properly initialised instance for tests and update createDriverWithVersion to call that factory instead of using ReflectionProperty::setValue; alternatively, if you prefer not to change production code, add a clear comment inside createDriverWithVersion explaining the reliance on newInstanceWithoutConstructor(), ReflectionProperty::setValue(), and readonly behavior so future maintainers understand why reflection is used.tests/Drupal/Tests/Driver/FieldHandlerLookupTest.php (2)
118-122:->with('bundle')makes the mock brittle.
$entity_type->method('getKey')->with('bundle')->willReturn('type')is scoped to a specific argument. If any current or future field handler callsgetKey()with anything other than'bundle'(e.g.,'id','label'), PHPUnit will fail the expectation rather than returning a default. Given the test only needs the field-lookup chain to resolve, consider removing the->with(...)constraint, or using awillReturnMap()for the key names you know about.♻️ Suggested loosening
- $entity_type->method('getKey')->with('bundle')->willReturn('type'); + $entity_type->method('getKey')->willReturnMap([ + ['bundle', 'type'], + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/FieldHandlerLookupTest.php` around lines 118 - 122, The mock on EntityTypeInterface is too strict because $entity_type->method('getKey')->with('bundle')->willReturn('type') will fail if getKey() is called with any other argument; update the test by removing the ->with('bundle') constraint on the getKey() mock (or replace it with a willReturnMap for the known key names) so getKey() returns expected values for multiple keys; adjust the mock setup around $entity_type (EntityTypeInterface::class, getKey) and ensure $entity_type_manager->method('getDefinition') still returns $entity_type.
132-157: Consider movingCore99TestCoreout of the test file.Declaring a second top-level class in a PHPUnit test file works but has two downsides:
- Test discovery tools occasionally flag multiple top-level classes per file.
- The class can’t be reused by sibling tests (e.g., a future
FieldHandlerLookupExtendedTest) without copy/paste.Either move it to
tests/fixtures/Drupal/Driver/Core99/TestCore.php(alongside the other Core99 fixtures — nicely symmetrical with the PR’s autoload-dev setup), or make it a private nested anonymous class if each test only needs a one-shot instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/FieldHandlerLookupTest.php` around lines 132 - 157, Extract the Core99TestCore test helper (subclassing AbstractCore and implementing getVersion() and getEntityFieldTypes()) out of this test file into its own dedicated fixture class file so it can be reused by other tests and avoids multiple top-level classes in one PHPUnit file; update the test to instantiate that fixture instead of declaring the class inline, or alternatively replace the inline class with a private nested anonymous class if the helper is truly one-off.src/Drupal/Driver/DrupalDriver.php (1)
227-241: Lookup chain is correct; one edge-case & minor perf note.The descending
$version..10walk plusCore::classterminal is fine and matchesAbstractCore::getFieldHandler()'s pattern for consistency.Two small observations:
- If
$version < 10(shouldn't happen today becausedetectMajorVersion()already guards), theforloop is simply skipped and onlyCore::classis tried. That's benign, but worth an assertion or early-return comment to make the invariant explicit for future maintainers.class_exists()triggers autoload on every candidate. For a large detected version (e.g., the99used by lookup tests) this amounts to ~90 failed autoload attempts on a cold run. In production where the range is 10–12 this is trivially 1–3 attempts, so not a blocker — just flagging for future-proofing if the ceiling ever grows.Note: the CI
ParseErrorreported on this line is a downstream effect of a PHP 8.3 typed-constant in theCore99fixture — see the comment ontests/Drupal/Tests/Driver/CoreLookupTest.phpfor the root cause and fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/DrupalDriver.php` around lines 227 - 241, Add an explicit invariant check and a short note about autoload cost in setCoreFromVersion(): assert or throw if $this->getDrupalVersion() < 10 to make the precondition explicit (so callers/maintainers know detectMajorVersion() must have run), and add a brief comment above the for-loop documenting that class_exists() triggers autoload attempts and may be costly for large ranges (hinting that future optimization could use class_exists($class, false) or a bounded autoload strategy); keep the existing candidate lookup and BootstrapException behavior unchanged and reference the setCoreFromVersion(), getDrupalVersion(), Core::class and $this->core symbols so the change is easy to find.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/extending.rst`:
- Around line 115-122: The example for Core class lookup misrepresents the
short-circuit behavior of setCoreFromVersion(): after finding Core13\Core it
returns and does not evaluate later fallbacks, so change the illustration to
either stop the chain at "Try: Drupal\Driver\Core13\Core ← exists, use it" or
relabel subsequent lines (e.g. "Try: Drupal\Driver\Core12\Core ← not reached",
"Try: Drupal\Driver\Core\Core ← not reached") to accurately reflect that
setCoreFromVersion() returns on the first match.
In `@tests/Drupal/Tests/Driver/CoreLookupTest.php`:
- Around line 30-36: Remove the PHP 8.3 typed class-constant annotations from
the test fixtures so they parse on PHP 8.2: update the Core99\Core class
constant (currently declared as "public const string MARKER = 'Core99\\Core';")
to an untyped constant ("public const MARKER = 'Core99\\Core';") and do the same
for the constant(s) in Core99\Field\FileHandler; do not change composer.json
(leave PHP ^8.2) unless you intentionally want to adopt Option B and raise the
minimum PHP to 8.3.
In `@tests/fixtures/Drupal/Driver/Core99/Core.php`:
- Line 21: The class constant declaration "public const string MARKER" uses a
PHP 8.3 typed-constant feature and breaks parsing on PHP 8.2; remove the type
annotation so the constant is declared as "public const MARKER" (retain the same
value 'Core99\\Core') within the Core class (symbol: MARKER) to restore
compatibility with PHP 8.2.
---
Nitpick comments:
In `@doc/extending.rst`:
- Line 70: Fix the stray double space in the documentation line by replacing the
two spaces between the symbols so it reads "**Default implementation:** ``Core``
and ``Field*Handler``"; update the text that references the Core and
Field*Handler symbols to use a single space between ``Core`` and "and" to remove
the double-space typo.
In `@phpcs.xml`:
- Line 19: The current phpcs exclude pattern "src/Drupal/Driver/Core/*.php" only
matches files directly under Core/ and will miss per-version override
directories like Core12/, so update the exclude-pattern in phpcs.xml to broadly
match Core and any versioned Core directories (e.g., use a glob that covers
Core*/** or Core*/**/*.php) so files under Core12/, Core13/, etc. are also
excluded from Drupal.Semantics.RemoteAddress.RemoteAddress checks.
In `@src/Drupal/Driver/Core/AbstractCore.php`:
- Around line 51-63: Add a documentation note to the CoreInterface explaining
the sentinel contract: the default AbstractCore::getVersion() returns 0 to skip
versioned handler lookup (the getFieldHandler() loop only iterates when $n >=
10), so any versioned core classes (e.g., Core{N}\Core implementations) must
override getVersion() to return their major version number; update
CoreInterface's docblock to describe this behavior and the requirement to
override getVersion() to ensure versioned field-handler directories are
considered.
- Around line 68-91: getFieldHandler builds two identical per-version loops
($candidates and $default_candidates) causing redundant work and repeated
autoloader lookups; refactor by constructing a single interleaved candidate list
(for each version push the type handler then the version DefaultHandler, then
fall back to global DefaultHandler::class) or extract a small helper to produce
candidates so you can remove array_merge, and add a simple static or private
cache inside getFieldHandler keyed by version+camelized type to remember the
resolved handler class (store the class name, not an instance) so subsequent
calls do a direct instantiation instead of re-trying class_exists for each
candidate; keep the final RuntimeException as defensive code and continue to
instantiate the resolved class with new $class($entity, $entity_type,
$field_name).
In `@src/Drupal/Driver/Core/Field/TimeHandler.php`:
- Around line 5-9: Update the docblock for the TimeHandler class (and similarly
for NameHandler, LinkHandler, AddressHandler) to remove the phrase "for Drupal
8" and replace it with a version-agnostic description such as "Time field
handler" (or "Field handler for core" / "Default time field handler") so the
comment accurately reflects that these handlers live under Core\Field and apply
to D10/11/12; edit the docblock above the TimeHandler class declaration to
change the string and keep the existing short description style.
In `@src/Drupal/Driver/DrupalDriver.php`:
- Around line 227-241: Add an explicit invariant check and a short note about
autoload cost in setCoreFromVersion(): assert or throw if
$this->getDrupalVersion() < 10 to make the precondition explicit (so
callers/maintainers know detectMajorVersion() must have run), and add a brief
comment above the for-loop documenting that class_exists() triggers autoload
attempts and may be costly for large ranges (hinting that future optimization
could use class_exists($class, false) or a bounded autoload strategy); keep the
existing candidate lookup and BootstrapException behavior unchanged and
reference the setCoreFromVersion(), getDrupalVersion(), Core::class and
$this->core symbols so the change is easy to find.
In `@tests/Drupal/Tests/Driver/CoreLookupTest.php`:
- Around line 41-47: The test testLookupFallsBackToDefault() relies on
createDriverWithVersion(50) assuming no Core50 fixture exists; change the
sentinel to an obviously fake/extreme version (e.g. 999) or introduce a named
constant (e.g. NO_OVERRIDE_VERSION) near the test/fixture configuration and use
that constant in createDriverWithVersion(), then run setCoreFromVersion() and
assertInstanceOf(Core::class, $driver->getCore()) as before so the test no
longer depends on the accidental absence of a real Core50 fixture.
- Around line 61-75: The test helper createDriverWithVersion relies on
reflection/newInstanceWithoutConstructor to set readonly properties (drupalRoot
and uri) which is fragile; add a small public static factory on DrupalDriver
(e.g., DrupalDriver::createForTesting(int $version, string $drupalRoot, string
$uri = 'default')) that constructs a properly initialised instance for tests and
update createDriverWithVersion to call that factory instead of using
ReflectionProperty::setValue; alternatively, if you prefer not to change
production code, add a clear comment inside createDriverWithVersion explaining
the reliance on newInstanceWithoutConstructor(), ReflectionProperty::setValue(),
and readonly behavior so future maintainers understand why reflection is used.
In `@tests/Drupal/Tests/Driver/FieldHandlerLookupTest.php`:
- Around line 118-122: The mock on EntityTypeInterface is too strict because
$entity_type->method('getKey')->with('bundle')->willReturn('type') will fail if
getKey() is called with any other argument; update the test by removing the
->with('bundle') constraint on the getKey() mock (or replace it with a
willReturnMap for the known key names) so getKey() returns expected values for
multiple keys; adjust the mock setup around $entity_type
(EntityTypeInterface::class, getKey) and ensure
$entity_type_manager->method('getDefinition') still returns $entity_type.
- Around line 132-157: Extract the Core99TestCore test helper (subclassing
AbstractCore and implementing getVersion() and getEntityFieldTypes()) out of
this test file into its own dedicated fixture class file so it can be reused by
other tests and avoids multiple top-level classes in one PHPUnit file; update
the test to instantiate that fixture instead of declaring the class inline, or
alternatively replace the inline class with a private nested anonymous class if
the helper is truly one-off.
🪄 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: b57fe4ca-e8f1-4fcb-b5c3-6e6df321a590
📒 Files selected for processing (53)
composer.jsondoc/extending.rstdoc/index.rstphpcs.xmlrector.phpspec/Drupal/Driver/Core/CoreSpec.phpsrc/Drupal/Driver/Core/AbstractCore.phpsrc/Drupal/Driver/Core/Core.phpsrc/Drupal/Driver/Core/CoreAuthenticationInterface.phpsrc/Drupal/Driver/Core/CoreInterface.phpsrc/Drupal/Driver/Core/Field/AbstractHandler.phpsrc/Drupal/Driver/Core/Field/AddressHandler.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/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/ListIntegerHandler.phpsrc/Drupal/Driver/Core/Field/ListStringHandler.phpsrc/Drupal/Driver/Core/Field/NameHandler.phpsrc/Drupal/Driver/Core/Field/OgStandardReferenceHandler.phpsrc/Drupal/Driver/Core/Field/SupportedImageHandler.phpsrc/Drupal/Driver/Core/Field/TaxonomyTermReferenceHandler.phpsrc/Drupal/Driver/Core/Field/TextWithSummaryHandler.phpsrc/Drupal/Driver/Core/Field/TimeHandler.phpsrc/Drupal/Driver/DrupalDriver.phptests/Drupal/Tests/Driver/Core/Field/AddressHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/DaterangeHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/EntityReferenceHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/FileHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/ImageHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/ListHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/SupportedImageHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/TaxonomyTermReferenceHandlerTest.phptests/Drupal/Tests/Driver/Core/Field/TextWithSummaryHandlerTest.phptests/Drupal/Tests/Driver/CoreLookupTest.phptests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.phptests/Drupal/Tests/Driver/Drupal8PermissionsTest.phptests/Drupal/Tests/Driver/Drupal8Test.phptests/Drupal/Tests/Driver/FieldHandlerLookupTest.phptests/Drupal/Tests/Driver/LinkHandlerTest.phptests/Drupal/Tests/Driver/NameHandlerTest.phptests/Drupal/Tests/Driver/TimeHandlerTest.phptests/fixtures/Drupal/Driver/Core99/Core.phptests/fixtures/Drupal/Driver/Core99/Field/FileHandler.php
This was referenced Apr 19, 2026
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.
Part of #312
Summary
Restructures the source layout so each Drupal version can override behavior cleanly without copy-pasting the entire implementation. The old
Cores/Drupal8.php(which actually served D8-D12+) is renamed toCore/Core.php, the abstract Core layer is consolidated underCore/, and field handlers move next to the Core they belong to (Core/Field/). Version-specific overrides live inCore{N}/directories (e.g.,Core12/) - these don't exist today; the lookup chain inDrupalDriver::setCoreFromVersion()andAbstractCore::getFieldHandler()falls back to the defaultCore\CoreandCore\Field\*when no override is registered.Changes
Documentation
doc/extending.rstexplaining the Core layer, the lookup chain, and how to add per-version customizations.doc/index.rsttoctree updated.Source restructure
src/Drupal/Driver/Cores/->src/Drupal/Driver/Core/(singular).src/Drupal/Driver/Cores/Drupal8.php->src/Drupal/Driver/Core/Core.php. Class renamed fromDrupal8toCore.src/Drupal/Driver/Fields/Drupal8/*.php(17 files) ->src/Drupal/Driver/Core/Field/*.php.src/Drupal/Driver/Fields/FieldHandlerInterface.php->src/Drupal/Driver/Core/Field/FieldHandlerInterface.php.git mvto preserve history.Lookup chain
DrupalDriver::setCoreFromVersion(): walksDrupal\Driver\Core{N}\Corefrom the detected major version down toDrupal\Driver\Core\Core. First existing class wins.AbstractCore::getFieldHandler(): walksDrupal\Driver\Core{N}\Field\{X}Handlersimilarly, falling back toDrupal\Driver\Core\Field\{X}Handlerand then toDefaultHandlercandidates.AbstractCore::getVersion()method (returns0by default; future per-versionCore{N}\Coreclasses override it to return their version).DrupalDriver::detectMajorVersion()now returns the actual Drupal major version (10, 11, 12) instead of always returning8.Tests + spec
Fields/Drupal8/moved toCore/Field/(13 files).Drupal8Test,Drupal8FieldMethodsTest,Drupal8PermissionsTestkept at root; namespaces and references to old class names updated; internal helper classes renamed (TestDrupal8Core->TestCore).spec/Drupal/Driver/Cores/Drupal8Spec.php->spec/Drupal/Driver/Core/CoreSpec.php. Class renamed.Config
phpcs.xmlexclude-pattern path updated fromsrc/Drupal/Driver/Cores/*.phptosrc/Drupal/Driver/Core/*.php.Before
After (this PR)
No
Core{N}/directories exist yet. They are not needed today - all currently-supported Drupal versions use the default inCore/.Future (when a Drupal version diverges)
When Drupal 12 ships and one handler needs different behavior:
The
Core12/Field/FileHandler.phpextends the default and overrides only what changed:Adding a new Drupal version (D13) that diverges further:
The lookup chain at runtime for D13:
The pattern keeps overrides local to the version that introduced them and avoids duplicating the entire handler set per Drupal version. New Drupal versions that don't diverge cost zero new files. See
doc/extending.rstfor the full guide.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests