SUP-6848: Misc fixes #27
Conversation
Also, ensure that we switch back to the original account consistently.
Was all fixable via phpcbf.
WalkthroughCron now deletes and recreates the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cron
participant Queue as dgi_fixity.process_source
participant SourceProvider
Cron->>Queue: deleteQueue()
Cron->>Queue: createQueue()
Cron->>SourceProvider: enumerate sources
loop per source
Cron->>Queue: createItem(source)
end
sequenceDiagram
autonumber
participant Worker as ProcessSourceWorker
participant AccSwitch as AccountSwitcher
participant Views as ViewLoader
participant Fixity as FixityService
participant QueueAPI as Queue API
Worker->>AccSwitch: switchTo(account)
alt account invalid
Worker->>AccSwitch: (finally) switchBack()
Worker-->>Worker: return
else account valid
rect rgba(230,245,255,0.6)
Worker->>Views: load view
alt view not found
Worker->>AccSwitch: (finally) switchBack()
Worker-->>Worker: return
else view found
Worker->>Fixity: source(view, chunkSize)
Worker->>Views: iterate rows
loop per row
Worker->>Fixity: mark periodic on check entity
end
alt needs requeue
Worker->>QueueAPI: throw RequeueException
end
end
end
Worker->>AccSwitch: finally switchBack()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Plugin/Action/FixityCheckActionBase.php (1)
76-83: Nullability introduced but$accountis dereferenced without a guard
$accountcan now be NULL, yet$account->hasPermission()is called unconditionally. This will fatally error when invoked without an account (e.g., from a queue/CLI context).Apply this diff to guard null and preserve the original semantics (default to the current user when none is provided):
- public function access($object, ?AccountInterface $account = NULL, $return_as_object = FALSE) { - // If it exists and the user has permission to administer fixity checks. - $result = $this->getCheck($object) && $account->hasPermission('administer fixity checks') ? - AccessResult::allowed() : - AccessResult::forbidden(); - - return $return_as_object ? $result : $result->isAllowed(); - } + public function access($object, ?AccountInterface $account = NULL, $return_as_object = FALSE) { + // Default to current user if no account was provided. + $account = $account ?? \Drupal::currentUser(); + $check = $this->getCheck($object); + + $result = ($check && $account->hasPermission('administer fixity checks')) + ? AccessResult::allowed() + : AccessResult::forbidden(); + + return $return_as_object ? $result : $result->isAllowed(); + }src/Form/RevisionDeleteForm.php (1)
101-106: buildForm allows NULL revision but later dereferences it; add a guard
$this->revisionis used without null checks (e.g., getQuestion(), submitForm()). IfbuildFormreceives NULL, this will fatal. Throw 404 on missing revision to fail safely.Apply this diff:
- public function buildForm(array $form, FormStateInterface $form_state, ?FixityCheckInterface $fixity_check_revision = NULL) { - $this->revision = $fixity_check_revision; + public function buildForm(array $form, FormStateInterface $form_state, ?FixityCheckInterface $fixity_check_revision = NULL) { + if ($fixity_check_revision === NULL) { + throw new \Symfony\Component\HttpKernel\Exception\NotFoundHttpException(); + } + $this->revision = $fixity_check_revision; $form = parent::buildForm($form, $form_state); return $form; }src/Plugin/QueueWorker/ProcessSourceWorker.php (1)
114-121: Double accountSwitcher->switchBack() causes an unbalanced account stackYou call
switchBack()before throwingRequeueException, and then call it again infinally. This will unbalance the switcher stack and can trigger errors.Apply this diff to rely solely on the
finallyblock:// Not finished processing. if (count($view->result) !== 0) { - $this->accountSwitcher->switchBack(); throw new RequeueException(); } } finally { $this->accountSwitcher->switchBack(); }
🧹 Nitpick comments (5)
src/FixityCheckBatchCheck.php (1)
27-32: Nullable parameters accepted; ensure type safety and documentation match
- The config value for batch size may be a string; cast it to int to avoid accidental string math.
- Docblocks above still state non-null types; they should reflect
int[]|nullandint|null.Apply this diff for the cast:
- 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; + public static function build(?array $fids = NULL, bool $force = FALSE, ?int $batch_size = NULL) { + $batch_size = is_null($batch_size) + ? (int) \Drupal::config(SettingsForm::CONFIG_NAME)->get(SettingsForm::BATCH_SIZE) + : (int) $batch_size; return is_null($fids) ? static::buildPeriodic($force, $batch_size) : static::buildFixed($fids, $force, $batch_size); }Docblock suggestion (outside the changed hunk):
/** * @param int[]|null $fids * @param bool $force * @param int|null $batch_size */src/Plugin/QueueWorker/ProcessSourceWorker.php (3)
98-101: Prefer injected service over \Drupal::service for testability and consistencyYou already inject
FixityCheckServiceInterfaceas$this->fixity. Use it instead of the service locator.Apply this diff:
- /** @var \Drupal\dgi_fixity\FixityCheckServiceInterface $fixity */ - $fixity = \Drupal::service('dgi_fixity.fixity_check'); - $view = $fixity->source($data, 1000); + $view = $this->fixity->source($data, 1000);
101-105: Abort on missing view should be logged for observabilitySilently returning hides misconfiguration. Log a warning to aid operators.
Apply this diff:
if (!$view) { - // Failed to load view? Abort. - return; + // Failed to load view? Log and abort. + \Drupal::logger('dgi_fixity')->warning('ProcessSourceWorker: failed to load fixity view for source "@source".', ['@source' => $data]); + return; }
91-94: Early return when account #1 is missing; consider loggingIf user 1 doesn't exist or can't be loaded, the worker exits silently. Emit a log so site operators understand why nothing is processed.
Apply this diff:
- if (!($account instanceof AccountInterface)) { - return; - } + if (!($account instanceof AccountInterface)) { + \Drupal::logger('dgi_fixity')->warning('ProcessSourceWorker: unable to load account 1; skipping.'); + return; + }dgi_fixity.module (1)
122-125: Resetting the queue on each cron run: confirm operational intent and concurrency effectsCalling
deleteQueue()will purge all items, including any that were requeued but not yet processed. Given you immediately repopulate from configuration, this seems intentional to prevent duplicates, but it can drop in-flight requeues if cron overlaps worker execution. Verify this aligns with your operational expectations (especially on multi-webhead setups).Consider alternatives if you want to avoid wholesale purges:
- Deduplicate by using a side-channel (e.g., a keyed state or a custom “unique queue” table) so each source is enqueued at most once per window.
- Have the worker idempotently no-op on already-processed sources and leave cron to only enqueue new sources without deleting the queue.
- If you keep the reset, document that this queue is ephemeral and rebuilt each cron run; this helps future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
dgi_fixity.module(1 hunks)src/Commands/FixityCheck.php(1 hunks)src/FixityCheckBatchCheck.php(1 hunks)src/Form/RevisionDeleteForm.php(1 hunks)src/Plugin/Action/FixityCheckActionBase.php(1 hunks)src/Plugin/QueueWorker/ProcessSourceWorker.php(2 hunks)
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/Plugin/QueueWorker/ProcessSourceWorker.php (1)
95-120: Protect switchBack() with a flag; use injected service; add exception logging.Prevents a possible LogicException if switchTo() fails mid-flight, removes service locator usage, and logs failures.
- try { - $this->accountSwitcher->switchTo($account); - - /** @var \Drupal\dgi_fixity\FixityCheckServiceInterface $fixity */ - $fixity = \Drupal::service('dgi_fixity.fixity_check'); - $view = $fixity->source($data, 1000); - if (!$view) { - // Failed to load view? Abort. - return; - } + $switched = FALSE; + try { + $this->accountSwitcher->switchTo($account); + $switched = TRUE; + + $view = $this->fixity->source($data, 1000); + if ($view === NULL) { + \Drupal::logger('dgi_fixity')->warning('ProcessSourceWorker: failed to load view for source "@source"; skipping.', ['@source' => (string) $data]); + return; + } $view->execute(); + if (!isset($view->field['periodic'])) { + \Drupal::logger('dgi_fixity')->error('ProcessSourceWorker: expected "periodic" field missing on view for source "@source"; skipping.', ['@source' => (string) $data]); + return; + } // Only processes those which have not already enabled periodic checks. foreach ($view->result as $row) { /** @var \Drupal\dgi_fixity\FixityCheckInterface $check */ $check = $view->field['periodic']->getEntity($row); $check->setPeriodic(TRUE); $check->save(); } // Not finished processing. if (count($view->result) !== 0) { throw new RequeueException(); } - } - finally { - $this->accountSwitcher->switchBack(); - } + } + catch (\Throwable $e) { + \Drupal::logger('dgi_fixity')->error('ProcessSourceWorker exception for source "@source": @message', [ + '@source' => (string) $data, + '@message' => $e->getMessage(), + ]); + throw $e; + } + finally { + if ($switched) { + $this->accountSwitcher->switchBack(); + } + }
🧹 Nitpick comments (2)
src/Plugin/QueueWorker/ProcessSourceWorker.php (2)
91-94: Log and avoid hardcoding uid 1 (consider configurability).Early-return is fine. Add a log entry for diagnostics, and consider making the admin UID configurable instead of hardcoding 1.
- if (!($account instanceof AccountInterface)) { - return; - } + if (!($account instanceof AccountInterface)) { + \Drupal::logger('dgi_fixity')->error('ProcessSourceWorker: failed to load administrative account (uid 1); skipping item.'); + return; + }
101-104: Nit: keep the early-return, but log the reason.The guard is good; logging improves traceability (covered in the previous diff).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Plugin/QueueWorker/ProcessSourceWorker.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Plugin/QueueWorker/ProcessSourceWorker.php (2)
src/FixityCheckServiceInterface.php (1)
source(94-94)src/FixityCheckService.php (1)
source(168-188)
Misc fixes:
Summary by CodeRabbit
Bug Fixes
New Features
Refactor