Added 'declare(strict_types=1)' and PHP 8.2 type declarations via Rector.#337
Merged
AlexSkrypnyk merged 7 commits intomasterfrom Apr 17, 2026
Merged
Added 'declare(strict_types=1)' and PHP 8.2 type declarations via Rector.#337AlexSkrypnyk merged 7 commits intomasterfrom
AlexSkrypnyk merged 7 commits intomasterfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (43)
📝 WalkthroughWalkthroughThe pull request systematically adds PHP 8+ strict typing declarations and return types across the codebase. Updates include enabling Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Drupal/Driver/Exception/Exception.php (1)
36-44:⚠️ Potential issue | 🟡 MinorDocblock return type is inconsistent with the nullable property.
$this->driveris now typed as?DriverInterfaceand defaults toNULLwhen the constructor is called without a driver, sogetDriver()can returnnull. The@return \Drupal\Driver\DriverInterfacedocblock should reflect that (and adding a real: ?DriverInterfacereturn type here would match the strict-typing goals of this PR).📝 Suggested doc/return type alignment
/** * Returns exception driver. * - * `@return` \Drupal\Driver\DriverInterface + * `@return` \Drupal\Driver\DriverInterface|null * The driver where the exception occurred. */ - public function getDriver() { + public function getDriver(): ?DriverInterface { return $this->driver; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Exception/Exception.php` around lines 36 - 44, Update the getDriver() signature and docblock to match the nullable $driver property: change the docblock `@return` to `@return` \Drupal\Driver\DriverInterface|null and add a nullable return type to the method (public function getDriver(): ?\Drupal\Driver\DriverInterface) so the method and docs correctly reflect that $this->driver may be null; reference the getDriver() method and the $driver property in the Exception class when making the change.src/Drupal/Driver/Fields/Drupal8/FileHandler.php (1)
19-23:⚠️ Potential issue | 🟡 MinorVerify
file_get_contents()understrict_types=1with non-string$file_path.Line 19 now casts
$file_pathtostringforpathinfo(), but line 20 (file_get_contents($file_path)) and line 23 (sprintf('... %s ...', $file_path)) still pass the raw$file_path. If callers ever pass a non-string (e.g. integertarget_idfrom an entity reference),file_get_contents()will raise aTypeErrorunder strict types. Consider casting once and reusing:Proposed change
- $file_path = is_array($value) ? $value['target_id'] ?? $value[0] : $value; - $file_extension = pathinfo((string) $file_path, PATHINFO_EXTENSION); - $data = file_get_contents($file_path); + $file_path = (string) (is_array($value) ? $value['target_id'] ?? $value[0] : $value); + $file_extension = pathinfo($file_path, PATHINFO_EXTENSION); + $data = file_get_contents($file_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Fields/Drupal8/FileHandler.php` around lines 19 - 23, The code in FileHandler.php uses pathinfo((string) $file_path) but leaves file_get_contents($file_path) and sprintf(... $file_path) using the original variable, which can cause TypeError under strict_types if callers pass non-strings; fix by casting $file_path to string once up front (e.g., $file_path = (string) $file_path) and then use that casted variable for pathinfo, file_get_contents and sprintf so all calls use a guaranteed string; update references to $file_path within the method where pathinfo, file_get_contents and the exception sprintf are used.src/Drupal/Driver/Fields/Drupal8/DatetimeHandler.php (1)
40-46:⚠️ Potential issue | 🟡 MinorDocblock/return-type inconsistent with actual behavior.
formatDateValue()can returnNULL(line 45) but the docblock declares@return stringand — unlike most methods touched by this PR — no explicit return type was added. Understrict_types=1, this will bite if a future caller assigns the result to astring-typed sink. Consider tightening to: ?stringand updating the docblock, or normalizing the empty case to return''.♻️ Proposed fix
- * `@return` string + * `@return` string|null * The formatted date string. */ - protected function formatDateValue($value, \DateTimeZone $site_timezone, \DateTimeZone $storage_timezone) { + protected function formatDateValue($value, \DateTimeZone $site_timezone, \DateTimeZone $storage_timezone): ?string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Fields/Drupal8/DatetimeHandler.php` around lines 40 - 46, The docblock and signature for formatDateValue are inconsistent with its behavior (it returns NULL); update formatDateValue to have a nullable return type and matching docblock or normalize null/empty to an empty string—specifically, change the method signature to return ?string and update the `@return` in the docblock to "@return string|null", or alternatively ensure the early-return branch returns '' instead of NULL and keep `@return` string; update any callers/tests if needed to match the chosen approach.
♻️ Duplicate comments (1)
tests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.php (1)
52-132:⚠️ Potential issue | 🟡 MinorData providers likely need
public staticfor PHPUnit 10+.
dataProviderIsBaseField,dataProviderIsField, anddataProviderGetEntityFieldTypesare instance methods. If the repo uses PHPUnit ≥10, non-static data providers are deprecated (removed in 11). SinceBlackboxDriverTest::dataProviderUnsupportedActionsThrowin this same PR is alreadypublic static, aligning these maintains consistency.Apply the same conversion to all three providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.php` around lines 52 - 132, Update the three data provider methods to be static so PHPUnit 10+ accepts them: change the signatures of dataProviderIsBaseField(), dataProviderIsField(), and dataProviderGetEntityFieldTypes() to public static function ... : \Iterator; keep their yields and return types unchanged and leave the `@dataProvider` annotations in testIsBaseField, testIsField, and testGetEntityFieldTypes as-is.
🧹 Nitpick comments (14)
tests/Drupal/Tests/Driver/Drupal8Test.php (1)
22-24: Property type hint would be more consistent with PR direction.The PR summary mentions adding property type declarations across the codebase.
$originalRequestTimeis declared as@var intbut without a type hint, and it's assignedNULLinsetUp()(line 31). Considerprotected ?int $originalRequestTime = NULL;to match both the actual runtime values and the PR's typing goals; the@vardocblock would then be redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Drupal8Test.php` around lines 22 - 24, The property $originalRequestTime in class Drupal8Test is documented as `@var` int but is assigned NULL in setUp(), so change its declaration to a nullable typed property to match runtime values and the PR typing goal: replace the untyped protected $originalRequestTime with protected ?int $originalRequestTime = NULL and remove the redundant `@var` docblock; ensure any references in setUp() and tests still work with the nullable int type.src/Drupal/Driver/Fields/Drupal8/FileHandler.php (1)
15-15: Consider propagating: arrayreturn type toFieldHandlerInterface::expand().
FileHandler::expand()(and sibling handlers) now declares: array, butFieldHandlerInterface::expand()still lacks a return type. While covariance makes this legal, tightening the interface would make the contract consistent across all implementations and prevent drift in future handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Fields/Drupal8/FileHandler.php` at line 15, The interface method FieldHandlerInterface::expand() lacks the stricter return type even though implementations like FileHandler::expand() declare ": array"; update the FieldHandlerInterface::expand() signature to include the ": array" return type so all implementations conform to the interface (and adjust any other handler implementations or usages if needed to satisfy the new signature), then run static/type checks to ensure no breaks.src/Drupal/Driver/BlackboxDriver.php (1)
15-15: Consider adding: booltoDriverInterface::isBootstrapped().Adding the return type only on the implementation leaves the interface contract loose. Since all concrete drivers return bool, declaring
: boolonDriverInterface::isBootstrapped()(and on sibling methods where applicable) would better express the contract and help static analysis. Covariant return types make this a safe, source-compatible change for existing implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/BlackboxDriver.php` at line 15, Add the explicit return type on the interface: update DriverInterface::isBootstrapped() to declare ": bool" so the interface matches implementations like BlackboxDriver::isBootstrapped(); likewise add ": bool" to any sibling methods in DriverInterface that concrete drivers already return bool for (so implementations remain source-compatible and static analysis is accurate).src/Drupal/Driver/Cores/AbstractCore.php (1)
32-44: Consider making$randomnon-nullable.The constructor always assigns a non-null
Random(lines 40–43 instantiate a default when the argument isNULL), and subclasses such asDrupal8dereference$this->random->name(...)without null checks. Declaringprotected Random $random;more accurately reflects the invariant and avoids misleading?at the type site and any future null-safety churn.♻️ Proposed tweak
- protected ?Random $random; + protected Random $random;Note: Rector's
TypedPropertyFromAssignsRector/PropertyTypeDeclarationRectortypically infer nullability from declared defaults; since there's no default here the nullable inference is conservative rather than necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Cores/AbstractCore.php` around lines 32 - 44, The property $random is always initialized in AbstractCore::__construct and is dereferenced elsewhere (e.g., Drupal8->...->name), so change the property declaration from protected ?Random $random; to protected Random $random; to reflect the invariant; keep the constructor parameter as ?Random $random = NULL (or accept Random|null) and retain the fallback new Random() assignment in __construct so the non-nullable property is always initialized; scan usages of AbstractCore::$random (e.g., Drupal8) to ensure no null checks are required after this change.tests/Drupal/Tests/Driver/Fields/Drupal8/DaterangeHandlerTest.php (1)
73-95: Minor:loadDatetimeModuleInterface()lacks a return type.For consistency with the PR's type-declaration pass (and the strict-types header), consider adding
: bool— all return statements already returnTRUE/FALSE.♻️ Proposed tweak
- protected function loadDatetimeModuleInterface() { + protected function loadDatetimeModuleInterface(): bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/DaterangeHandlerTest.php` around lines 73 - 95, The method loadDatetimeModuleInterface currently returns only TRUE/FALSE but lacks a declared return type; update its signature to declare a boolean return type (add ": bool") on the loadDatetimeModuleInterface method and ensure the implementation (uses of DateTimeItemInterface and InstalledVersions) remains unchanged so all existing return TRUE/FALSE statements satisfy the new type.src/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.php (1)
15-15: Optional: consider adding: arrayonFieldHandlerInterface::expand()for consistency.Adding a covariant
: arrayreturn on the implementation is valid PHP, but the interface and some siblings (DefaultHandler,TextWithSummaryHandler,DatetimeHandler) still omit the return type. Declaring it on the interface would enforce the contract uniformly across all handlers. Not a blocker — can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.php` at line 15, Add a covariant return type on the FieldHandlerInterface::expand() signature (declare it as returning : array) so all implementations (including EntityReferenceHandler::expand(), DefaultHandler::expand(), TextWithSummaryHandler::expand(), DatetimeHandler::expand(), etc.) must return an array; update the interface method signature only (not the implementations) to include ": array" to enforce the contract across all handlers.tests/Drupal/Tests/Driver/Drupal8PermissionsTest.php (2)
94-105: Consistency: add: voidto the two reflection invoker helpers.
setPermissions()and the test methods in this file were all updated to: void, but these two helpers weren't. They don't return a value — aligning them keeps the file consistent with the repo-wide typing pass.♻️ Proposed fix
- protected function callConvertPermissions(Drupal8 $core, array &$permissions) { + protected function callConvertPermissions(Drupal8 $core, array &$permissions): void { $method = new \ReflectionMethod($core, 'convertPermissions'); $method->invokeArgs($core, [&$permissions]); } @@ - protected function callCheckPermissions(Drupal8 $core, array &$permissions) { + protected function callCheckPermissions(Drupal8 $core, array &$permissions): void { $method = new \ReflectionMethod($core, 'checkPermissions'); $method->invokeArgs($core, [&$permissions]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Drupal8PermissionsTest.php` around lines 94 - 105, Add explicit return type annotations ": void" to the two helper methods callConvertPermissions and callCheckPermissions; these methods currently declare "protected function callConvertPermissions(Drupal8 $core, array &$permissions)" and "protected function callCheckPermissions(Drupal8 $core, array &$permissions)" and should be changed to "protected function callConvertPermissions(Drupal8 $core, array &$permissions): void" and "protected function callCheckPermissions(Drupal8 $core, array &$permissions): void" respectively—nothing else in the body needs to change (they use ReflectionMethod and invokeArgs).
110-123: Minor: anonymous-class$labelis untyped, so__toString(): stringcan fatal under strict types.With
declare(strict_types=1);, if a non-string ever reaches$this->label, the__toString(): stringreturn will throw aTypeError. Current callers pass strings, so this is just a defensive hardening.♻️ Proposed fix
- protected function stringable($label): object { + protected function stringable(string $label): object { return new class($label) { - - public function __construct(private $label) {} + + public function __construct(private string $label) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Drupal8PermissionsTest.php` around lines 110 - 123, The anonymous class used by stringable() accepts an untyped $label which can cause a TypeError in strict mode when __toString(): string returns it; update the signature and constructor to enforce a string: change stringable($label) to stringable(string $label): object and change the anonymous class constructor to accept private string $label so the stored value is always a string for __toString().tests/Drupal/Tests/Driver/Fields/Drupal8/ListHandlerTest.php (1)
87-87: Consistency: add types tocreateHandler()helper.Other reworked test helpers in this PR (
LinkHandlerTest::createHandler(): LinkHandler,NameHandlerTest::createHandler(): NameHandler,ImageHandlerTest::createHandler(): ImageHandler,SupportedImageHandlerTest::createHandler(): SupportedImageHandler) all got concrete return types. Consider mirroring that here for consistency.♻️ Proposed fix
- protected function createHandler($class_name, array $allowed_values) { + protected function createHandler(string $class_name, array $allowed_values): object {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Fields/Drupal8/ListHandlerTest.php` at line 87, Update the helper signature to add concrete types: change protected function createHandler($class_name, array $allowed_values) to include a string type for $class_name and a ListHandler return type (e.g. protected function createHandler(string $class_name, array $allowed_values): ListHandler). Ensure the ListHandler symbol is imported or fully-qualified to match the other test helpers.src/Drupal/Driver/Cores/Drupal8.php (2)
439-447: Tighten return type fromobjecttoEntityFieldManagerInterface.
objectis unnecessarily loose —\Drupal::service('entity_field.manager')returns anEntityFieldManagerInterface, andcreateMock(EntityFieldManagerInterface::class)in the tests also produces an instance of that interface, so a narrower return type doesn’t break the test double.♻️ Proposed change
- protected function getEntityFieldManager(): object { - return \Drupal::service('entity_field.manager'); + protected function getEntityFieldManager(): \Drupal\Core\Entity\EntityFieldManagerInterface { + return \Drupal::service('entity_field.manager'); }And correspondingly in
tests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.php:- protected function getEntityFieldManager(): object { + protected function getEntityFieldManager(): \Drupal\Core\Entity\EntityFieldManagerInterface { return $this->entityFieldManager; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Cores/Drupal8.php` around lines 439 - 447, The getEntityFieldManager() return type is too broad; change the method signature in class Drupal8 from protected function getEntityFieldManager(): object to protected function getEntityFieldManager(): \Drupal\Core\Entity\EntityFieldManagerInterface (or import and use EntityFieldManagerInterface) and keep the body returning \Drupal::service('entity_field.manager'); this tightens the contract to the actual service type and matches tests that createMock(EntityFieldManagerInterface::class).
595-601: Lambda’s strictarrayparam assumes well-formed state.With
strict_types=1, ifsystem.test_mail_collectorever contains a non-array entry (e.g., stale data, foreign writers),array_filterwill raise aTypeErroron the first such item. In practice Drupal’s test mail collector stores associative arrays, so this is unlikely — just noting the new failure mode versus the previously untyped closure. Consider a light guard if robustness matters:🛡️ Optional guard
- $mail = array_values(array_filter($mail, fn(array $mail_item): bool => $mail_item['send'] == TRUE)); + $mail = array_values(array_filter( + $mail, + fn($mail_item): bool => is_array($mail_item) && !empty($mail_item['send']) + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Cores/Drupal8.php` around lines 595 - 601, The inline arrow function in getMail (the array_filter call using fn(array $mail_item): bool => $mail_item['send'] == TRUE) will throw under strict_types if state contains non-array entries; change the predicate to defensively handle non-arrays (e.g., use a closure that first checks is_array($mail_item) and isset($mail_item['send']) and then returns (bool)$mail_item['send']) so array_filter only keeps well-formed, send==TRUE items; update the array_filter call in getMail accordingly.src/Drupal/Driver/DrupalDriver.php (1)
61-68: Consider typing$uriasstringfor consistency with$drupal_root.
$drupal_rootis nowstringbut$uriremains untyped. Addingstring $uriwould make the constructor signature uniform. All callers in the codebase pass string values (e.g.,'http://d8.devl'), and this change poses no compatibility risk sinceDriverInterfacedoes not declare a constructor signature.♻️ Proposed change
- public function __construct(string $drupal_root, $uri) { + public function __construct(string $drupal_root, string $uri) {🤖 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 61 - 68, The constructor in class DrupalDriver (__construct) currently types $drupal_root as string but leaves $uri untyped; update the constructor signature to type-hint $uri as string (i.e., change $uri to string $uri) so both parameters are consistent, and update any related docblock or usages in DrupalDriver if present to reflect the new type (no other code changes needed because callers already pass strings).src/Drupal/Driver/DrushDriver.php (2)
47-47: Property should be non-nullableRandom, not?Random.The constructor (lines 101-104) guarantees
$this->randomis always assigned aRandominstance (falling back tonew Random()when the argument is null). Declaring the property as?RandomwhilegetRandom(): Randomreturns it non-nullable creates a type mismatch that static analyzers (PHPStan/Psalm) will flag, and misrepresents the invariant.♻️ Proposed fix
- private readonly ?Random $random; + private readonly Random $random;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/DrushDriver.php` at line 47, Property $random is incorrectly declared nullable while the constructor always assigns a Random and getRandom(): Random returns non-nullable; change the property declaration in class DrushDriver from "private readonly ?Random $random" to "private readonly Random $random" (and update any docblocks if present) so the type matches the constructor and getRandom() invariant; check constructor assignment in __construct(...) remains the fallback "new Random()" path to guarantee non-null.
320-328: Stale docblock:@return objectno longer matches: mixed.
json_decode()can return an object, array, scalar, or null, and the signature now reflects that with: mixed. The@return objectline (and description "The decoded JSON object") is misleading — callers such ascallBehatCommand()also advertise@return objectbut now returnmixed. Consider updating the docblocks to@return mixed(or narrowing both to@return \stdClass|nullif that is the actual intended contract).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/DrushDriver.php` around lines 320 - 328, Update the stale docblock on decodeJsonObject to match the actual return type: change the `@return` annotation from "object" to "mixed" (or to a narrower type like "\stdClass|null" if you intend to only return objects/null) and update the description text from "The decoded JSON object" to reflect that json_decode may return object, array, scalar, or null; also audit sibling methods like callBehatCommand() that still say "@return object" and align their docblocks to the same type contract.
🤖 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/DrushDriverTest.php`:
- Around line 76-123: Change the two data provider methods to be static: update
the signatures of dataProviderParseUserId() and dataProviderIsLegacyDrush() to
public static function dataProviderParseUserId(): \Iterator and public static
function dataProviderIsLegacyDrush(): \Iterator respectively, keeping their
bodies and return types intact so PHPUnit 10/11 will accept them as static data
providers.
---
Outside diff comments:
In `@src/Drupal/Driver/Exception/Exception.php`:
- Around line 36-44: Update the getDriver() signature and docblock to match the
nullable $driver property: change the docblock `@return` to `@return`
\Drupal\Driver\DriverInterface|null and add a nullable return type to the method
(public function getDriver(): ?\Drupal\Driver\DriverInterface) so the method and
docs correctly reflect that $this->driver may be null; reference the getDriver()
method and the $driver property in the Exception class when making the change.
In `@src/Drupal/Driver/Fields/Drupal8/DatetimeHandler.php`:
- Around line 40-46: The docblock and signature for formatDateValue are
inconsistent with its behavior (it returns NULL); update formatDateValue to have
a nullable return type and matching docblock or normalize null/empty to an empty
string—specifically, change the method signature to return ?string and update
the `@return` in the docblock to "@return string|null", or alternatively ensure
the early-return branch returns '' instead of NULL and keep `@return` string;
update any callers/tests if needed to match the chosen approach.
In `@src/Drupal/Driver/Fields/Drupal8/FileHandler.php`:
- Around line 19-23: The code in FileHandler.php uses pathinfo((string)
$file_path) but leaves file_get_contents($file_path) and sprintf(... $file_path)
using the original variable, which can cause TypeError under strict_types if
callers pass non-strings; fix by casting $file_path to string once up front
(e.g., $file_path = (string) $file_path) and then use that casted variable for
pathinfo, file_get_contents and sprintf so all calls use a guaranteed string;
update references to $file_path within the method where pathinfo,
file_get_contents and the exception sprintf are used.
---
Duplicate comments:
In `@tests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.php`:
- Around line 52-132: Update the three data provider methods to be static so
PHPUnit 10+ accepts them: change the signatures of dataProviderIsBaseField(),
dataProviderIsField(), and dataProviderGetEntityFieldTypes() to public static
function ... : \Iterator; keep their yields and return types unchanged and leave
the `@dataProvider` annotations in testIsBaseField, testIsField, and
testGetEntityFieldTypes as-is.
---
Nitpick comments:
In `@src/Drupal/Driver/BlackboxDriver.php`:
- Line 15: Add the explicit return type on the interface: update
DriverInterface::isBootstrapped() to declare ": bool" so the interface matches
implementations like BlackboxDriver::isBootstrapped(); likewise add ": bool" to
any sibling methods in DriverInterface that concrete drivers already return bool
for (so implementations remain source-compatible and static analysis is
accurate).
In `@src/Drupal/Driver/Cores/AbstractCore.php`:
- Around line 32-44: The property $random is always initialized in
AbstractCore::__construct and is dereferenced elsewhere (e.g.,
Drupal8->...->name), so change the property declaration from protected ?Random
$random; to protected Random $random; to reflect the invariant; keep the
constructor parameter as ?Random $random = NULL (or accept Random|null) and
retain the fallback new Random() assignment in __construct so the non-nullable
property is always initialized; scan usages of AbstractCore::$random (e.g.,
Drupal8) to ensure no null checks are required after this change.
In `@src/Drupal/Driver/Cores/Drupal8.php`:
- Around line 439-447: The getEntityFieldManager() return type is too broad;
change the method signature in class Drupal8 from protected function
getEntityFieldManager(): object to protected function getEntityFieldManager():
\Drupal\Core\Entity\EntityFieldManagerInterface (or import and use
EntityFieldManagerInterface) and keep the body returning
\Drupal::service('entity_field.manager'); this tightens the contract to the
actual service type and matches tests that
createMock(EntityFieldManagerInterface::class).
- Around line 595-601: The inline arrow function in getMail (the array_filter
call using fn(array $mail_item): bool => $mail_item['send'] == TRUE) will throw
under strict_types if state contains non-array entries; change the predicate to
defensively handle non-arrays (e.g., use a closure that first checks
is_array($mail_item) and isset($mail_item['send']) and then returns
(bool)$mail_item['send']) so array_filter only keeps well-formed, send==TRUE
items; update the array_filter call in getMail accordingly.
In `@src/Drupal/Driver/DrupalDriver.php`:
- Around line 61-68: The constructor in class DrupalDriver (__construct)
currently types $drupal_root as string but leaves $uri untyped; update the
constructor signature to type-hint $uri as string (i.e., change $uri to string
$uri) so both parameters are consistent, and update any related docblock or
usages in DrupalDriver if present to reflect the new type (no other code changes
needed because callers already pass strings).
In `@src/Drupal/Driver/DrushDriver.php`:
- Line 47: Property $random is incorrectly declared nullable while the
constructor always assigns a Random and getRandom(): Random returns
non-nullable; change the property declaration in class DrushDriver from "private
readonly ?Random $random" to "private readonly Random $random" (and update any
docblocks if present) so the type matches the constructor and getRandom()
invariant; check constructor assignment in __construct(...) remains the fallback
"new Random()" path to guarantee non-null.
- Around line 320-328: Update the stale docblock on decodeJsonObject to match
the actual return type: change the `@return` annotation from "object" to "mixed"
(or to a narrower type like "\stdClass|null" if you intend to only return
objects/null) and update the description text from "The decoded JSON object" to
reflect that json_decode may return object, array, scalar, or null; also audit
sibling methods like callBehatCommand() that still say "@return object" and
align their docblocks to the same type contract.
In `@src/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.php`:
- Line 15: Add a covariant return type on the FieldHandlerInterface::expand()
signature (declare it as returning : array) so all implementations (including
EntityReferenceHandler::expand(), DefaultHandler::expand(),
TextWithSummaryHandler::expand(), DatetimeHandler::expand(), etc.) must return
an array; update the interface method signature only (not the implementations)
to include ": array" to enforce the contract across all handlers.
In `@src/Drupal/Driver/Fields/Drupal8/FileHandler.php`:
- Line 15: The interface method FieldHandlerInterface::expand() lacks the
stricter return type even though implementations like FileHandler::expand()
declare ": array"; update the FieldHandlerInterface::expand() signature to
include the ": array" return type so all implementations conform to the
interface (and adjust any other handler implementations or usages if needed to
satisfy the new signature), then run static/type checks to ensure no breaks.
In `@tests/Drupal/Tests/Driver/Drupal8PermissionsTest.php`:
- Around line 94-105: Add explicit return type annotations ": void" to the two
helper methods callConvertPermissions and callCheckPermissions; these methods
currently declare "protected function callConvertPermissions(Drupal8 $core,
array &$permissions)" and "protected function callCheckPermissions(Drupal8
$core, array &$permissions)" and should be changed to "protected function
callConvertPermissions(Drupal8 $core, array &$permissions): void" and "protected
function callCheckPermissions(Drupal8 $core, array &$permissions): void"
respectively—nothing else in the body needs to change (they use ReflectionMethod
and invokeArgs).
- Around line 110-123: The anonymous class used by stringable() accepts an
untyped $label which can cause a TypeError in strict mode when __toString():
string returns it; update the signature and constructor to enforce a string:
change stringable($label) to stringable(string $label): object and change the
anonymous class constructor to accept private string $label so the stored value
is always a string for __toString().
In `@tests/Drupal/Tests/Driver/Drupal8Test.php`:
- Around line 22-24: The property $originalRequestTime in class Drupal8Test is
documented as `@var` int but is assigned NULL in setUp(), so change its
declaration to a nullable typed property to match runtime values and the PR
typing goal: replace the untyped protected $originalRequestTime with protected
?int $originalRequestTime = NULL and remove the redundant `@var` docblock; ensure
any references in setUp() and tests still work with the nullable int type.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/DaterangeHandlerTest.php`:
- Around line 73-95: The method loadDatetimeModuleInterface currently returns
only TRUE/FALSE but lacks a declared return type; update its signature to
declare a boolean return type (add ": bool") on the loadDatetimeModuleInterface
method and ensure the implementation (uses of DateTimeItemInterface and
InstalledVersions) remains unchanged so all existing return TRUE/FALSE
statements satisfy the new type.
In `@tests/Drupal/Tests/Driver/Fields/Drupal8/ListHandlerTest.php`:
- Line 87: Update the helper signature to add concrete types: change protected
function createHandler($class_name, array $allowed_values) to include a string
type for $class_name and a ListHandler return type (e.g. protected function
createHandler(string $class_name, array $allowed_values): ListHandler). Ensure
the ListHandler symbol is imported or fully-qualified to match the other test
helpers.
🪄 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: f83a0869-16a8-48d6-94dd-5068e6b856a0
📒 Files selected for processing (57)
composer.jsonrector.phpsrc/Drupal/Driver/AuthenticationDriverInterface.phpsrc/Drupal/Driver/BaseDriver.phpsrc/Drupal/Driver/BlackboxDriver.phpsrc/Drupal/Driver/Cores/AbstractCore.phpsrc/Drupal/Driver/Cores/CoreAuthenticationInterface.phpsrc/Drupal/Driver/Cores/CoreInterface.phpsrc/Drupal/Driver/Cores/Drupal8.phpsrc/Drupal/Driver/DriverInterface.phpsrc/Drupal/Driver/DrupalDriver.phpsrc/Drupal/Driver/DrushDriver.phpsrc/Drupal/Driver/Exception/BootstrapException.phpsrc/Drupal/Driver/Exception/Exception.phpsrc/Drupal/Driver/Exception/UnsupportedDriverActionException.phpsrc/Drupal/Driver/Fields/Drupal8/AbstractHandler.phpsrc/Drupal/Driver/Fields/Drupal8/AddressHandler.phpsrc/Drupal/Driver/Fields/Drupal8/DaterangeHandler.phpsrc/Drupal/Driver/Fields/Drupal8/DatetimeHandler.phpsrc/Drupal/Driver/Fields/Drupal8/DefaultHandler.phpsrc/Drupal/Driver/Fields/Drupal8/EmbridgeAssetItemHandler.phpsrc/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.phpsrc/Drupal/Driver/Fields/Drupal8/FileHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ImageHandler.phpsrc/Drupal/Driver/Fields/Drupal8/LinkHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ListFloatHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ListHandlerBase.phpsrc/Drupal/Driver/Fields/Drupal8/ListIntegerHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ListStringHandler.phpsrc/Drupal/Driver/Fields/Drupal8/NameHandler.phpsrc/Drupal/Driver/Fields/Drupal8/OgStandardReferenceHandler.phpsrc/Drupal/Driver/Fields/Drupal8/SupportedImageHandler.phpsrc/Drupal/Driver/Fields/Drupal8/TaxonomyTermReferenceHandler.phpsrc/Drupal/Driver/Fields/Drupal8/TextWithSummaryHandler.phpsrc/Drupal/Driver/Fields/Drupal8/TimeHandler.phpsrc/Drupal/Driver/Fields/FieldHandlerInterface.phpsrc/Drupal/Driver/SubDriverFinderInterface.phptests/Drupal/Tests/Driver/BlackboxDriverTest.phptests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.phptests/Drupal/Tests/Driver/Drupal8PermissionsTest.phptests/Drupal/Tests/Driver/Drupal8Test.phptests/Drupal/Tests/Driver/DrushDriverTest.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.phptests/Drupal/Tests/Driver/LinkHandlerTest.phptests/Drupal/Tests/Driver/NameHandlerTest.phptests/Drupal/Tests/Driver/TimeHandlerTest.php
…Slevomat type-hint sniffs.
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
Introduces
declare(strict_types=1)and PHP 8.2 parameter/return/property type declarations across all 57 files in the codebase (32 source files, 24 test files, plusrector.php). Rector is configured to mirror the Drupal Extension's setup, usingphp82sets,typeDeclarationsandnamingprepared sets, andDeclareStrictTypesRector. Switched from the unmaintainedpalantirnet/drupal-rector ^0.20torector/rector ^2.0directly - the same dependency Drupal Extension uses.Changes
Tooling
composer.json: droppedpalantirnet/drupal-rector ^0.20; addedrector/rector ^2.0directly. Matches Drupal Extension's setup.rector.php: rewritten to mirror Drupal Extension's rector config. Key changes:withPhpSets(php74: TRUE)->withPhpSets(php82: TRUE)typeDeclarationsandnamingtowithPreparedSetsDeclareStrictTypesRectorandYieldDataProviderRectorrulesChangeSwitchToMatchRector,CompleteDynamicPropertiesRector, severalNamingrules,ClassPropertyAssignToConstructorPromotionRector)withImportNames(importNames: TRUE, importDocBlockNames: FALSE, importShortClasses: FALSE)withFileExtensions(['php', 'inc'])Source code (
src/)declare(strict_types=1);to every file (32 files).DriverInterface,CoreInterface,BaseDriver,BlackboxDriver,DrupalDriver,DrushDriver,AbstractCore,Drupal8, all field handlers).AbstractCore.php,Exception.php, andDrupalDriver.php(the promotion mangled existing docblocks); skipped that rule going forward.@return mixed[]docblocks rector added when the signature already declares: arrayon{@inheritdoc}methods (PHPCS sniffs require a description otherwise).Tests (
tests/)declare(strict_types=1);to every test file (24 files).: voidreturn types onsetUp/tearDown/ test methods.: arrayreturn types on data providers.Summary by CodeRabbit
Refactor
Chores