Added smoke tests with the downstream project.#355
Closed
AlexSkrypnyk wants to merge 13 commits intomasterfrom
Closed
Added smoke tests with the downstream project.#355AlexSkrypnyk wants to merge 13 commits intomasterfrom
AlexSkrypnyk wants to merge 13 commits intomasterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors field classification into a bundle-aware FieldClassifier used by Core, removes the old FieldCapabilityInterface and related driver methods, adds multiple new/updated field handlers and extensive unit/kernel tests, updates docs/UPGRADING, and extends CI with a manual Changes
Sequence Diagram(s)sequenceDiagram
participant Core as Core
participant Classifier as FieldClassifier
participant EFM as EntityFieldManager
participant Handler as FieldHandler
participant Storage as EntityStorage/DB
Core->>Classifier: create/resolve classifier (classifier())
Classifier->>EFM: request base/field/bundle definitions
Core->>Classifier: evaluate predicates (e.g., fieldIsBundleStorageBacked)
alt field requires expansion
Core->>Handler: select handler and call expand(stub value)
Handler->>Storage: query/load target entities / revisions
Storage-->>Handler: return ids/revision ids
Handler-->>Core: return expanded field data
else field skipped
Classifier-->>Core: classified as passthrough (no expansion)
end
sequenceDiagram
participant Runner as GH Actions Runner
participant Repo as drupaldriver
participant Ext as drupalextension
participant PHP as PHP 8.4
participant Composer as Composer
participant PHPUnit as PHPUnit
Runner->>Repo: checkout drupaldriver
Runner->>Ext: checkout DRUPALEXTENSION_REPO@DRUPALEXTENSION_REF into drupalextension
Runner->>PHP: setup PHP 8.4 + extensions
Runner->>Composer: enable cache
Composer->>Ext: add local-path repo -> ../drupaldriver and require drupal/drupal-driver:*
Composer->>Composer: composer update
Composer->>Composer: composer show (verify local resolution)
Runner->>Ext: ln -s /var/www/html/build -> drupalextension checkout
Runner->>PHPUnit: run vendor/bin/phpunit in drupalextension
PHPUnit-->>Runner: test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 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 |
…ension' smoke check blocking.
![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: 1
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
223-227: Composer cache is effectively unused here (and in the other jobs).The cache path is
/tmp/composer-cache, butCOMPOSER_CACHE_DIRis never set, so Composer writes to its default (~/.cache/composer). Theactions/cachestep will just persist an empty directory. Not a regression from this PR — thelintandtestsjobs have the same shape — but since you're adding a third copy, consider either settingCOMPOSER_CACHE_DIR: /tmp/composer-cacheas a workflow-level env var or dropping the cache steps. Happy to open a separate cleanup PR if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 223 - 227, The Composer cache step is saving /tmp/composer-cache but Composer still writes to ~/.cache/composer; set the COMPOSER_CACHE_DIR environment variable to /tmp/composer-cache so Composer uses the cached folder (either as a workflow-level env or in each job that runs the Cache Composer dependencies step), or remove the actions/cache step entirely if you prefer not to cache; update the jobs that include the Cache Composer dependencies step (e.g., lint and tests) to use the same COMPOSER_CACHE_DIR setting to ensure the cache is actually populated and restored.
252-256: Symlink step assumes/var/www/htmlexists and is writable viasudo; worth hardening.On
ubuntu-latest/var/www/htmlexists today (apache2 is preinstalled), but the job silently depends on that runner detail. Also,ln -sfnbehaves surprisingly if/var/www/html/buildever exists as a real directory (it creates the symlink inside it rather than replacing it). A small hardening:🛠️ Proposed tweak
- - name: Symlink expected Drupal root for BrowserKitFactoryTest - run: sudo ln -sfn "${GITHUB_WORKSPACE}/drupalextension" /var/www/html/build + - name: Symlink expected Drupal root for BrowserKitFactoryTest + run: | + sudo mkdir -p /var/www/html + sudo rm -rf /var/www/html/build + sudo ln -s "${GITHUB_WORKSPACE}/drupalextension" /var/www/html/buildAlso worth leaving a note that this couples the smoke job to a hard-coded path in
BrowserKitFactoryTest; a cleaner long-term fix is for that test to accept an env-overridable root so CI doesn't needsudoat all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 252 - 256, The workflow step named "Symlink expected Drupal root for BrowserKitFactoryTest" assumes /var/www/html exists and that ln -sfn will replace any preexisting build path; update that step to (1) ensure the parent exists with mkdir -p /var/www/html, (2) check and remove an existing /var/www/html/build whether it's a file, symlink or directory (use a safe rm -rf on that target only), (3) create the symlink to ${GITHUB_WORKSPACE}/drupalextension, and (4) avoid unnecessary sudo by checking writability before using sudo; also add a comment pointing out BrowserKitFactoryTest’s hard-coded path and suggest making it env-overridable to avoid CI coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 14-18: Update the inline comment above
DRUPALEXTENSION_REPO/DRUPALEXTENSION_REF to explicitly state that the default
DRUPALEXTENSION_REF currently points to the temporary sister branch
"feature/drupal-driver-3x" and must be reverted to "main" after that branch is
merged; add a TODO with an issue or PR link to track flipping it back and/or
mention re-enabling continue-on-error for the "extension" job until the revert
is completed so CI does not silently break if the ephemeral branch is renamed or
removed (refer to DRUPALEXTENSION_REF and the "extension" job in the workflow).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 223-227: The Composer cache step is saving /tmp/composer-cache but
Composer still writes to ~/.cache/composer; set the COMPOSER_CACHE_DIR
environment variable to /tmp/composer-cache so Composer uses the cached folder
(either as a workflow-level env or in each job that runs the Cache Composer
dependencies step), or remove the actions/cache step entirely if you prefer not
to cache; update the jobs that include the Cache Composer dependencies step
(e.g., lint and tests) to use the same COMPOSER_CACHE_DIR setting to ensure the
cache is actually populated and restored.
- Around line 252-256: The workflow step named "Symlink expected Drupal root for
BrowserKitFactoryTest" assumes /var/www/html exists and that ln -sfn will
replace any preexisting build path; update that step to (1) ensure the parent
exists with mkdir -p /var/www/html, (2) check and remove an existing
/var/www/html/build whether it's a file, symlink or directory (use a safe rm -rf
on that target only), (3) create the symlink to
${GITHUB_WORKSPACE}/drupalextension, and (4) avoid unnecessary sudo by checking
writability before using sudo; also add a comment pointing out
BrowserKitFactoryTest’s hard-coded path and suggest making it env-overridable to
avoid CI coupling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+14
to
+18
| # DrupalExtension repo/ref used by the 'extension' job to verify that the | ||
| # downstream consumer still works against this DrupalDriver HEAD. Override | ||
| # DRUPALEXTENSION_REF to target a branch with pending API compatibility changes. | ||
| DRUPALEXTENSION_REPO: jhedstrom/drupalextension | ||
| DRUPALEXTENSION_REF: feature/drupal-driver-3x |
Contributor
There was a problem hiding this comment.
Default DRUPALEXTENSION_REF points to a temporary feature branch — plan a follow-up to flip back to main.
A couple of related concerns here:
- Inconsistency vs. PR summary. The PR summary states the default was "changed to
main", but the committed default isfeature/drupal-driver-3x. The later commit ("Made the DrupalExtension smoke check blocking for a targeted sister branch") explains the intent, but the summary and code now disagree. - Coupling to an ephemeral branch. Combined with the job no longer being
continue-on-error, every PR and push tomasterwill now hard-fail ifjhedstrom/drupalextension#feature/drupal-driver-3xis renamed, deleted, or force-pushed to an incompatible state after the upstream 3.x work lands. Consider one of:- Track this as an explicit TODO (with an issue link) to revert the default to
mainonce the sister branch is merged upstream, and/or - Re-enable
continue-on-error: trueon theextensionjob until the default is back onmain, so downstream churn in DrupalExtension can't block DrupalDriver CI.
- Track this as an explicit TODO (with an issue link) to revert the default to
At minimum, please expand the comment on lines 14-16 to call out that the default intentionally points at an in-flight branch and must be flipped back to main once feature/drupal-driver-3x is merged — otherwise this easily rots silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 14 - 18, Update the inline comment
above DRUPALEXTENSION_REPO/DRUPALEXTENSION_REF to explicitly state that the
default DRUPALEXTENSION_REF currently points to the temporary sister branch
"feature/drupal-driver-3x" and must be reverted to "main" after that branch is
merged; add a TODO with an issue or PR link to track flipping it back and/or
mention re-enabling continue-on-error for the "extension" job until the revert
is completed so CI does not silently break if the ephemeral branch is renamed or
removed (refer to DRUPALEXTENSION_REF and the "extension" job in the workflow).
Collaborator
Author
There was a problem hiding this comment.
This is pre-existing infrastructure from an earlier commit on this branch (unrelated to the field classifier work this PR now focuses on); tracked separately. The default target of feature/drupal-driver-3x will flip back to main once the sister branch is merged upstream.
Contributor
There was a problem hiding this comment.
@AlexSkrypnyk, understood — thanks for the context. Acknowledged that this is pre-existing infrastructure tied to the in-flight sister branch and will be reverted to main once feature/drupal-driver-3x merges upstream. No action needed here.
🧠 Learnings used
Learnt from: AlexSkrypnyk
Repo: jhedstrom/DrupalDriver PR: 334
File: .github/workflows/ci.yml:15-46
Timestamp: 2026-04-17T06:55:20.605Z
Learning: For CI workflows, if the linting tooling (e.g., phpcs/rector) is insensitive to the runtime PHP version, configure the `lint` job to run once (not once per PHP matrix entry), mirroring the Drupal Extension CI approach. Keep the test job running across all PHP versions in the matrix (e.g., PHPUnit per PHP) to still catch real runtime regressions.
AlexSkrypnyk
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.php (1)
72-79: Reflection assumesfieldInfo/fieldConfiglive on the parent class.
$reflection->getParentClass()->getProperty(...)will fatal if these properties are ever moved ontoDefaultHandleritself or pulled up a further ancestor. Since they're guaranteed to exist on the handler instance regardless of declaring class,ReflectionPropertyresolved against the concrete class (or$reflection->getProperty(...)after confirming inheritance) would be more robust. Optional — current test passes givenAbstractHandlerowns them.Possible tightening
- $info_prop = $reflection->getParentClass()->getProperty('fieldInfo'); - $info_prop->setValue($handler, $storage); - $config_prop = $reflection->getParentClass()->getProperty('fieldConfig'); - $config_prop->setValue($handler, $config); + (new \ReflectionProperty(DefaultHandler::class, 'fieldInfo'))->setValue($handler, $storage); + (new \ReflectionProperty(DefaultHandler::class, 'fieldConfig'))->setValue($handler, $config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.php` around lines 72 - 79, The test currently assumes fieldInfo/fieldConfig are declared on DefaultHandler's parent by calling $reflection->getParentClass()->getProperty(...), which will fail if the properties move; update the resolution to target the concrete handler class first (use $reflection->hasProperty('fieldInfo') ? $reflection->getProperty('fieldInfo') : $reflection->getParentClass()->getProperty('fieldInfo') and the same for 'fieldConfig') so the ReflectionProperty is resolved against DefaultHandler when possible and falls back to the parent only if needed; keep using the same $info_prop->setValue($handler, $storage) and $config_prop->setValue($handler, $config) calls after resolving the properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/Driver/Core/Core.php`:
- Around line 832-849: When a $bundle is provided, filter configurable fields to
the resolved bundle so we don't return configurable fields attached only to
other bundles; in getEntityFieldTypes, change the $is_configurable check to
require that the field is both classified configurable
(classifier()->fieldIsConfigurable($entity_type, $field_name)) and actually
present in the bundle's field definitions (use
$entity_field_manager->getFieldDefinitions($entity_type, $bundle) and check
isset(...) for $field_name) when $bundle !== NULL, while keeping the existing
bundle-backed logic using classifier()->fieldIsBundleStorageBacked as-is.
In `@src/Drupal/Driver/Core/Field/DefaultHandler.php`:
- Around line 23-31: The exception message in DefaultHandler (the throw in
DefaultHandler.php) uses $this->fieldConfig->getTargetBundle() ??
$this->fieldInfo->getTargetEntityTypeId() which prints the entity type as a
misleading "bundle" when getTargetBundle() is NULL; change the logic used in the
sprintf argument to use a clear sentinel (e.g. "(none)") or omit the bundle
segment when getTargetBundle() === NULL so the message shows either the real
bundle name or a literal "(none)" instead of the entity type id—update the
expression that provides the bundle value for the sprintf in the throw to
reference $this->fieldConfig->getTargetBundle() and substitute "(none)" when it
is null.
In `@UPGRADING.md`:
- Around line 44-53: The migration docs currently instruct callers to use
$core->classifier()->fieldIsConfigurable(...) / fieldIsBaseStandard(...), but
classifier() is protected; either make classifier() public and add it to
CoreInterface so external code can call
$core->classifier()->fieldIsConfigurable(...) (update Core::classifier()
visibility and CoreInterface signature), or change the docs to show the
supported seam: instantiate FieldClassifier with the entity field manager (new
FieldClassifier($entityFieldManager)) and call ->fieldIsConfigurable(...) /
->fieldIsBaseStandard(...) instead of calling the protected $core->classifier();
pick one approach and update the docs and interface/visibility accordingly.
---
Nitpick comments:
In `@tests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.php`:
- Around line 72-79: The test currently assumes fieldInfo/fieldConfig are
declared on DefaultHandler's parent by calling
$reflection->getParentClass()->getProperty(...), which will fail if the
properties move; update the resolution to target the concrete handler class
first (use $reflection->hasProperty('fieldInfo') ?
$reflection->getProperty('fieldInfo') :
$reflection->getParentClass()->getProperty('fieldInfo') and the same for
'fieldConfig') so the ReflectionProperty is resolved against DefaultHandler when
possible and falls back to the parent only if needed; keep using the same
$info_prop->setValue($handler, $storage) and $config_prop->setValue($handler,
$config) calls after resolving the properties.
🪄 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: d9b613bc-0c9c-4498-857f-1d48713e0349
📒 Files selected for processing (20)
UPGRADING.mdsrc/Drupal/Driver/Capability/FieldCapabilityInterface.phpsrc/Drupal/Driver/Core/Core.phpsrc/Drupal/Driver/Core/CoreInterface.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/Core/Field/EntityReferenceRevisionsHandler.phpsrc/Drupal/Driver/Core/Field/FieldClassifier.phpsrc/Drupal/Driver/Core/Field/FieldClassifierInterface.phpsrc/Drupal/Driver/Core/Field/README.mdsrc/Drupal/Driver/DrupalDriver.phpsrc/Drupal/Driver/DrupalDriverInterface.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityCreateModerationStateKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.phptests/Drupal/Tests/Driver/Unit/BlackboxDriverTest.phptests/Drupal/Tests/Driver/Unit/Core/CoreFieldHandlerLookupTest.phptests/Drupal/Tests/Driver/Unit/Core/CoreFieldMethodsTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldClassifierTest.phptests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.phptests/Drupal/Tests/Driver/Unit/DrupalDriverTest.php
💤 Files with no reviewable changes (6)
- tests/Drupal/Tests/Driver/Unit/BlackboxDriverTest.php
- src/Drupal/Driver/DrupalDriverInterface.php
- src/Drupal/Driver/DrupalDriver.php
- tests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php
- tests/Drupal/Tests/Driver/Unit/DrupalDriverTest.php
- src/Drupal/Driver/Capability/FieldCapabilityInterface.php
✅ Files skipped from review due to trivial changes (1)
- src/Drupal/Driver/Core/Field/README.md
…dClassifier' coverage.
…assifier()' visibility.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.php (1)
135-144: Use an entity mock for the non-revisionable load result.
EntityTypeInterfacerepresents type metadata, not an entity returned by storage. At line 71, the handler calls$target instanceof RevisionableInterfaceand conditionally callsgetRevisionId()on the loaded entity. UsingEntityTypeInterfaceas the test double for non-revisionable targets is semantically incorrect—it should beEntityInterface.♻️ Proposed test-double fix
use Drupal\Core\DependencyInjection\ContainerBuilder; +use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Entity\EntityTypeInterface;else { - $target = $this->createMock(EntityTypeInterface::class); + $target = $this->createMock(EntityInterface::class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.php` around lines 135 - 144, The test uses EntityTypeInterface as the non-revisionable load result but the handler expects an entity instance and checks "instanceof RevisionableInterface"; replace the non-revisionable mock with an EntityInterface mock instead. In the setup where $revisionable is false, create a mock of EntityInterface (not EntityTypeInterface) and return that from the entity storage mock used by entity_storage->method('load') so the instanceof checks and potential getRevisionId() calls behave correctly; keep the revisionable branch unchanged where you mock RevisionableInterface and its getRevisionId().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.php`:
- Around line 135-144: The test uses EntityTypeInterface as the non-revisionable
load result but the handler expects an entity instance and checks "instanceof
RevisionableInterface"; replace the non-revisionable mock with an
EntityInterface mock instead. In the setup where $revisionable is false, create
a mock of EntityInterface (not EntityTypeInterface) and return that from the
entity storage mock used by entity_storage->method('load') so the instanceof
checks and potential getRevisionId() calls behave correctly; keep the
revisionable branch unchanged where you mock RevisionableInterface and its
getRevisionId().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bda3228b-e3f1-4fa7-86fe-f6cb001e7f09
📒 Files selected for processing (7)
src/Drupal/Driver/Core/Core.phpsrc/Drupal/Driver/Core/CoreInterface.phpsrc/Drupal/Driver/Core/Field/DefaultHandler.phpsrc/Drupal/Driver/DrupalDriver.phpsrc/Drupal/Driver/DrupalDriverInterface.phptests/Drupal/Tests/Driver/Unit/Core/Field/EntityReferenceRevisionsHandlerTest.phptests/Drupal/Tests/Driver/Unit/Core/Field/FieldClassifierTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Drupal/Driver/Core/Field/DefaultHandler.php
- src/Drupal/Driver/DrupalDriverInterface.php
AlexSkrypnyk
force-pushed
the
feature/ext-smoke
branch
from
14b5f2d to
37f3b14
Compare
… and added type-coverage safety net.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
src/Drupal/Driver/Core/Field/TextHandler.php (1)
14-23: LGTM.Mirrors
TextLongHandlerfor the one-linetextcounterpart; the(value, format)shape is identical totext_long, so a pass-through is the right choice and avoids theDefaultHandlerloud-fail path.Given the two classes are byte-identical except for the class name and docblock, you could factor out a shared
AbstractPassthroughHandler(or makeTextLongHandlerextendTextHandler) to avoid drift if the passthrough contract evolves. Not required — chill-mode suggestion only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Core/Field/TextHandler.php` around lines 14 - 23, The two handlers TextHandler and TextLongHandler are identical pass-throughs; refactor by extracting a shared AbstractPassthroughHandler (or have TextLongHandler extend TextHandler) that implements FieldHandlerInterface with the identical expand(mixed $values): array { return (array) $values; } behavior so both TextHandler and TextLongHandler delegate to that single implementation to avoid future drift.tests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.php (1)
20-57: LGTM — optional: share the FilterFormat bootstrap withTextHandlerKernelTest.
TextLongHandlerKernelTestandTextHandlerKernelTestshare the same$modulesadditions (text,filter) and the samesetUp()that creates aplain_textFilterFormat. Consider extracting an intermediate abstract base (e.g.,TextualFieldHandlerKernelTestBase extends FieldHandlerKernelTestBase) that owns both, so future textual handlers (e.g.,text_with_summary) only add theirattachField()call. Not required for this PR.Same verification note as on
TextHandlerKernelTest: confirmFilterFormat::save()in this kernel-test base doesn't require an explicitinstallConfig(['filter'])in your environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.php` around lines 20 - 57, Extract the shared bootstrap from TextLongHandlerKernelTest and TextHandlerKernelTest into a new abstract base class (e.g., TextualFieldHandlerKernelTestBase extends FieldHandlerKernelTestBase) that defines the extra protected static $modules = [...self::BASE_MODULES, 'text', 'filter'] and the setUp() method which calls parent::setUp() and creates the plain_text FilterFormat via FilterFormat::create([...])->save(); then have both TextLongHandlerKernelTest and TextHandlerKernelTest extend this new base and keep only their attachField(...) and test methods (so future textual handlers like text_with_summary can just call attachField()), and ensure FilterFormat::create(...)->save() remains in the base (verify if installConfig(['filter']) is needed in your test env).tests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.php (1)
22-30: LGTM — optional: expand coverage to non-list inputs.The happy-path passthrough is covered. Since
expand()relies on(array) $values, you might optionally add a case for a non-list payload (e.g.,['value' => 'x', 'format' => 'plain_text']without the outer list) and/or a scalarstringto lock in the cast behavior thatDefaultHandler's loud-fail policy now depends upstream callers to provide correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.php` around lines 22 - 30, Add additional unit cases to TextLongHandlerTest to verify expand() behavior with non-list inputs: create tests calling TextLongHandler::expand with a single associative array (e.g., ['value'=>'x','format'=>'plain_text']) and with a scalar string to confirm the (array) cast behavior; assert the returned value matches the expected casted/list form and that no exceptions are thrown (or that behavior matches DefaultHandler expectations). Reference the TextLongHandler::expand method and the test class TextLongHandlerTest when adding these new assertions.src/Drupal/Driver/DrupalDriverInterface.php (1)
42-45: Nit: thin docblock forgetCore().The one-liner "Return current core." omits a
@returndescription and contrasts with the much richersetCore()docblock immediately below. Consider a brief note on what the returned instance represents (e.g., "The active Core implementation the driver delegates to.") for consistency and IDE help.Proposed tweak
- /** - * Return current core. - */ - public function getCore(): CoreInterface; + /** + * Returns the active Core implementation. + * + * `@return` \Drupal\Driver\Core\CoreInterface + * The Core instance the driver delegates to. + */ + public function getCore(): CoreInterface;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/DrupalDriverInterface.php` around lines 42 - 45, Update the docblock for the getCore() method to include a `@return` tag and a short description of the returned instance (for example: "The active Core implementation the driver delegates to."), matching the richer style used by setCore(); reference the getCore() method and CoreInterface in the comment so IDEs and readers know what type and role are returned.tests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php (2)
186-209: Stale docblock: helper no longer "injects private properties directly via reflection" for core.Lines 189–191 still describe the old behaviour, but
$coreis now set through the publicsetCore()API at line 206 (onlydrupalRoot,uri, andversiongo through reflection). Update the docblock for accuracy so future readers don't add a second code path back.Proposed tweak
- /** - * Creates a 'DrupalDriver' with an injected core and a fixed version. - * - * Bypasses the constructor (which requires a real Drupal installation) and - * sets the private properties directly via reflection. - */ + /** + * Creates a 'DrupalDriver' with an injected core and a fixed version. + * + * Bypasses the constructor (which requires a real Drupal installation), + * sets the internal scaffolding ('drupalRoot', 'uri', 'version') via + * reflection, and injects the core through the public 'setCore()' API. + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php` around lines 186 - 209, Update the method docblock for createDriverWithCore to accurately state that the function bypasses the constructor only for drupalRoot, uri and version (which are set via reflection), while the CoreInterface $core is assigned via the public setCore() method; mention that the method creates a DrupalDriver instance without invoking the constructor and sets drupalRoot, uri and version by reflection before calling setCore($core).
87-94:testSetCoreAssignsInjectedInstanceindirectly depends on helper callingsetCore().
createDriverWithCore()already invokessetCore()internally (line 206), so the test at lines 87–94 exercises a secondsetCore()call rather than a first assignment. If the helper is ever refactored to inject core via reflection, this test would still pass for the wrong reason and fail to catch a regression insetCore()'s wiring.Optional: bypass the helper here and build a driver without a pre-set core, then assert that
setCore()alone establishes the binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php` around lines 87 - 94, testSetCoreAssignsInjectedInstance currently relies on createDriverWithCore() which already calls setCore(), so update the test to instantiate a driver that does NOT have a core pre-set (e.g. use a constructor helper that doesn't call setCore() or instantiate the driver directly) and then call setCore($custom) and assertSame($custom, $driver->getCore()); ensure you remove or stop using createDriverWithCore() in this test so it verifies setCore() wiring directly (references: testSetCoreAssignsInjectedInstance, createDriverWithCore, setCore, getCore).tests/Drupal/Tests/Driver/Unit/CoreLookupTest.php (1)
78-79: LGTM — reflection-based injection matches sibling patterns.Using
getProperty('version')->setValue(...)is consistent with howdrupalRootanduriare already injected above, and the relevant context snippet confirms$versionisprotected int, so reflection is necessary.Minor nit (optional): the method docblock at lines 58–59 still says "injects the required private properties directly," but the driver's
$version/$core/etc. areprotected, not private. Consider updating the wording for accuracy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/CoreLookupTest.php` around lines 78 - 79, The docblock for the test method currently says "injects the required private properties directly" but the driver properties like $version/$core are protected, not private; update the method docblock wording (around the method that uses reflection to set drupalRoot/uri/version) to say "injects the required protected properties directly" or more generally "injects required properties directly" to accurately reflect the visibility of the fields ($version, $core, drupalRoot, uri) being set via reflection in CoreLookupTest.tests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.php (1)
112-262: Optional: lift the shared stub helpers into a trait or base class.
createFakeFile,createFileRepositoryReturning,createEntityTypeManagerReturningNoMatches, andcreateEntityTypeManagerReturningFileAtUriare duplicated essentially verbatim withFileHandlerTest.php(≈150 lines). Extracting them into aFileHandlerStubsTrait(or an abstract parent likeAbstractFileFieldHandlerUnitTestBase) would keep the two suites in lockstep and avoid drift when the underlying services evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.php` around lines 112 - 262, The test helpers (createFakeFile, createFileRepositoryReturning, createEntityTypeManagerReturningNoMatches, createEntityTypeManagerReturningFileAtUri) are duplicated across ImageHandlerTest and FileHandlerTest; extract them into a shared trait (e.g. FileHandlerStubsTrait) or an abstract test base and have both test classes use that trait or extend the base. Move the four private methods into the trait as protected methods, update ImageHandlerTest and FileHandlerTest to remove the duplicated methods and use the trait (or extend the new base) so both tests reference the single shared implementations.tests/Drupal/Tests/Driver/Kernel/Core/Field/FileHandlerReuseKernelTest.php (1)
104-127: Optional: liftcreateManagedFileAt()andfileEntityCount()intoFieldHandlerKernelTestBase(or aManagedFileKernelTrait).These two helpers are byte-identical with
ImageHandlerReuseKernelTest.php(L109-132), and thesetUp()bodies for thefile/file_usage/file_public_pathwiring are near-identical as well. Moving them into a shared location would keep both reuse suites from drifting as the managed-file contract evolves (e.g., iffile_usagebecomes unnecessary or a new setting has to be primed).♻️ Sketch of the shared trait
trait ManagedFileKernelTrait { protected function setUpManagedFileStorage(): void { $this->installEntitySchema('file'); $this->installSchema('file', ['file_usage']); $public_path = $this->siteDirectory . '/files'; if (!is_dir($public_path)) { mkdir($public_path, 0777, TRUE); } $this->setSetting('file_public_path', $public_path); } protected function createManagedFileAt(string $uri, string $contents): File { file_put_contents($uri, $contents); $file = File::create(['uri' => $uri, 'filename' => basename($uri), 'status' => 1]); $file->save(); return $file; } protected function fileEntityCount(): int { return (int) \Drupal::entityTypeManager()->getStorage('file')->getQuery() ->accessCheck(FALSE)->count()->execute(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/FileHandlerReuseKernelTest.php` around lines 104 - 127, The two helper methods createManagedFileAt() and fileEntityCount() are duplicated across FileHandlerReuseKernelTest and ImageHandlerReuseKernelTest; extract them (and the file setup wiring) into a shared location such as a ManagedFileKernelTrait or into FieldHandlerKernelTestBase: move the createManagedFileAt() and fileEntityCount() implementations into the new trait/class, update both tests to use the trait or inherit the base class, and relocate the file storage setup (installEntitySchema('file'), installSchema('file', ['file_usage']), creating $this->siteDirectory . '/files' and setSetting('file_public_path', ...)) into a shared setUpManagedFileStorage() method so both suites call it instead of duplicating the same code.tests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.php (1)
40-73: Consider noting that some SKIP entries are inert until their provider module is enabled.
password(user),uuid(base-field / core), andshape*(entity_test) only appear ingetDefinitions()when their providing module is loaded. With the current$moduleslist these SKIP keys never match, which is fine as forward-compat insurance but can mislead a future contributor into deleting a "dead" entry. A short comment on theSKIPconstant documenting the intent (or a sanity assertion that each SKIP key corresponds to a plugin id that would appear under some well-defined module set) would prevent that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.php` around lines 40 - 73, The SKIP constant contains entries for field types (e.g., 'password', 'uuid', 'shape', 'shape_required') that only exist when their provider modules are enabled; add a brief clarifying comment above private const SKIP explaining these entries are intentionally kept as forward-compat guards even if the current protected static $modules list doesn’t load their providers, or alternatively add a small sanity check in the test setup (referencing SKIP and getDefinitions()) that asserts any SKIP key either appears in the current definitions or documents why it is inert to prevent future accidental deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/Driver/DrupalDriverInterface.php`:
- Around line 61-72: The `@throws` BootstrapException tag on the
getDrupalVersion() interface method is incorrect because the exception is raised
during object construction (detectMajorVersion() called in the constructor), not
by the getter; remove the `@throws` annotation from the getDrupalVersion()
declaration in DrupalDriverInterface (or replace it with a clarifying comment
that initialization may throw BootstrapException) and keep exception handling
where detectMajorVersion()/the constructor may throw BootstrapException so
callers know to catch it during instantiation rather than when calling
getDrupalVersion().
---
Nitpick comments:
In `@src/Drupal/Driver/Core/Field/TextHandler.php`:
- Around line 14-23: The two handlers TextHandler and TextLongHandler are
identical pass-throughs; refactor by extracting a shared
AbstractPassthroughHandler (or have TextLongHandler extend TextHandler) that
implements FieldHandlerInterface with the identical expand(mixed $values): array
{ return (array) $values; } behavior so both TextHandler and TextLongHandler
delegate to that single implementation to avoid future drift.
In `@src/Drupal/Driver/DrupalDriverInterface.php`:
- Around line 42-45: Update the docblock for the getCore() method to include a
`@return` tag and a short description of the returned instance (for example: "The
active Core implementation the driver delegates to."), matching the richer style
used by setCore(); reference the getCore() method and CoreInterface in the
comment so IDEs and readers know what type and role are returned.
In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.php`:
- Around line 40-73: The SKIP constant contains entries for field types (e.g.,
'password', 'uuid', 'shape', 'shape_required') that only exist when their
provider modules are enabled; add a brief clarifying comment above private const
SKIP explaining these entries are intentionally kept as forward-compat guards
even if the current protected static $modules list doesn’t load their providers,
or alternatively add a small sanity check in the test setup (referencing SKIP
and getDefinitions()) that asserts any SKIP key either appears in the current
definitions or documents why it is inert to prevent future accidental deletion.
In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/FileHandlerReuseKernelTest.php`:
- Around line 104-127: The two helper methods createManagedFileAt() and
fileEntityCount() are duplicated across FileHandlerReuseKernelTest and
ImageHandlerReuseKernelTest; extract them (and the file setup wiring) into a
shared location such as a ManagedFileKernelTrait or into
FieldHandlerKernelTestBase: move the createManagedFileAt() and fileEntityCount()
implementations into the new trait/class, update both tests to use the trait or
inherit the base class, and relocate the file storage setup
(installEntitySchema('file'), installSchema('file', ['file_usage']), creating
$this->siteDirectory . '/files' and setSetting('file_public_path', ...)) into a
shared setUpManagedFileStorage() method so both suites call it instead of
duplicating the same code.
In `@tests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.php`:
- Around line 20-57: Extract the shared bootstrap from TextLongHandlerKernelTest
and TextHandlerKernelTest into a new abstract base class (e.g.,
TextualFieldHandlerKernelTestBase extends FieldHandlerKernelTestBase) that
defines the extra protected static $modules = [...self::BASE_MODULES, 'text',
'filter'] and the setUp() method which calls parent::setUp() and creates the
plain_text FilterFormat via FilterFormat::create([...])->save(); then have both
TextLongHandlerKernelTest and TextHandlerKernelTest extend this new base and
keep only their attachField(...) and test methods (so future textual handlers
like text_with_summary can just call attachField()), and ensure
FilterFormat::create(...)->save() remains in the base (verify if
installConfig(['filter']) is needed in your test env).
In `@tests/Drupal/Tests/Driver/Unit/Core/Field/ImageHandlerTest.php`:
- Around line 112-262: The test helpers (createFakeFile,
createFileRepositoryReturning, createEntityTypeManagerReturningNoMatches,
createEntityTypeManagerReturningFileAtUri) are duplicated across
ImageHandlerTest and FileHandlerTest; extract them into a shared trait (e.g.
FileHandlerStubsTrait) or an abstract test base and have both test classes use
that trait or extend the base. Move the four private methods into the trait as
protected methods, update ImageHandlerTest and FileHandlerTest to remove the
duplicated methods and use the trait (or extend the new base) so both tests
reference the single shared implementations.
In `@tests/Drupal/Tests/Driver/Unit/Core/Field/TextLongHandlerTest.php`:
- Around line 22-30: Add additional unit cases to TextLongHandlerTest to verify
expand() behavior with non-list inputs: create tests calling
TextLongHandler::expand with a single associative array (e.g.,
['value'=>'x','format'=>'plain_text']) and with a scalar string to confirm the
(array) cast behavior; assert the returned value matches the expected
casted/list form and that no exceptions are thrown (or that behavior matches
DefaultHandler expectations). Reference the TextLongHandler::expand method and
the test class TextLongHandlerTest when adding these new assertions.
In `@tests/Drupal/Tests/Driver/Unit/CoreLookupTest.php`:
- Around line 78-79: The docblock for the test method currently says "injects
the required private properties directly" but the driver properties like
$version/$core are protected, not private; update the method docblock wording
(around the method that uses reflection to set drupalRoot/uri/version) to say
"injects the required protected properties directly" or more generally "injects
required properties directly" to accurately reflect the visibility of the fields
($version, $core, drupalRoot, uri) being set via reflection in CoreLookupTest.
In `@tests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php`:
- Around line 186-209: Update the method docblock for createDriverWithCore to
accurately state that the function bypasses the constructor only for drupalRoot,
uri and version (which are set via reflection), while the CoreInterface $core is
assigned via the public setCore() method; mention that the method creates a
DrupalDriver instance without invoking the constructor and sets drupalRoot, uri
and version by reflection before calling setCore($core).
- Around line 87-94: testSetCoreAssignsInjectedInstance currently relies on
createDriverWithCore() which already calls setCore(), so update the test to
instantiate a driver that does NOT have a core pre-set (e.g. use a constructor
helper that doesn't call setCore() or instantiate the driver directly) and then
call setCore($custom) and assertSame($custom, $driver->getCore()); ensure you
remove or stop using createDriverWithCore() in this test so it verifies
setCore() wiring directly (references: testSetCoreAssignsInjectedInstance,
createDriverWithCore, setCore, getCore).
🪄 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: b0309f4b-03cc-479b-902b-38973e5723d5
📒 Files selected for processing (18)
src/Drupal/Driver/Core/Field/FileHandler.phpsrc/Drupal/Driver/Core/Field/ImageHandler.phpsrc/Drupal/Driver/Core/Field/README.mdsrc/Drupal/Driver/Core/Field/TextHandler.phpsrc/Drupal/Driver/Core/Field/TextLongHandler.phpsrc/Drupal/Driver/DrupalDriver.phpsrc/Drupal/Driver/DrupalDriverInterface.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FieldTypeCoverageKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/FileHandlerReuseKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/ImageHandlerReuseKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/TextHandlerKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/Field/TextLongHandlerKernelTest.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/CoreLookupTest.phptests/Drupal/Tests/Driver/Unit/DrupalDriverDelegationTest.php
✅ Files skipped from review due to trivial changes (1)
- src/Drupal/Driver/Core/Field/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Drupal/Driver/DrupalDriver.php
AlexSkrypnyk
changed the title
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
Introduces
FieldClassifier- a dedicated class that classifies Drupal fields into nine mutually exclusive F-row categories (F1-F9) based on a documented truth table. TheCore::expandEntityFields()andCore::getEntityFieldTypes()methods are rewritten to consult the classifier instead of ad-hoc boolean predicates, fixing a correctness bug with computed writable base fields likemoderation_state. The oldfieldExists()/fieldIsBase()predicates are removed fromDrupalDriverandCore, the emptyFieldCapabilityInterfaceis deleted, andDefaultHandlernow throws loudly when invoked on non-H1 fields (multi-column or non-value-keyed) rather than silently producing wrong data.A new
EntityReferenceRevisionsHandleris added for theentity_reference_revisionsfield type, a regression kernel test covers themoderation_stateF3 path end-to-end, and a full unit test suite for the classifier covers all nine F-rows.UPGRADING.mdis updated with migration guidance for consumers of the removed predicates.Changes
New:
FieldClassifierclassification systemFieldClassifierInterface- nine predicate methods (fieldIsBaseStandard,fieldIsBaseComputedReadOnly,fieldIsBaseComputedWritable,fieldIsBaseCustomStorage,fieldIsConfigurable,fieldIsConfigurableMultiColumn,fieldIsId,fieldIsBundle,fieldIsUnknown), one per F-row of the truth table.FieldClassifier- concrete implementation; all classification logic lives here, isolated fromCore.src/Drupal/Driver/Core/Field/README.md- full truth table, handler-selection sub-table, and examples per F-row.Rewritten:
Coreexpansion pipelineCore::expandEntityFields()andCore::getEntityFieldTypes()now delegate classification toFieldClassifierinstead of ad-hoc predicates.expandEntityFields()to prevent the bundle key from entering the handler pipeline - behavior previously buried indetectBaseFieldsOnEntity().Core::classifier()accessor exposes the classifier to consumers and tests.Removed
FieldCapabilityInterface- was empty; deleted entirely.DrupalDriver::fieldExists()andDrupalDriver::fieldIsBase()- replaced by classifier predicates.CoreInterface::fieldExists()andCoreInterface::fieldIsBase().Updated:
DefaultHandlerloud-fail\RuntimeExceptionwhen invoked on a field whose property is notvalueor that has more than one column, rather than silently returning wrong data.New:
EntityReferenceRevisionsHandlerentity_reference_revisionsfield type, resolving entity references by target ID and revision ID.New: kernel regression test
CoreEntityCreateModerationStateKernelTest- round-trip test throughCore::entityCreate()for a node withmoderation_state(F3: computed writable base field), verifying the classifier routes it correctly.New: unit tests
FieldClassifierTest- nine test methods, one per F-row predicate, covering all classification paths.DefaultHandlerTestextended with loud-fail assertion for non-H1 fields.CI: DrupalExtension smoke job
extensionjob in.github/workflows/ci.ymlinstalls DrupalExtension with this DrupalDriver HEAD via a Composer path repository and runs its test suite, acting as a downstream compatibility gate.Docs
UPGRADING.md- removesfieldExists/fieldIsBasefrom the renamed-methods table, adds "Field classification moved to FieldClassifier" section with migration examples, updates the DrushDriver section.Before / After
Field classification (Core expansion pipeline)
DefaultHandler invocation guard
Removed public API surface
Summary by CodeRabbit