Repository navigation
SEC-2816: Fix/avoid tracked dependencies - #25
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (2)
📝 WalkthroughWalkthroughThe PR extends method signatures across dependency resolvers, export managers, and forms to accept and propagate an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 21 seconds.Comment |
adam-vessey
changed the title
…e time. Still requires all of any particular type in an array; however, such should still be smaller than the combined array.
Should reduce the risk of malfunction should other module not be updated to provide 'batch_info' accordingly.
Analogous to the previous `!empty($entities_list)` checks.
Had overlooked the different value being provided in the two locations.
| else { | ||
| $activeStorage = new ContentDatabaseStorage(\Drupal::database(), 'cs_db_snapshot'); | ||
| $entity = $activeStorage->cs_read($identifier); | ||
| $this->activeStorage ??= new ContentDatabaseStorage(\Drupal::database(), 'cs_db_snapshot'); |
There was a problem hiding this comment.
Minor optimization; only instantiating once per instance.
... could possibly go further, to wrap in a service? But yeah, let's not get too crazy in terms of refactoring.
There was a problem hiding this comment.
The snapshotting side, where it was building out the huge array, which was turned into a generator (and DRY'd up slightly).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/DependencyResolver/ImportQueueResolver.php (1)
117-120: ⚡ Quick winForward
serializer_contextfrom the import entry point.
resolve()now advertisesarray $serializer_context = [], but it still drops that value before the first DFS call. That leaves the new parameter as dead API and makes the import resolver diverge fromExportQueueResolver::resolve(), which already forwards its context.♻️ Suggested change
public function resolve(array $normalized_entities, $visited = [], array $serializer_context = []) { $visited = []; foreach ($normalized_entities as $identifier => $entity) { - $this->depthFirstSearch($visited, [$identifier], $normalized_entities); + $this->depthFirstSearch($visited, [$identifier], $normalized_entities, $serializer_context); } // Reverse the array to adjust it to an array_pop-driven iterator. return array_reverse($visited); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DependencyResolver/ImportQueueResolver.php` around lines 117 - 120, The resolve() method in ImportQueueResolver drops the passed $serializer_context and resets $visited before calling depthFirstSearch, so the new parameter is unused; update resolve() to stop discarding $serializer_context and forward it into depthFirstSearch (and any downstream calls) similar to ExportQueueResolver::resolve(), and avoid reinitializing $visited unnecessarily so depthFirstSearch($visited, [$identifier], $normalized_entities, $serializer_context) (or the appropriate parameter order used by depthFirstSearch) receives the context.
🤖 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/Drush/Commands/ContentSyncCommands.php`:
- Around line 441-444: The current code always sets 'batch_info' to
['uuids'=>[], 'entity_types'=>[]] which makes
ContentExportTrait::processContentExportFiles() think exports are filtered;
update the assembly of the payload in ContentSyncCommands.php so that
'batch_info' is only included when at least one of $options['uuids'] or
$options['entity-types'] is provided (i.e., build $batch_info only if those
arrays are non-empty and otherwise omit the 'batch_info' key entirely);
reference the existing 'batch_info' array in this diff and ensure the change
preserves behavior for the filtered export path and lets --include-dependencies
run for unfiltered exports.
In `@src/Form/ContentExportTrait.php`:
- Around line 244-256: The code reads
$serializer_context['include_dependencies'] directly which can be undefined in
ContentExportForm::submitForm() and snapshot(), causing notices; update the
conditional in ContentExportTrait to guard that access (use
!empty($serializer_context['include_dependencies']) or isset+truthy check)
before evaluating the rest of the expression so the branch only runs when
include_dependencies is present and truthy, leaving the remaining checks
(batch_info, uuids, entity_types) unchanged.
---
Nitpick comments:
In `@src/DependencyResolver/ImportQueueResolver.php`:
- Around line 117-120: The resolve() method in ImportQueueResolver drops the
passed $serializer_context and resets $visited before calling depthFirstSearch,
so the new parameter is unused; update resolve() to stop discarding
$serializer_context and forward it into depthFirstSearch (and any downstream
calls) similar to ExportQueueResolver::resolve(), and avoid reinitializing
$visited unnecessarily so depthFirstSearch($visited, [$identifier],
$normalized_entities, $serializer_context) (or the appropriate parameter order
used by depthFirstSearch) receives the context.
🪄 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: 48c43212-bb8d-4f08-84cf-45f7c505fa11
📒 Files selected for processing (7)
src/ContentSyncManager.phpsrc/DependencyResolver/ContentSyncResolverInterface.phpsrc/DependencyResolver/ExportQueueResolver.phpsrc/DependencyResolver/ImportQueueResolver.phpsrc/Drush/Commands/ContentSyncCommands.phpsrc/Form/ContentExportForm.phpsrc/Form/ContentExportTrait.php
|
Tag generated by PR: v3.1.2 |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Performance Improvements