Address multiple improvement issues (#2, #3, #4)#9
Merged
Conversation
added 3 commits
January 28, 2026 10:49
Fix #2: Add ContentIntelCollectorInterface - Create interface defining the service contract - Update ContentIntelCollector to implement the interface - Add loadEntities() method for bulk entity loading Fix #3: Add logging for silent exception in ContentTranslationPlugin - Inject logger service - Log warnings when translation metadata fetch fails - Add metadata_error field to output for debugging Fix #4: Optimize batch command with loadMultiple() - Use loadEntities() for bulk loading instead of individual loads - Reduces N+1 queries to single loadMultiple() call - Improves performance for large batch operations
- Install statistics contrib module in drupal-check script - Add PHPStan config to ignore optional statistics module errors - Fix type safety: add ContentEntityInterface check in listEntities() - Fix DI: inject date.formatter service in StatisticsPlugin
jjroelofs
commented
Jan 28, 2026
Contributor
Author
jjroelofs
left a comment
There was a problem hiding this comment.
Code Review: Address multiple improvement issues (#2, #3, #4)
This PR cleanly addresses three issues with well-structured code. Looks good to merge.
Issue #2: Interface for ContentIntelCollector ✅
- Complete interface with all public methods documented
- Proper PHPDoc with return type descriptions
loadEntities()method correctly added to interface
Issue #3: Silent Exception Handling ✅
- Logger properly injected via container factory
- Uses appropriate
warninglevel - Adds
metadata_errorfield to output - nice touch for debugging
Issue #4: Batch Optimization ✅
loadMultiple()used correctly for bulk loading- Empty array guard prevents unnecessary DB calls
- Both code paths optimized (IDs provided vs listing)
Bonus Fixes 👍
- Fixed static
\Drupal::service('date.formatter')inStatisticsPlugin - Added
ContentEntityInterfacetype check inlistEntities() - PHPStan ignore patterns are reasonable for Drupal-specific idioms
Minor Suggestions (Non-blocking)
1. Entity order preservation in loadEntities()
loadMultiple() returns entities keyed by ID but not necessarily in input order. If order matters:
$result = [];
foreach ($entity_ids as $id) {
if (isset($entities[$id]) && $entities[$id] instanceof ContentEntityInterface) {
$result[$id] = $entities[$id];
}
}
return $result;Not critical since most consumers just iterate.
2. getPlugins() return structure (line 128 of interface)
The PHPDoc could document the expected array structure more explicitly, similar to how listEntities() mentions "Array of entity summaries."
Summary
Clean implementation that follows Drupal best practices. Code is well-structured, properly typed, and addresses all three issues as designed.
- Preserve input order in loadEntities() method - Document getPlugins() return structure in interface PHPDoc
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
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
Linked Issues
Changes
ContentIntelCollectorInterface (#2)
loadEntities()method for bulk entity loadingContentTranslationPlugin logging (#3)
metadata_errorfield to output for debuggingBatch command optimization (#4)
loadEntities()for bulk loading instead of individual loadsloadMultiple()callTest Plan
drush ci:pluginsto verify module loads correctlydrush ci:batch node --bundle=article --limit=10to test batch optimization