DGI9-617: Fix up logger of user switching.#31
Conversation
adam-vessey
changed the title
WalkthroughAdds Drush 11.6+ service mapping in composer; introduces a new drush.11.6-12.5.yml compatibility file; injects a dedicated logger service into the Drush command via services and constructor; tightens function signatures and simplifies batch op wrapping logic in the module. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant D as Drush CLI
participant C as Drupal Container
participant UWC as UserWrapperCommands
participant L as logger.islandora_drush_utils
participant M as islandora_drush_utils.module
U->>D: Invoke user-wrapper command
D->>C: Resolve command service (Drush 11.6+ mapping)
note right of C: drush.11.6-12.5.yml<br/>service compatibility
C-->>L: Get logger service
C-->>UWC: Instantiate with logger
UWC->>M: Prepare batch ops (wrapped)
M-->>UWC: Static-wrapped callable returned
UWC->>D: Execute batch (logs via L)
D-->>U: Output result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (5)
composer.json(1 hunks)drush.10.services.yml(1 hunks)drush.11.6-12.5.yml(1 hunks)islandora_drush_utils.module(4 hunks)src/Drush/Commands/UserWrapperCommands.php(4 hunks)
🔇 Additional comments (10)
drush.11.6-12.5.yml (1)
1-6: LGTM! Correct structure for Drush 11.6+ compatibility.The empty services mapping aligns with the documented shift to
create()and autowiring-based instantiation in newer Drush versions.composer.json (1)
21-21: LGTM! Correct Drush service mapping.The new entry properly registers the compatibility file for Drush 11.6+. Note that for Drush 11.6-11.x, both service files will be loaded (drush.10.services.yml and drush.11.6-12.5.yml), which is correct for the transition.
islandora_drush_utils.module (3)
11-11: LGTM! Improved type safety.The explicit array type hint and void return type improve function contract clarity and enable better static analysis.
57-77: LGTM! Improved clarity with explicit conditionals.The refactored namer logic uses separate if blocks instead of elseif chains, making each case more explicit and easier to maintain. The empty lines between cases enhance readability.
79-98: LGTM! Performance optimization with static closure.Making
wrap_opstatic and simplifying the return logic are both good optimizations. Static closures don't bind$this, which is appropriate here since only theusevariables are needed.src/Drush/Commands/UserWrapperCommands.php (4)
12-13: LGTM! Correct imports for explicit dependency injection.The added imports support the transition from
LoggerAwareTraitto explicit DI withAutowireTraitand theAutowireattribute, enabling better control over logger injection.Also applies to: 16-16
27-29: LGTM! Appropriate DI approach for Drush compatibility.The shift to
ContainerInjectionInterfacewithAutowireTraitprovides dual compatibility:
- The
create()method supports Drush 11.6+- The
Autowireattribute supports Drush 12.5+ autowiringThis maintains backward compatibility while enabling newer instantiation patterns.
43-50: LGTM! Explicit logger injection.The logger is properly injected via constructor with an
Autowireattribute binding to the service. This replaces the previousLoggerAwareTraitmagic with explicit, type-safe dependency injection.
55-62: LGTM! Consistent logger service retrieval.The
create()method correctly fetches the same logger service that theAutowireattribute references, ensuring consistency across both instantiation approaches.drush.10.services.yml (1)
31-31: Custom logger service is correctly defined.
The servicelogger.islandora_drush_utilsis declared in islandora_drush_utils.services.yml (parent: logger.channel_base), so using@logger.islandora_drush_utilsis appropriate.
| * Wrap batch op with user. | ||
| */ | ||
| function _islandora_drush_utils_user_wrapped_batch_op($id, $info, &$context) { | ||
| function _islandora_drush_utils_user_wrapped_batch_op(int|string $id, array $info, array &$context) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify PHP version supports union types.
The union type int|string requires PHP 8.0+. Please confirm the project's minimum PHP version meets this requirement.
Run the following script to check the PHP version constraint:
🏁 Script executed:
#!/bin/bash
# Description: Check minimum PHP version in composer.json
# Check for PHP version requirement
rg -n '"php":\s*"[^"]*"' composer.json
# Also check for any platform requirements
rg -n -A 5 '"platform"' composer.jsonLength of output: 0
🏁 Script executed:
#!/bin/bash
# Locate composer.json files and check PHP version constraint
find . -type f -name 'composer.json' -print | while read file; do
echo "File: $file"
rg -n '"php":\s*"[^"]*"' "$file"
rg -n -A 5 '"platform"' "$file"
doneLength of output: 21
🏁 Script executed:
#!/bin/bash
# Print the top of composer.json to locate PHP version requirement
head -n 200 composer.jsonLength of output: 683
Declare PHP 8.0+ as a requirement
The int|string union type needs PHP 8.0 or newer—add "php": ">=8.0" to your composer.json (or confirm your Drupal core constraint already enforces ≥8.0).
🤖 Prompt for AI Agents
In islandora_drush_utils.module around line 110 the function uses the PHP 8.0+
union type "int|string", which requires your project to declare PHP >=8.0;
update composer.json to include "php": ">=8.0" in the "require" section (or
verify that your project's Drupal core or platform constraint already enforces
PHP >=8.0) and run composer update/composer validate to ensure constraints are
consistent; if you cannot raise the PHP requirement, replace the union type with
a compatible declaration (e.g., omit the typehint and validate inside the
function) instead.
3 participants
Summary by CodeRabbit