Merged
Conversation
WalkthroughAdded runtime type guards requiring Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/MediaTrait.php (1)
228-240:⚠️ Potential issue | 🔴 CriticalType safety issue: DriverInterface does not guarantee
getCore()method exists.The code checks
instanceof DriverInterfacebut then immediately calls$driver->getCore(), which only exists on the concrete\Drupal\Driver\DrupalDriverclass. The interface check is insufficient—this will cause a fatal error at runtime if used with a driver that doesn't implementgetCore().Additionally, the validation pattern is duplicated in
mediaExpandEntityFieldsFixtures(lines 260-266). Both issues should be addressed simultaneously:
- Change the type check to
\Drupal\Driver\DrupalDriverinstead ofDriverInterface- Extract the validation into a helper method to eliminate duplication
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/MediaTrait.php` around lines 228 - 240, The code uses instanceof DriverInterface before calling getCore(), but DriverInterface doesn't guarantee getCore(); replace the instanceof check with a concrete class check for \Drupal\Driver\DrupalDriver (i.e., ensure $driver instanceof \Drupal\Driver\DrupalDriver) and throw the same RuntimeException if not; to remove duplication between mediaExpandEntityFields and mediaExpandEntityFieldsFixtures extract the validation into a private helper method (e.g., ensureDrupalDriverSupportsCore or validateDrupalDriver) that accepts $driver, performs the instanceof \Drupal\Driver\DrupalDriver check and returns the typed driver (or throws), then call that helper from both mediaExpandEntityFields and mediaExpandEntityFieldsFixtures before calling getCore()/Reflection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Drupal/MediaTrait.php`:
- Around line 228-240: The code uses instanceof DriverInterface before calling
getCore(), but DriverInterface doesn't guarantee getCore(); replace the
instanceof check with a concrete class check for \Drupal\Driver\DrupalDriver
(i.e., ensure $driver instanceof \Drupal\Driver\DrupalDriver) and throw the same
RuntimeException if not; to remove duplication between mediaExpandEntityFields
and mediaExpandEntityFieldsFixtures extract the validation into a private helper
method (e.g., ensureDrupalDriverSupportsCore or validateDrupalDriver) that
accepts $driver, performs the instanceof \Drupal\Driver\DrupalDriver check and
returns the typed driver (or throws), then call that helper from both
mediaExpandEntityFields and mediaExpandEntityFieldsFixtures before calling
getCore()/Reflection.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #532 +/- ##
==========================================
- Coverage 99.95% 99.83% -0.13%
==========================================
Files 35 35
Lines 2356 2365 +9
==========================================
+ Hits 2355 2361 +6
- Misses 1 4 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/fix-drupal-driver-check
branch
from
149c790 to
0b055b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/MediaTrait.php`:
- Around line 228-235: Extract the duplicated driver/core guard into a single
helper on the trait (e.g., add a private method ensureDrupalDriver():
DrupalDriver or getDrupalCore(): CoreType) that performs $driver =
$this->getDriver(), asserts $driver instanceof DrupalDriver and throws the
RuntimeException if not, then returns the driver or core as needed; replace both
duplicated blocks that call $this->getDriver() and $driver->getCore() with a
single call to the new helper (use the helper name you add in MediaTrait) so all
Drupal-driver validation and core retrieval is centralized.
Comment on lines
+228
to
235
| $driver = $this->getDriver(); | ||
|
|
||
| if (!$driver instanceof DrupalDriver) { | ||
| throw new \RuntimeException('The current driver does not support Drupal-specific operations. Ensure you are using a compatible Drupal driver.'); | ||
| } | ||
|
|
||
| $core = $driver->getCore(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract shared Drupal-driver/core guard to one helper.
The new guard is correct, but duplicated in two methods. Centralizing it will reduce maintenance drift.
♻️ Proposed refactor
protected function mediaExpandEntityFields(string $entity_type, \StdClass $stub): void {
- $driver = $this->getDriver();
-
- if (!$driver instanceof DrupalDriver) {
- throw new \RuntimeException('The current driver does not support Drupal-specific operations. Ensure you are using a compatible Drupal driver.');
- }
-
- $core = $driver->getCore();
+ $core = $this->mediaGetDrupalCore();
$class = new \ReflectionClass($core::class);
$method = $class->getMethod('expandEntityFields');
$method->invokeArgs($core, func_get_args());
}
@@
- $driver = $this->getDriver();
-
- if (!$driver instanceof DrupalDriver) {
- throw new \RuntimeException('The current driver does not support Drupal-specific operations. Ensure you are using a compatible Drupal driver.');
- }
-
- $field_types = $driver->getCore()->getEntityFieldTypes('media', array_keys($fields));
+ $field_types = $this->mediaGetDrupalCore()->getEntityFieldTypes('media', array_keys($fields));
@@
}
+
+ protected function mediaGetDrupalCore(): object {
+ $driver = $this->getDriver();
+ if (!$driver instanceof DrupalDriver) {
+ throw new \RuntimeException('The current driver does not support Drupal-specific operations. Ensure you are using a compatible Drupal driver.');
+ }
+ return $driver->getCore();
+ }Also applies to: 260-267
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 231-231: Missing class import via use statement (line '231', column '17'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Drupal/MediaTrait.php` around lines 228 - 235, Extract the duplicated
driver/core guard into a single helper on the trait (e.g., add a private method
ensureDrupalDriver(): DrupalDriver or getDrupalCore(): CoreType) that performs
$driver = $this->getDriver(), asserts $driver instanceof DrupalDriver and throws
the RuntimeException if not, then returns the driver or core as needed; replace
both duplicated blocks that call $this->getDriver() and $driver->getCore() with
a single call to the new helper (use the helper name you add in MediaTrait) so
all Drupal-driver validation and core retrieval is centralized.
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 by CodeRabbit