SUP-7425: add script for deleting media and files #6
Conversation
📝 WalkthroughWalkthroughAdds two new Drush PHP scripts: Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator (Drush)
participant Script as delete_media_file(.php)
participant Islandora as Islandora Utils
participant Drupal as Drupal Entity API
participant DB as Database
Operator->>Script: invoke with comma-separated file IDs
Script->>Drupal: File::load(fileId)
alt file not found
Drupal-->>Script: null
Script->>Operator: log warning (file not found)
else file found
Drupal-->>Script: File entity
Script->>Islandora: find referencing Media for File
Islandora-->>Script: list of Media IDs
alt multiple referencing media (>1)
Script->>Operator: log warning and skip file
else single or multiple handled for deletion
loop for each media
Script->>Drupal: load Media entity
Drupal-->>Script: Media entity
alt dry-run
Script->>Operator: log "would delete media {id}"
else execute
Script->>Drupal: delete Media entity
Drupal-->>DB: remove media record
Drupal-->>Script: deletion confirmation
Script->>Operator: log media deletion
end
end
alt dry-run
Script->>Operator: log "would delete file {id}"
else execute
Script->>Drupal: delete File entity
Drupal-->>DB: remove file record
Drupal-->>Script: file deletion confirmation
Script->>Operator: log file deletion
end
end
end
Script->>Operator: processing complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 5
🧹 Nitpick comments (1)
scripts/delete_media_file.php (1)
38-55: Consider transaction handling and structured logging.The script performs destructive operations without transaction handling. If a media deletion fails partway through the loop, the file may still be deleted, leaving an inconsistent state. Additionally, consider using Drupal's logger service instead of
echostatements for better observability in production environments.This is an optional enhancement for improved reliability and observability:
- Wrap the deletion logic in a database transaction that can be rolled back on failure.
- Replace
echostatements with\Drupal::logger('delete_media_file')->info()for structured logging.
📜 Review details
Configuration used: defaults
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 (1)
scripts/delete_media_file.php
🧰 Additional context used
🪛 GitHub Actions: Code Linting
scripts/delete_media_file.php
[warning] 10-10: PHPCS warning: Unused use statement. (Drupal.Classes.UnusedUseStatement.UnusedUse)
[error] 11-11: PHPCS error: There must be one blank line after the last USE statement; 0 found. (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)
[error] 16-16: PHPCS error: Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses. (Drupal.Commenting.InlineComment.InvalidEndChar)
[error] 17-17: PHPCS error: Use null coalesce operator instead of ternary operator. (SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator.NullCoalesceOperatorNotUsed)
[error] 17-17: PHPCS error: Use null coalesce operator instead of ternary operator. (SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator.NullCoalesceOperatorNotUsed)
[error] 53-53: PHPCS error: Line indented incorrectly; expected 2 spaces, found 4. (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
[error] 54-54: PHPCS error: Line indented incorrectly; expected 2 spaces, found 4. (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
[error] 57-57: PHPCS error: Line indented incorrectly; expected 0 spaces, found 2. (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @scripts/delete_media_file_dry_run.php:
- Line 48: The warning message echo that prints "WARNING: File {$fid} has more
than one media referenceing it, please investigate further. Skipping...\n"
contains a typo; update the string to use "referencing" instead of
"referenceing" so the echo reads "WARNING: File {$fid} has more than one media
referencing it, please investigate further. Skipping...\n".
- Around line 47-56: Cache the result of $utils->getReferencingMedia($fid) into
a local variable (e.g., $referencing = $utils->getReferencingMedia($fid)) and
use $referencing for the count() check and the foreach loop instead of calling
getReferencingMedia() twice; update the conditional that currently uses
count($utils->getReferencingMedia($fid)) to use count($referencing) and iterate
over $referencing to echo the media IDs and avoid redundant queries.
🧹 Nitpick comments (1)
scripts/delete_media_file_dry_run.php (1)
25-34: Consider validating that file IDs are numeric.While
File::load()may handle non-numeric values gracefully, adding explicit validation would make errors clearer and prevent potential issues.🔎 Proposed validation enhancement
// Remove empty values. $fids = array_filter($fids); + +// Validate that all values are numeric. +$fids = array_filter($fids, 'is_numeric'); if (empty($fids)) { echo "Error: No valid file IDs provided.\n"; exit(1); }
📜 Review details
Configuration used: defaults
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 (2)
scripts/delete_media_file.phpscripts/delete_media_file_dry_run.php
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/delete_media_file.php
🔇 Additional comments (6)
scripts/delete_media_file_dry_run.php (6)
1-8: LGTM!The file header and usage documentation are clear and accurate.
14-23: LGTM!The argument parsing correctly handles both Drush command formats and provides clear error messaging.
36-37: LGTM!Clear messaging indicating the dry-run mode and file count.
38-45: LGTM!The file loading and null check are appropriate, with clear warning messages for missing files.
59-61: LGTM!Clear completion message.
10-12: islandora.utils service usage is correct.The
islandora.utilsservice andgetReferencingMedia()method are provided by the Islandora module, which is a declared requirement in README.md. The method calls at lines 47 and 51 properly pass the file ID and iterate over the returned media objects. No issues found.
| if (count($utils->getReferencingMedia($fid)) > 1) { | ||
| echo "WARNING: File {$fid} has more than one media referenceing it, please investigate further. Skipping...\n"; | ||
| continue; | ||
| } | ||
| foreach ($utils->getReferencingMedia($fid) as $media) { | ||
| if ($media) { | ||
| echo "Dry run - prod run will delete media ID: " . $media->id() . "\n"; | ||
| } | ||
| } | ||
| echo "Dry run - prod run will delete file ID: " . $file->id() . "\n"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Optimize by caching the result of getReferencingMedia().
The script calls getReferencingMedia($fid) twice: once on Line 47 to check the count and again on Line 51 to iterate. This is inefficient and could impact performance, especially if the method involves database queries.
🔎 Proposed optimization
- if (count($utils->getReferencingMedia($fid)) > 1) {
+ $referencing_media = $utils->getReferencingMedia($fid);
+ if (count($referencing_media) > 1) {
echo "WARNING: File {$fid} has more than one media referencing it, please investigate further. Skipping...\n";
continue;
}
- foreach ($utils->getReferencingMedia($fid) as $media) {
+ foreach ($referencing_media as $media) {
if ($media) {
echo "Dry run - prod run will delete media ID: " . $media->id() . "\n";
}
}📝 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.
| if (count($utils->getReferencingMedia($fid)) > 1) { | |
| echo "WARNING: File {$fid} has more than one media referenceing it, please investigate further. Skipping...\n"; | |
| continue; | |
| } | |
| foreach ($utils->getReferencingMedia($fid) as $media) { | |
| if ($media) { | |
| echo "Dry run - prod run will delete media ID: " . $media->id() . "\n"; | |
| } | |
| } | |
| echo "Dry run - prod run will delete file ID: " . $file->id() . "\n"; | |
| $referencing_media = $utils->getReferencingMedia($fid); | |
| if (count($referencing_media) > 1) { | |
| echo "WARNING: File {$fid} has more than one media referencing it, please investigate further. Skipping...\n"; | |
| continue; | |
| } | |
| foreach ($referencing_media as $media) { | |
| if ($media) { | |
| echo "Dry run - prod run will delete media ID: " . $media->id() . "\n"; | |
| } | |
| } | |
| echo "Dry run - prod run will delete file ID: " . $file->id() . "\n"; |
🤖 Prompt for AI Agents
In @scripts/delete_media_file_dry_run.php around lines 47 - 56, Cache the result
of $utils->getReferencingMedia($fid) into a local variable (e.g., $referencing =
$utils->getReferencingMedia($fid)) and use $referencing for the count() check
and the foreach loop instead of calling getReferencingMedia() twice; update the
conditional that currently uses count($utils->getReferencingMedia($fid)) to use
count($referencing) and iterate over $referencing to echo the media IDs and
avoid redundant queries.
| } | ||
|
|
||
| if (count($utils->getReferencingMedia($fid)) > 1) { | ||
| echo "WARNING: File {$fid} has more than one media referenceing it, please investigate further. Skipping...\n"; |
There was a problem hiding this comment.
Fix the typo in the warning message.
Line 48 contains a typo: "referenceing" should be "referencing".
🔎 Proposed fix
- echo "WARNING: File {$fid} has more than one media referenceing it, please investigate further. Skipping...\n";
+ echo "WARNING: File {$fid} has more than one media referencing it, please investigate further. Skipping...\n";📝 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.
| echo "WARNING: File {$fid} has more than one media referenceing it, please investigate further. Skipping...\n"; | |
| echo "WARNING: File {$fid} has more than one media referencing it, please investigate further. Skipping...\n"; |
🤖 Prompt for AI Agents
In @scripts/delete_media_file_dry_run.php at line 48, The warning message echo
that prints "WARNING: File {$fid} has more than one media referenceing it,
please investigate further. Skipping...\n" contains a typo; update the string to
use "referencing" instead of "referenceing" so the echo reads "WARNING: File
{$fid} has more than one media referencing it, please investigate further.
Skipping...\n".
|
no longer needed.. closing pr |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.