DGI9-593: Perform user swapping during batch invocation.#33
Conversation
📝 WalkthroughWalkthroughAdmin context switching for fixity operations is introduced, where the admin user (ID 1) is loaded, account context is switched to perform fixity operations, then switched back using a finally block. Error handling added for admin load and view load failures. The batch_size parameter is cast to integer and its public signature relaxed to accept null values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
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: 3
🤖 Fix all issues with AI agents
In `@src/FixityCheckBatchCheck.php`:
- Around line 134-137: When the admin account fails to load in
FixityCheckBatchCheck (i.e., $admin_user is not an instance of
AccountInterface), set $context['finished'] = TRUE before returning so the batch
runner does not retry indefinitely; update the failure branch that currently
calls $logger->error('Failed to load the admin account to switch.') to also
assign $context['finished'] = TRUE (and then return) so the operation is marked
finished.
- Around line 28-29: build() currently casts the configured batch size to int
which can yield 0 or negative and thus create a limit:0; validate $batch_size
after casting (from SettingsForm::CONFIG_NAME / SettingsForm::BATCH_SIZE or the
passed $batch_size) and enforce a sensible minimum (e.g. if ((int)$batch_size <
1) $batch_size = <default>), ensuring $batch_size is at least 1 before it is
used by FixityCheckBatchCheck::build and any view query.
- Around line 147-150: When the view fails to load in FixityCheckBatchCheck (the
branch where if (!$view) { ... return; }), set $context['finished'] to a
completed value before returning to avoid Drupal batch retries; e.g. assign
$context['finished'] = 1 (or true) and optionally record an error in
$context['results'] (e.g. increment an errors counter or add a message) so the
batch marks the operation finished and the failure is visible in logs; update
the early-return branch that references $view and $logger to perform these
assignments before returning.
| public static function build(?array $fids = NULL, bool $force = FALSE, ?int $batch_size = NULL) { | ||
| $batch_size = is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size; | ||
| $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size); |
There was a problem hiding this comment.
Consider adding validation for zero/negative batch_size.
If the config value is missing or invalid, the (int) cast could result in batch_size = 0, which would cause the view to return no results (due to limit: 0) and operations to complete immediately without processing anything.
🛡️ Suggested validation
- $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size);
+ $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size);
+ if ($batch_size <= 0) {
+ throw new \InvalidArgumentException("Batch size must be a positive integer.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function build(?array $fids = NULL, bool $force = FALSE, ?int $batch_size = NULL) { | |
| $batch_size = is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size; | |
| $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size); | |
| public static function build(?array $fids = NULL, bool $force = FALSE, ?int $batch_size = NULL) { | |
| $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size); | |
| if ($batch_size <= 0) { | |
| throw new \InvalidArgumentException("Batch size must be a positive integer."); | |
| } |
🤖 Prompt for AI Agents
In `@src/FixityCheckBatchCheck.php` around lines 28 - 29, build() currently casts
the configured batch size to int which can yield 0 or negative and thus create a
limit:0; validate $batch_size after casting (from SettingsForm::CONFIG_NAME /
SettingsForm::BATCH_SIZE or the passed $batch_size) and enforce a sensible
minimum (e.g. if ((int)$batch_size < 1) $batch_size = <default>), ensuring
$batch_size is at least 1 before it is used by FixityCheckBatchCheck::build and
any view query.
| * elapsed since the last check has not exceeded the required threshold. | ||
| * @param int|null $batch_size | ||
| * The number of files to process at a time. | ||
| * If not specified it will default to the module's configuration. | ||
| */ | ||
| public static function build(?array $fids = NULL, bool $force = FALSE, ?int $batch_size = NULL) { | ||
| $batch_size = is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size; | ||
| $batch_size = (int) (is_null($batch_size) ? \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) : $batch_size); |
There was a problem hiding this comment.
Few fixes that static analysis/my IDE was flagging that caught my attention.
| $user_storage = \Drupal::entityTypeManager()->getStorage('user'); | ||
| $admin_user = $user_storage->load(1); | ||
| if (!($admin_user instanceof AccountInterface)) { | ||
| $logger->error('Failed to load the admin account to switch.'); |
There was a problem hiding this comment.
Unsure if this would ever be seen, given oddities with batches run via Drush and logs, but given this may be used outside of Drush, could make sense?
Should be a rare error condition, in any case.
MorganDawe
deleted the
fix/controlled-file-enumeration-via-user-switch
branch
|
Tag generated by PR: v1.4.3 |
2 participants
Alternate approach to #32 , since it was realized that we already do similar things elsewhere.
Summary by CodeRabbit
Release Notes