D11-11: Bump core version #35
Conversation
Drush 13 introduced assertions, expecting the use of a "logger manager". See: drush-ops/drush#5022
Was this command ever used? The missing `$context` seems to suggest no?
WalkthroughThis PR updates islandora_drush_utils to support Drupal 10 and 11, enforces Drush 13+ compatibility, refactors logger registration in two Drush command classes using conditional/lazy initialization, and extends batch processing in PublishUnpublishCollectionsDrushCommands with context tracking and access-control-disabled queries. Changes
Sequence DiagramsequenceDiagram
participant Batch as Batch API
participant updateStatusBatch as updateStatusBatch()
participant Query as Query Handler
participant Node/Media as Node/Media<br/>Entities
rect rgb(200, 220, 255)
Note over Batch,Node/Media: New: Context-aware Batch Processing
end
Batch->>updateStatusBatch: Invoke with &$context
updateStatusBatch->>updateStatusBatch: Track progress in $context<br/>(total, completed, finished)
updateStatusBatch->>Query: Build query with accessCheck(FALSE)
Query->>Node/Media: Fetch all items<br/>(ignoring access control)
Node/Media-->>Query: Return entities
Query-->>updateStatusBatch: Results
updateStatusBatch->>updateStatusBatch: Update batch status<br/>in $context
updateStatusBatch-->>Batch: Continue/finish
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
There was a problem hiding this comment.
This command never seems to have been used? The missing $context should've made rather a mess, and the missing accessCheck() invocations seem to suggest that this was never called in D10?
Could just drop this command/file?
There was a problem hiding this comment.
Looks like it was written back in 2023 and was a one time use command that the team deemed as potentially useful for the future. I'm indifferent on leaving it in
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drush/Commands/PublishUnpublishCollectionsDrushCommands.php (1)
184-188: Use PSR-3 logging for exceptions at line 185LoggerInterface has no exception() method. Change to error():
- $this->logger->exception('Encountered an exception: {exception}', [ - 'exception' => $e, - ]); + $this->logger->error('Encountered an exception during batch processing', [ + 'exception' => $e, + 'message' => $e->getMessage(), + ]);
🧹 Nitpick comments (3)
src/Drush/Commands/Sec873DrushCommands.php (1)
67-67: Logger wiring: add a safe fallback to keep $this->logger usableGuard aggregator usage and fall back to setLogger to ensure property logging continues to work.
- $this->logger()?->add('islandora_drush_utils.sec_873', $logger); + if (method_exists($this, 'logger') && ($agg = $this->logger())) { + $agg->add('islandora_drush_utils.sec_873', $logger); + } + else { + // Ensure PSR-3 logger remains available on $this->logger. + $this->setLogger($logger); + }Please confirm WrappedCommandVerbosityTrait provides logger()->add(...) and that $this->logger is a PSR-3 logger throughout this class.
src/Drush/Commands/PublishUnpublishCollectionsDrushCommands.php (2)
51-51: Logger wiring: apply the same safe fallback patternMirror the guarded fallback to preserve $this->logger if aggregator/helper is unavailable.
- $this->logger()?->add('islandora_drush_utils', $logger); + if (method_exists($this, 'logger') && ($agg = $this->logger())) { + $agg->add('islandora_drush_utils', $logger); + } + else { + $this->setLogger($logger); + }
105-105: Bypassing access checks: confirm intent and run contextUsing ->accessCheck(FALSE) ensures full coverage in CLI; confirm this is intended and only used by privileged operators. Consider a brief code comment to document intent.
Also applies to: 158-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
composer.json(1 hunks)islandora_drush_utils.info.yml(1 hunks)src/Drush/Commands/PublishUnpublishCollectionsDrushCommands.php(3 hunks)src/Drush/Commands/Sec873DrushCommands.php(1 hunks)
🔇 Additional comments (4)
islandora_drush_utils.info.yml (1)
5-5: Core requirement bump looks goodCompatibility updated to Drupal ^10 || ^11. No issues.
src/Drush/Commands/PublishUnpublishCollectionsDrushCommands.php (2)
100-104: Batch API: adding &$context is correctSignature and doc update fix prior batch breakage. Good change.
374-375: Review comment references incorrect file and suggests invalid fixThe review cites
PublishUnpublishCollectionsDrushCommands.phpat lines 374-375, but the env varISLANDORA_DRUSH_UTILS_SEC_783__DRY_RUNis actually inSec873DrushCommands.php:374. More critically, the suggested fix to change SEC_783 to SEC_873 is unfounded—SEC_873 does not exist anywhere in the codebase. The current code consistently uses SEC_783 in both the setter (Sec873DrushCommands.php:374) and getter (Sec873.php:66), indicating this is intentional, not a typo.Likely an incorrect or invalid review comment.
composer.json (1)
18-20: Review comment is based on incorrect constraint analysisThe verification reveals the review comment's core findings are incorrect:
"drush.10.services.yml is unreachable" — False. The constraint
^10 || ^11makes it reachable for Drush 10.x and 11.0–11.5. It's only unreachable for Drush 12.x+.">=11.6 also matches 13, so Drush 13 may load" — False. The
conflictdirective prevents Drush 13 from installing at all; no version resolution occurs for 13.x.Actual issue: For Drush 11.6–11.x, both
drush.11.6-12.5.ymlanddrush.10.services.ymlload simultaneously, creating a genuine overlap. The problem is not an unreachable file, but overlapping service definitions at a specific version range.The recommendation to add
<13to the>=11.6constraint is redundant (the top-level conflict already enforces this). If overlap at 11.6–11.x is a concern, the real fix is to constraindrush.10.services.ymlto exclude 11.6+, not to remove it entirely.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Looks like it was written back in 2023 and was a one time use command that the team deemed as potentially useful for the future. I'm indifferent on leaving it in
2 participants
Builds on/contains #34
Summary by CodeRabbit
Chores
Refactor