Added project-level rector configuration and applied fixes.#328
Added project-level rector configuration and applied fixes.#328AlexSkrypnyk merged 3 commits intomasterfrom
Conversation
|
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 47 minutes and 28 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 (3)
📝 WalkthroughWalkthroughThis pull request refactors the codebase to improve code quality across multiple files. Key changes include: updating composer.json rector commands to use a centralized rector.php configuration, adding a new rector.php file with strict typing and multiple rule sets, adopting stricter type comparisons throughout the codebase, converting string concatenation to sprintf formatting in multiple locations, removing unnecessary intermediate variables and property initializations, and adjusting conditional logic in several field handlers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/Driver/Cores/Drupal8.php (1)
210-218:⚠️ Potential issue | 🟠 MajorStatic cache is never populated — Rector broke the
drupal_staticreference pattern.The original pattern was to assign the computed value back into the referenced
$permissionsso subsequent calls hit the static cache. Returning the service call directly from inside the!isset($permissions)branch means the reference is never written; the static cache staysNULL, every invocation re-executes the service lookup +getPermissions(), and the trailingreturn $permissions;at line 217 is unreachable on the first call and returns the empty uncached NULL path afterward... actually it never runs at all because the first-call branch always returns.🔧 Suggested fix
protected function getAllPermissions() { $permissions = &drupal_static(__FUNCTION__); if (!isset($permissions)) { - return \Drupal::service('user.permissions')->getPermissions(); + $permissions = \Drupal::service('user.permissions')->getPermissions(); } return $permissions; }🤖 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 210 - 218, The drupal_static cache in getAllPermissions() is never populated because the code returns the service result directly instead of assigning it into the referenced $permissions; update getAllPermissions() so that when !isset($permissions) you assign the result of \Drupal::service('user.permissions')->getPermissions() into $permissions (via the existing drupal_static reference) and then return $permissions, ensuring subsequent calls read the cached value rather than re-calling the service.
🧹 Nitpick comments (3)
tests/Drupal/Tests/Driver/FieldHandlerAbstractTestBase.php (1)
15-17: Consider callingparent::tearDown().The visibility change to
protectedis correct for PHPUnit 8+. While here, consider invokingparent::tearDown()after\Mockery::close()so futureTestCaseteardown hooks are not silently skipped. Non-blocking.Proposed tweak
protected function tearDown(): void { \Mockery::close(); + parent::tearDown(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/FieldHandlerAbstractTestBase.php` around lines 15 - 17, The tearDown method currently calls \Mockery::close() but omits invoking the base class teardown; update the FieldHandlerAbstractTestBase::tearDown() method to call parent::tearDown() (after \Mockery::close()) so any TestCase teardown hooks run as expected, keeping the method protected and returning void.src/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.php (1)
21-25: Lines 23-25 are now dead code.After the ternary on line 21,
$label_keyis unconditionally'name'whenever$entity_type_id === 'user', so theif (!$label_key && $entity_type_id == 'user')block can never execute. Safe to remove for clarity (this block was likely already unreachable pre-refactor, but the ternary makes it obvious).Proposed cleanup
// Determine label field key. $label_key = $entity_type_id !== 'user' ? $entity_definition->getKey('label') : 'name'; - - if (!$label_key && $entity_type_id == 'user') { - $label_key = 'name'; - }🤖 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` around lines 21 - 25, The ternary assignment to $label_key in EntityReferenceHandler makes the subsequent if check "if (!$label_key && $entity_type_id == 'user')" dead code; remove that unreachable if block and any related comments so $label_key is only set via "$label_key = $entity_type_id !== 'user' ? $entity_definition->getKey('label') : 'name';" and ensure no other code relies on the removed branch.src/Drupal/Driver/Fields/Drupal8/AbstractHandler.php (1)
55-58: Dead code:$fieldsstringis built but never used.The loop concatenates field names into
$fieldsstringbut the variable is never referenced afterward (it was presumably intended for the exception message on line 60, which now usessprintfwith only$field_name/$entity_type/$bundle). Rector's dead-code set should have flagged this — worth removing, or alternatively including$fieldsstringin the exception message to aid debugging.♻️ Proposed cleanup
$fields = $entity_field_manager->getFieldDefinitions($entity_type, $bundle); - $fieldsstring = ''; - foreach ($fields as $key => $value) { - $fieldsstring = $fieldsstring . ", " . $key; - } if (empty($fields[$field_name])) { throw new \Exception(sprintf('The field "%s" does not exist on entity type "%s" bundle "%s".', $field_name, $entity_type, $bundle)); }Or, if the intent was to list available fields on error:
- $fieldsstring = ''; - foreach ($fields as $key => $value) { - $fieldsstring = $fieldsstring . ", " . $key; - } if (empty($fields[$field_name])) { - throw new \Exception(sprintf('The field "%s" does not exist on entity type "%s" bundle "%s".', $field_name, $entity_type, $bundle)); + throw new \Exception(sprintf('The field "%s" does not exist on entity type "%s" bundle "%s". Available fields: %s.', $field_name, $entity_type, $bundle, implode(', ', array_keys($fields)))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/Driver/Fields/Drupal8/AbstractHandler.php` around lines 55 - 58, Remove or use the dead variable $fieldsstring inside the loop over $fields: either delete the $fieldsstring initialization and the foreach that concatenates values since it is never read, or include $fieldsstring in the exception/sprintf that currently references $field_name/$entity_type/$bundle so the built list of available fields is reported on error; update the code around the foreach and the exception (references: $fieldsstring, $fields, $field_name, $entity_type, $bundle, sprintf in AbstractHandler) accordingly.
🤖 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/Cores/Drupal6.php`:
- Around line 360-362: The exception message contains a leftover "%s"
placeholder; replace the literal string with a formatted message using sprintf
so the vocabulary identifier is interpolated—change the throw in the Drupal6
class (where $term->vid is checked) to throw new \Exception(sprintf('No "%s"
vocabulary found.', $term->vid)); so the actual vocabulary value is shown.
In `@src/Drupal/Driver/Cores/Drupal8.php`:
- Around line 522-524: The sprintf call uses $entity (an object) causing a
TypeError and the format '%s->%s' is incorrect; update the exception in Drupal8
(in the entity creation code where $entity and $bundle_key are used) to pass a
string value for the bundle, e.g. use $entity->{$bundle_key} (or cast it to
string) instead of $entity and keep $bundle_key as the second arg so the call
becomes sprintf("Cannot create entity because provided bundle '%s->%s' does not
exist.", $entity->{$bundle_key}, $bundle_key); ensure the placeholder order
matches the passed values similar to Drupal7::entityCreate().
In `@src/Drupal/Driver/Fields/Drupal7/ListTextHandler.php`:
- Around line 28-31: The branch uses loose in_array but strict array_search
which can yield FALSE keys; in the ListTextHandler (same pattern as
Drupal8/ListHandlerBase::expand()) change the logic to perform a strict
array_search($value, $options, TRUE), check the result with !== FALSE, and only
then set $allowed_values[$value] = $key; also use strict in_array only if you
intend loose behavior—prefer using the strict array_search check and remove the
loose in_array to avoid assigning FALSE into $allowed_values (refer to $value,
$options, $allowed_values, array_search).
---
Outside diff comments:
In `@src/Drupal/Driver/Cores/Drupal8.php`:
- Around line 210-218: The drupal_static cache in getAllPermissions() is never
populated because the code returns the service result directly instead of
assigning it into the referenced $permissions; update getAllPermissions() so
that when !isset($permissions) you assign the result of
\Drupal::service('user.permissions')->getPermissions() into $permissions (via
the existing drupal_static reference) and then return $permissions, ensuring
subsequent calls read the cached value rather than re-calling the service.
---
Nitpick comments:
In `@src/Drupal/Driver/Fields/Drupal8/AbstractHandler.php`:
- Around line 55-58: Remove or use the dead variable $fieldsstring inside the
loop over $fields: either delete the $fieldsstring initialization and the
foreach that concatenates values since it is never read, or include
$fieldsstring in the exception/sprintf that currently references
$field_name/$entity_type/$bundle so the built list of available fields is
reported on error; update the code around the foreach and the exception
(references: $fieldsstring, $fields, $field_name, $entity_type, $bundle, sprintf
in AbstractHandler) accordingly.
In `@src/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.php`:
- Around line 21-25: The ternary assignment to $label_key in
EntityReferenceHandler makes the subsequent if check "if (!$label_key &&
$entity_type_id == 'user')" dead code; remove that unreachable if block and any
related comments so $label_key is only set via "$label_key = $entity_type_id !==
'user' ? $entity_definition->getKey('label') : 'name';" and ensure no other code
relies on the removed branch.
In `@tests/Drupal/Tests/Driver/FieldHandlerAbstractTestBase.php`:
- Around line 15-17: The tearDown method currently calls \Mockery::close() but
omits invoking the base class teardown; update the
FieldHandlerAbstractTestBase::tearDown() method to call parent::tearDown()
(after \Mockery::close()) so any TestCase teardown hooks run as expected,
keeping the method protected and returning void.
🪄 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: 05979bc8-634c-464f-9a3d-256d83cb6959
📒 Files selected for processing (20)
composer.jsonrector.phpsrc/Drupal/Driver/Cores/Drupal6.phpsrc/Drupal/Driver/Cores/Drupal7.phpsrc/Drupal/Driver/Cores/Drupal8.phpsrc/Drupal/Driver/DrupalDriver.phpsrc/Drupal/Driver/DrushDriver.phpsrc/Drupal/Driver/Fields/Drupal7/AbstractHandler.phpsrc/Drupal/Driver/Fields/Drupal7/ListTextHandler.phpsrc/Drupal/Driver/Fields/Drupal7/TaxonomyTermReferenceHandler.phpsrc/Drupal/Driver/Fields/Drupal8/AbstractHandler.phpsrc/Drupal/Driver/Fields/Drupal8/AddressHandler.phpsrc/Drupal/Driver/Fields/Drupal8/EntityReferenceHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ImageHandler.phpsrc/Drupal/Driver/Fields/Drupal8/ListHandlerBase.phpsrc/Drupal/Driver/Fields/Drupal8/SupportedImageHandler.phpsrc/Drupal/Driver/Fields/Drupal8/TimeHandler.phptests/Drupal/Tests/Driver/Drupal8FieldMethodsTest.phptests/Drupal/Tests/Driver/Drupal8Test.phptests/Drupal/Tests/Driver/FieldHandlerAbstractTestBase.php
|
All three CodeRabbit findings fixed in ea8f8a7: restored sprintf in Drupal6 exception, corrected sprintf args in Drupal8 entityCreate, and replaced loose in_array + strict array_search with strict-only pattern in D7 ListTextHandler. |
1 participant
Summary
rector.phpconfiguration targeting PHP 7.4, replacing the previous approach of copyingpalantirnet/drupal-rector's config into thedrupal/directory before running Rector against a limited set of files.instanceofrule sets, with targeted skips for rules that conflict with Drupal coding standards or the existing mixed-type codebase.composer lintandcomposer lint-fixscripts to invokerector processdirectly using the project-level config, removing the fragilecp+cdworkaround.array_search()comparisons, ternary-to-null-coalescing simplifications, explicitreturnstatements, encapsulated string literals converted tosprintf(), and dead code removal.Test plan
composer lintand confirm Rector reports no remaining fixable issues.composer testand confirm all existing tests pass.rector.phpis picked up automatically without anycpor directory-change steps.Summary by CodeRabbit
Bug Fixes
Refactor
Chores