Added PHPUnit tests for D8+ field handlers and exceptions.#331
Merged
AlexSkrypnyk merged 2 commits intomasterfrom Apr 17, 2026
Merged
Added PHPUnit tests for D8+ field handlers and exceptions.#331AlexSkrypnyk merged 2 commits intomasterfrom
AlexSkrypnyk merged 2 commits intomasterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds comprehensive test coverage for Drupal field handlers and driver exception handling, along with updates to CI configuration. The changes include 13 new PHPUnit test classes covering field handler expansion logic and exception behavior, plus CI coverage threshold adjustment from 10% to 35% and test coverage excludes for specific Drupal driver subdirectories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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)
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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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 (7)
tests/Drupal/Tests/Driver/BlackboxDriverTest.php (1)
70-102: Solid coverage of all 26 unsupported methods. Two small notes.
- The PR description mentions 26 methods; the provider lists 26 entries, but the
BaseDriversnippet in context shows 26 throwing methods too — good. If you want, add one negative-control key per group (e.g. one non-existent method) to prove the provider keys aren't silently swallowed — optional.$message_fragmentis matched viaexpectExceptionMessageMatches('/' . preg_quote(...) . '/').expectExceptionMessage($message_fragment)would be simpler and equivalent (it already does substring matching), avoiding the regex round-trip. Purely stylistic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/BlackboxDriverTest.php` around lines 70 - 102, The data provider dataProviderUnsupportedActionsThrow returns 26 unsupported-action cases which is fine; change the test assertion that currently uses expectExceptionMessageMatches('/' . preg_quote($message_fragment) . '/') to use expectExceptionMessage($message_fragment) instead (expectExceptionMessage does substring matching and avoids the regex); optionally add a single negative-control entry (e.g. a non-existent method key) to dataProviderUnsupportedActionsThrow to assert provider keys aren't ignored, but this is optional.phpunit.xml.dist (1)
19-26: Remove non-existent directory excludes and add a comment explaining why D6/D7 is excluded.The
<directory>excludes forDrupal6andDrupal7undersrc/Drupal/Driver/Coresdon't exist; only the.phpfile excludes in that section are effective. However, the<directory>excludes forsrc/Drupal/Driver/Fields/Drupal6andDrupal7are valid and necessary. Remove the non-existent Cores directory excludes to avoid confusion. Also add a brief comment (e.g., "Legacy D6/D7 is out of scope for v2.x") to clarify the rationale—it will save the next maintainer a git-blame.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpunit.xml.dist` around lines 19 - 26, Remove the two non-existent <directory> excludes for src/Drupal/Driver/Cores/Drupal6 and src/Drupal/Driver/Cores/Drupal7 from phpunit.xml.dist (leave the existing <file> excludes for Drupal6.php and Drupal7.php and keep the valid src/Drupal/Driver/Fields/Drupal6 and Fields/Drupal7 directory excludes), and add a short XML comment in the exclude block (e.g., "Legacy D6/D7 is out of scope for v2.x") explaining why D6/D7 are excluded to aid future maintainers.tests/Drupal/Tests/Driver/Fields/Drupal8/EntityReferenceHandlerTest.php (2)
114-125: Consider mockingEntityStorageInterfaceinstead of an anonymous class.The anonymous storage exposes only
getQuery(). IfEntityReferenceHandler::expand()ever calls another storage method before the "no matching entity" throw (e.g.loadByProperties,load), the test will fail with a fatal "undefined method" rather than the intended assertion. Using$this->createMock(EntityStorageInterface::class)would gracefully return null/empty for any unstubbed call and keep the test resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/EntityReferenceHandlerTest.php` around lines 114 - 125, Replace the anonymous storage class with a PHPUnit mock of EntityStorageInterface so unstubbed calls won't error; create the mock via $this->createMock(EntityStorageInterface::class) and stub its getQuery() to return the $query used in the test, ensuring any other methods (e.g. loadByProperties, load) will safely return null/empty if called; this keeps the EntityReferenceHandler::expand() test resilient and avoids fatal "undefined method" errors from the anonymous class.
100-134: Drop the unused$entity_type_idparameter.PHPMD flags it and the method body doesn't use it — the storage/definition mocks return the same values regardless. Either wire it into a
with($entity_type_id)constraint ongetStorage()/getDefinition()(tighter test), or remove it to silence the warning.♻️ Minimal change
- protected function setUpEmptyQueryContainer($entity_type_id) { + protected function setUpEmptyQueryContainer() {and update the call site in
testExpandThrowsWhenNoEntityMatches().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/EntityReferenceHandlerTest.php` around lines 100 - 134, The method setUpEmptyQueryContainer currently takes an unused parameter $entity_type_id; remove this parameter from the setUpEmptyQueryContainer signature and update all call sites (e.g., testExpandThrowsWhenNoEntityMatches) to call setUpEmptyQueryContainer() with no arguments; keep the existing mocks for EntityTypeManagerInterface::getDefinition and getStorage as-is so behavior is unchanged, and run tests to ensure no other callers pass the removed argument (adjust any remaining usages accordingly).tests/Drupal/Tests/Driver/Fields/Drupal8/SupportedImageHandlerTest.php (1)
122-126: Clean up temp fixture files.
createTempFile()writes intosys_get_temp_dir()but nothing removes the files after each test. Over time on CI this leaks fixtures. Consider tracking created paths on$thisandunlink()-ing them intearDown().🧹 Suggested cleanup
+ /** + * `@var` string[] + */ + private array $tempFiles = []; + protected function tearDown(): void { + foreach ($this->tempFiles as $path) { + if (is_file($path)) { + `@unlink`($path); + } + } + $this->tempFiles = []; \Drupal::unsetContainer(); parent::tearDown(); } @@ protected function createTempFile($extension) { $path = tempnam(sys_get_temp_dir(), 'drupal-driver-') . '.' . $extension; file_put_contents($path, 'fixture'); + $this->tempFiles[] = $path; return $path; }Note:
tempnam()also creates a zero-byte file at its returned name (without the appended extension) which is never written to or deleted — worth also removing that original path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/SupportedImageHandlerTest.php` around lines 122 - 126, createTempFile currently creates temp files in sys_get_temp_dir() and appends an extension but never records or removes them, leaking fixtures (and leaving the zero-byte file created by tempnam); modify createTempFile to push both the tempnam() return value and the final $path into a property on $this (e.g. $this->tempFiles[]), and implement tearDown() in the test class (or extend existing tearDown) to iterate $this->tempFiles and `@unlink`() each path (clearing the array afterwards) so both the original tempnam file and the extension-appended file are removed after each test.tests/Drupal/Tests/Driver/Fields/Drupal8/ImageHandlerTest.php (1)
38-55: Factor the temp-file fixture and clean it up.
tempnam(...) . '.jpg'is duplicated across both passing tests, and the created files are not unlinked. Extract acreateTempFile()helper (like the File/SupportedImage tests) and remove the files intearDown().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/ImageHandlerTest.php` around lines 38 - 55, Add a reusable temp-file helper and cleanup: add a private property (e.g., $tempFiles = []) to the ImageHandlerTest class, implement a private createTempFile() method that creates tempnam(sys_get_temp_dir(), 'drupal-driver-') . '.jpg', writes the fixture contents and pushes the path onto $tempFiles, replace the duplicate tempnam(...) calls in testExpand... and testExpandPropagatesAltAndTitleExtras with calls to createTempFile(), and implement tearDown() to iterate $tempFiles and unlink any existing files (clearing the array afterward) so created temp files are removed after each test; keep existing helpers like setFileRepositoryWithReturnId and createHandler unchanged.tests/Drupal/Tests/Driver/Fields/Drupal8/FileHandlerTest.php (1)
83-87: Same temp-file leak asSupportedImageHandlerTest.
tempnam()creates a file at the returned name, andcreateTempFile()writes to a second path (<tempnam>.<ext>). Neither is unlinked on teardown. Consider tracking and deleting both intearDown().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/FileHandlerTest.php` around lines 83 - 87, createTempFile creates two filesystem entries (the tempnam() path and the <tempnam>.<ext> file written by file_put_contents) and neither is removed; modify createTempFile (in FileHandlerTest) to record both paths (the raw tempnam() return and the appended-with-extension path) into a test property (e.g. $this->tempFiles[]), and implement tearDown() to iterate over $this->tempFiles and `@unlink`() each path; mirror the same tracking/cleanup approach used in SupportedImageHandlerTest so both files are deleted after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/DaterangeHandlerTest.php`:
- Line 13: The hardcoded require_once for DateTimeItemInterface breaks Composer
installs; in DaterangeHandlerTest and DatetimeHandlerTest remove the direct
require_once and instead: first check
interface_exists('Drupal\\datetime\\Plugin\\Field\\FieldType\\DateTimeItemInterface'),
if missing try to resolve the path with
Composer\InstalledVersions::getInstallPath('drupal/core') and require the file
from vendor/.../modules/datetime/.../DateTimeItemInterface.php when that install
path is found, and if still not available call
$this->markTestSkipped('drupal/core datetime interface not available') so tests
fail gracefully; update references to the interface name (DateTimeItemInterface)
in both test classes to locate the change.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/DatetimeHandlerTest.php`:
- Around line 64-80: The createHandler method builds a mock using
getMockBuilder(\stdClass::class)->addMethods(['getSetting']) which uses the
deprecated addMethods API; replace it with createMock of the actual field
storage definition interface that declares getSetting (e.g.
FieldStorageConfigInterface) and set the getSetting expectation on that mock,
then keep injecting it into the DatetimeHandler instance via
ReflectionProperty('fieldInfo'); update references to
$field_info->method('getSetting')->with('datetime_type')->willReturn($datetime_type)
to use the new createMock instance so PHPUnit 10+/11+ will not emit deprecation
warnings.
In
`@tests/Drupal/Tests/Driver/Fields/Drupal8/TaxonomyTermReferenceHandlerTest.php`:
- Around line 28-30: Replace soft-deprecated addMethods() usage by mocking the
real Drupal interface instead of \stdClass: in TaxonomyTermReferenceHandlerTest
replace the $term mock built from \stdClass with a mock of the proper interface
(e.g., \Drupal\taxonomy\TermInterface) created via
createMock()/getMockBuilder(...)->onlyMethods(...), then stub the id() method on
that mock; apply the same pattern in the other tests mentioned (ListHandlerTest,
FileHandlerTest, ImageHandlerTest, SupportedImageHandlerTest,
EntityReferenceHandlerTest, AddressHandlerTest, DatetimeHandlerTest) by swapping
\stdClass mocks for the appropriate interfaces (for example FileInterface,
FieldStorageDefinitionInterface, FileRepositoryInterface) and configuring
onlyMethods/created mock methods instead of addMethods().
---
Nitpick comments:
In `@phpunit.xml.dist`:
- Around line 19-26: Remove the two non-existent <directory> excludes for
src/Drupal/Driver/Cores/Drupal6 and src/Drupal/Driver/Cores/Drupal7 from
phpunit.xml.dist (leave the existing <file> excludes for Drupal6.php and
Drupal7.php and keep the valid src/Drupal/Driver/Fields/Drupal6 and
Fields/Drupal7 directory excludes), and add a short XML comment in the exclude
block (e.g., "Legacy D6/D7 is out of scope for v2.x") explaining why D6/D7 are
excluded to aid future maintainers.
In `@tests/Drupal/Tests/Driver/BlackboxDriverTest.php`:
- Around line 70-102: The data provider dataProviderUnsupportedActionsThrow
returns 26 unsupported-action cases which is fine; change the test assertion
that currently uses expectExceptionMessageMatches('/' .
preg_quote($message_fragment) . '/') to use
expectExceptionMessage($message_fragment) instead (expectExceptionMessage does
substring matching and avoids the regex); optionally add a single
negative-control entry (e.g. a non-existent method key) to
dataProviderUnsupportedActionsThrow to assert provider keys aren't ignored, but
this is optional.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/EntityReferenceHandlerTest.php`:
- Around line 114-125: Replace the anonymous storage class with a PHPUnit mock
of EntityStorageInterface so unstubbed calls won't error; create the mock via
$this->createMock(EntityStorageInterface::class) and stub its getQuery() to
return the $query used in the test, ensuring any other methods (e.g.
loadByProperties, load) will safely return null/empty if called; this keeps the
EntityReferenceHandler::expand() test resilient and avoids fatal "undefined
method" errors from the anonymous class.
- Around line 100-134: The method setUpEmptyQueryContainer currently takes an
unused parameter $entity_type_id; remove this parameter from the
setUpEmptyQueryContainer signature and update all call sites (e.g.,
testExpandThrowsWhenNoEntityMatches) to call setUpEmptyQueryContainer() with no
arguments; keep the existing mocks for EntityTypeManagerInterface::getDefinition
and getStorage as-is so behavior is unchanged, and run tests to ensure no other
callers pass the removed argument (adjust any remaining usages accordingly).
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/FileHandlerTest.php`:
- Around line 83-87: createTempFile creates two filesystem entries (the
tempnam() path and the <tempnam>.<ext> file written by file_put_contents) and
neither is removed; modify createTempFile (in FileHandlerTest) to record both
paths (the raw tempnam() return and the appended-with-extension path) into a
test property (e.g. $this->tempFiles[]), and implement tearDown() to iterate
over $this->tempFiles and `@unlink`() each path; mirror the same tracking/cleanup
approach used in SupportedImageHandlerTest so both files are deleted after each
test.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/ImageHandlerTest.php`:
- Around line 38-55: Add a reusable temp-file helper and cleanup: add a private
property (e.g., $tempFiles = []) to the ImageHandlerTest class, implement a
private createTempFile() method that creates tempnam(sys_get_temp_dir(),
'drupal-driver-') . '.jpg', writes the fixture contents and pushes the path onto
$tempFiles, replace the duplicate tempnam(...) calls in testExpand... and
testExpandPropagatesAltAndTitleExtras with calls to createTempFile(), and
implement tearDown() to iterate $tempFiles and unlink any existing files
(clearing the array afterward) so created temp files are removed after each
test; keep existing helpers like setFileRepositoryWithReturnId and createHandler
unchanged.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/SupportedImageHandlerTest.php`:
- Around line 122-126: createTempFile currently creates temp files in
sys_get_temp_dir() and appends an extension but never records or removes them,
leaking fixtures (and leaving the zero-byte file created by tempnam); modify
createTempFile to push both the tempnam() return value and the final $path into
a property on $this (e.g. $this->tempFiles[]), and implement tearDown() in the
test class (or extend existing tearDown) to iterate $this->tempFiles and
`@unlink`() each path (clearing the array afterwards) so both the original tempnam
file and the extension-appended file are removed after each test.
🪄 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: 026f6be2-1d96-4c2d-bdad-19cfdfe70d5a
📒 Files selected for processing (15)
.github/workflows/ci.ymlphpunit.xml.disttests/Drupal/Tests/Driver/BlackboxDriverTest.phptests/Drupal/Tests/Driver/Exception/UnsupportedDriverActionExceptionTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/AddressHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/DaterangeHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/DatetimeHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/DefaultHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/EntityReferenceHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/FileHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/ImageHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/ListHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/SupportedImageHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/TaxonomyTermReferenceHandlerTest.phptests/Drupal/Tests/Driver/Fields/Drupal8/TextWithSummaryHandlerTest.php
…pped deprecated addMethods mocks.
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
Excluded D6/D7 source directories from PHPUnit coverage measurement (legacy cores are out of scope for v2.x), added unit tests for D8+ field handlers and exception classes, and bumped the CI coverage threshold from 10% to 35%. D7 regression tests (
Drupal7FieldHandlerTest) still run as before - they just do not count toward the coverage metric.The previous 10% baseline was dominated by D6/D7 code that is not being maintained. After scoping coverage to D8+ only, targeted tests pushed line coverage to 39.08% (classes: 54.17%, methods: 30.73%), well above the new 35% floor.
Changes
Coverage scope (
phpunit.xml.dist)Added
<exclude>entries under<source>to dropsrc/Drupal/Driver/Cores/Drupal6.php,Drupal7.php,Fields/Drupal6/, andFields/Drupal7/from coverage measurement.New tests
BlackboxDriverTestcovers 26BaseDrivermethods that throwUnsupportedDriverActionException, plusisBootstrapped,isField,isBaseField, andbootstrap.UnsupportedDriverActionExceptionTestcovers constructor formatting, driver accessor, and code/previous propagation.AddressHandler,DaterangeHandler,DatetimeHandler,DefaultHandler,EntityReferenceHandler,FileHandler,ImageHandler,ListHandlerBase(viaListString,ListInteger,ListFloat),SupportedImageHandler,TaxonomyTermReferenceHandler,TextWithSummaryHandler. Tests useReflectionClass::newInstanceWithoutConstructor()to bypassAbstractHandler's Drupal bootstrap, and where handlers touch\Drupal::service(...), the Drupal container is populated with PHPUnit mocks.CI threshold (
.github/workflows/ci.yml)Raised
CI_COVERAGE_THRESHOLDfrom'10'to'35'to lock in the new floor.Before / After
Coverage (D8+ scope):
Summary by CodeRabbit
Release Notes
Tests
Chores