Restore backup#119
Draft
tijmenbruggeman wants to merge 20 commits into
Draft
Conversation
Replace <form method="dialog"> with div + command/commandfor attributes, aligning with the Invoker Commands API. Cancel no longer auto-closes via form submission; both buttons now use `command="close"` explicitly. JS listener simplified: drop named handler + self-removal, check for `returnValue === 'submit'` instead of `'confirm'`.
Clearer dialog title/body split, added spinner, descriptive confirmation text, and fixed HTML nesting.
Contributor
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds original-image backup lifecycle methods, authenticated admin restoration, backup controls in compression details, cleanup on attachment deletion, and unit coverage for restoration and metadata behavior. ChangesImage backup restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant admin.js
participant restore_backup_image
participant Tiny_Image
participant WordPressFilesystem
AdminUI->>admin.js: Confirm backup restoration
admin.js->>restore_backup_image: POST tiny_restore_backup with attachment ID
restore_backup_image->>Tiny_Image: restore_backup()
Tiny_Image->>WordPressFilesystem: Copy backup over original
restore_backup_image-->>admin.js: Return compression details
admin.js-->>AdminUI: Replace media container
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/class-tiny-image.php`:
- Around line 720-772: Handle exceptions from Tiny_Helpers::get_wp_filesystem()
in all backup methods: create_backup(), get_backup(), restore_backup(), and
delete_backup(). Wrap each filesystem initialization in try/catch and return
false when initialization fails, preserving normal behavior when it succeeds so
media rendering and compression degrade gracefully.
- Around line 774-825: Update restore_backup() to reuse get_original_image() for
the restored original image instead of duplicating the
ORIGINAL_UNSCALED/ORIGINAL lookup. After wp_generate_attachment_metadata()
succeeds, rebuild $this->sizes from the new attachment metadata so
get_image_sizes() and get_image_size() immediately reflect regenerated, added,
removed, or renamed thumbnails; retain the existing metadata update and success
behavior.
In `@src/js/admin.js`:
- Around line 22-64: Move the restore workflow from the dialog’s cancel listener
to the confirm button’s click handler, so Escape only dismisses the dialog and
never calls restoreBackup. In the relevant admin.js handler, explicitly remove
any previously registered confirm handler before adding or assigning the new
one, rather than relying on { once: true }, so cancelling and reopening cannot
accumulate listeners. Update the confirm button markup to use type="button" and
remove command="request-close" and commandfor.
In `@src/views/compress-details-backup.php`:
- Around line 18-49: Fix the conditional markup around the backup view in the
template: remove the paragraph wrapper that contains the flow-content dialog,
ensure the no-backup branch emits no orphaned closing tag, and keep the dialog
as valid sibling markup. Also update the “original upload” translation call to
use esc_html_e() without echo, add the tiny-compress-images text domain to “View
uncompressed file,” and use esc_url() for the $backup href.
In `@src/views/compress-details.php`:
- Around line 116-121: Update the modal_title construction and Thickbox name
attribute around $modal_title to escape the translated title exactly once for
its HTML attribute context. Keep the filename unescaped while formatting the
translated string, then rely on esc_attr() at output without changing the
displayed title or surrounding link behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a9aa22b-2fe7-40ef-b27c-4d843c3c1e32
📒 Files selected for processing (9)
src/class-tiny-image.phpsrc/class-tiny-plugin.phpsrc/css/admin.csssrc/js/admin.jssrc/views/compress-details-backup.phpsrc/views/compress-details.phptest/helpers/wordpress.phptest/unit/TinyPluginBackupTest.phptest/wp-includes-for-tests/file.php
Comment on lines
+720
to
+772
| /** | ||
| * Creates a backup copy of the original image, if one does not already exist. | ||
| * | ||
| * @return bool true on success, false on failure or if a backup already exists | ||
| */ | ||
| public function create_backup() { | ||
|
|
||
| $backup_file_path = $this->get_backup_path(); | ||
| if ( false === $backup_file_path ) { | ||
| return false; | ||
| } | ||
|
|
||
| $wp_filesystem = Tiny_Helpers::get_wp_filesystem(); | ||
|
|
||
| if ( $wp_filesystem->exists( $backup_file_path ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $backup_dir = dirname( $backup_file_path ); | ||
|
|
||
| if ( ! wp_mkdir_p( $backup_dir ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $original_image = $this->get_original_image(); | ||
|
|
||
| return $wp_filesystem->copy( $original_image->filename, $backup_file_path ); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Retrieves the public URL of the backup of the original image. | ||
| * | ||
| * @return string|false the backup URL, or false if no backup exists | ||
| */ | ||
| public function get_backup() { | ||
| $backup_file_path = $this->get_backup_path(); | ||
| if ( false === $backup_file_path ) { | ||
| return false; | ||
| } | ||
|
|
||
| $wp_filesystem = Tiny_Helpers::get_wp_filesystem(); | ||
|
|
||
| if ( ! $wp_filesystem->exists( $backup_file_path ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $upload_dir = wp_upload_dir(); | ||
| $basedir = trailingslashit( $upload_dir['basedir'] ); | ||
| $baseurl = trailingslashit( $upload_dir['baseurl'] ); | ||
|
|
||
| return str_replace( $basedir, $baseurl, $backup_file_path ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled Tiny_Helpers::get_wp_filesystem() exception on hot paths.
get_wp_filesystem() throws when WP_Filesystem() can't connect (e.g. environments without direct filesystem access, requiring FTP credentials):
if ( ! ( $wp_filesystem instanceof WP_Filesystem_Base ) ) {
throw new Exception( 'Unable to initialize WordPress filesystem.' );
}None of the new backup methods (create_backup, get_backup, restore_backup, delete_backup) catch this. get_backup() in particular runs on every admin media-list render for any compressed image (via compress-details-backup.php → render_media_column()), so on such environments the whole media list would fatal instead of degrading gracefully. create_backup() is invoked synchronously from the tiny_image_before_compression action, so the same failure would abort compression entirely.
Consider wrapping these calls in try/catch and failing soft (return false) instead of letting the exception propagate into page rendering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/class-tiny-image.php` around lines 720 - 772, Handle exceptions from
Tiny_Helpers::get_wp_filesystem() in all backup methods: create_backup(),
get_backup(), restore_backup(), and delete_backup(). Wrap each filesystem
initialization in try/catch and return false when initialization fails,
preserving normal behavior when it succeeds so media rendering and compression
degrade gracefully.
Comment on lines
+774
to
+825
| /** | ||
| * Restores the original image from its backup. | ||
| * | ||
| * - Copies the backup file over the current original. | ||
| * - Clears compression metadata for all image sizes. | ||
| * - Regenerates all thumbnail sizes from the restored image. | ||
| * - Updates the WordPress attachment metadata. | ||
| * | ||
| * @since 3.7.0 | ||
| * | ||
| * @return bool True on success, false if no backup exists or the copy fails. | ||
| */ | ||
| public function restore_backup() { | ||
| $backup_file_path = $this->get_backup_path(); | ||
| if ( false === $backup_file_path ) { | ||
| return false; | ||
| } | ||
|
|
||
| $wp_filesystem = Tiny_Helpers::get_wp_filesystem(); | ||
|
|
||
| if ( ! $wp_filesystem->exists( $backup_file_path ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $original_size_key = null !== $this->get_image_size( self::ORIGINAL_UNSCALED ) | ||
| ? self::ORIGINAL_UNSCALED | ||
| : self::ORIGINAL; | ||
|
|
||
| $original_image = $this->get_image_size( $original_size_key ); | ||
| if ( null === $original_image ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( ! $wp_filesystem->copy( $backup_file_path, $original_image->filename, true ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Clear compression metadata for all image sizes. | ||
| foreach ( $this->sizes as $size ) { | ||
| $size->meta = array(); | ||
| } | ||
| $this->update_tiny_post_meta(); | ||
|
|
||
| // Regenerate all thumbnail sizes from the restored image. | ||
| $new_metadata = wp_generate_attachment_metadata( $this->id, $original_image->filename ); | ||
| if ( $new_metadata ) { | ||
| $this->wp_metadata = $new_metadata; | ||
| wp_update_attachment_metadata( $this->id, $this->wp_metadata ); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale $this->sizes after restore — renders incorrect details immediately after restoring.
After the backup is copied over and wp_generate_attachment_metadata() regenerates thumbnails, $this->wp_metadata is updated, but $this->sizes (used by get_image_sizes()/get_image_size()) is never rebuilt from the new metadata — only the .meta compression-stats field is cleared on the existing (potentially stale) Tiny_Image_Size objects. Any newly generated/removed thumbnail sizes, or filenames that changed as a result of the regenerated image, won't be reflected.
This directly affects Tiny_Plugin::restore_backup_image() (src/class-tiny-plugin.php:987-997), which immediately calls render_compress_details() on this same instance — the AJAX response the user sees right after clicking "Yes, Restore" can show a stale/incorrect size list until the page is reloaded.
Also, lines 798-802 re-implement the exact original-size lookup already provided by get_original_image() (lines 685-696) — worth reusing instead of duplicating.
🐛 Proposed fix
- $original_size_key = null !== $this->get_image_size( self::ORIGINAL_UNSCALED )
- ? self::ORIGINAL_UNSCALED
- : self::ORIGINAL;
-
- $original_image = $this->get_image_size( $original_size_key );
- if ( null === $original_image ) {
- return false;
- }
+ $original_image = $this->get_original_image();
+ if ( false === $original_image ) {
+ return false;
+ }
if ( ! $wp_filesystem->copy( $backup_file_path, $original_image->filename, true ) ) {
return false;
}
// Clear compression metadata for all image sizes.
foreach ( $this->sizes as $size ) {
$size->meta = array();
}
$this->update_tiny_post_meta();
// Regenerate all thumbnail sizes from the restored image.
$new_metadata = wp_generate_attachment_metadata( $this->id, $original_image->filename );
if ( $new_metadata ) {
$this->wp_metadata = $new_metadata;
wp_update_attachment_metadata( $this->id, $this->wp_metadata );
+ $this->sizes = array();
+ $this->parse_wp_metadata();
}
return true;Unit test verifies restore_backup() overwrites the original attachment file from the tinify_backup copy., but does not assert the sizes list is consistent afterwards, so this gap isn't currently caught.
📝 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.
Suggested change
| /** | |
| * Restores the original image from its backup. | |
| * | |
| * - Copies the backup file over the current original. | |
| * - Clears compression metadata for all image sizes. | |
| * - Regenerates all thumbnail sizes from the restored image. | |
| * - Updates the WordPress attachment metadata. | |
| * | |
| * @since 3.7.0 | |
| * | |
| * @return bool True on success, false if no backup exists or the copy fails. | |
| */ | |
| public function restore_backup() { | |
| $backup_file_path = $this->get_backup_path(); | |
| if ( false === $backup_file_path ) { | |
| return false; | |
| } | |
| $wp_filesystem = Tiny_Helpers::get_wp_filesystem(); | |
| if ( ! $wp_filesystem->exists( $backup_file_path ) ) { | |
| return false; | |
| } | |
| $original_size_key = null !== $this->get_image_size( self::ORIGINAL_UNSCALED ) | |
| ? self::ORIGINAL_UNSCALED | |
| : self::ORIGINAL; | |
| $original_image = $this->get_image_size( $original_size_key ); | |
| if ( null === $original_image ) { | |
| return false; | |
| } | |
| if ( ! $wp_filesystem->copy( $backup_file_path, $original_image->filename, true ) ) { | |
| return false; | |
| } | |
| // Clear compression metadata for all image sizes. | |
| foreach ( $this->sizes as $size ) { | |
| $size->meta = array(); | |
| } | |
| $this->update_tiny_post_meta(); | |
| // Regenerate all thumbnail sizes from the restored image. | |
| $new_metadata = wp_generate_attachment_metadata( $this->id, $original_image->filename ); | |
| if ( $new_metadata ) { | |
| $this->wp_metadata = $new_metadata; | |
| wp_update_attachment_metadata( $this->id, $this->wp_metadata ); | |
| } | |
| return true; | |
| } | |
| /** | |
| * Restores the original image from its backup. | |
| * | |
| * - Copies the backup file over the current original. | |
| * - Clears compression metadata for all image sizes. | |
| * - Regenerates all thumbnail sizes from the restored image. | |
| * - Updates the WordPress attachment metadata. | |
| * | |
| * `@since` 3.7.0 | |
| * | |
| * `@return` bool True on success, false if no backup exists or the copy fails. | |
| */ | |
| public function restore_backup() { | |
| $backup_file_path = $this->get_backup_path(); | |
| if ( false === $backup_file_path ) { | |
| return false; | |
| } | |
| $wp_filesystem = Tiny_Helpers::get_wp_filesystem(); | |
| if ( ! $wp_filesystem->exists( $backup_file_path ) ) { | |
| return false; | |
| } | |
| $original_image = $this->get_original_image(); | |
| if ( false === $original_image ) { | |
| return false; | |
| } | |
| if ( ! $wp_filesystem->copy( $backup_file_path, $original_image->filename, true ) ) { | |
| return false; | |
| } | |
| // Clear compression metadata for all image sizes. | |
| foreach ( $this->sizes as $size ) { | |
| $size->meta = array(); | |
| } | |
| $this->update_tiny_post_meta(); | |
| // Regenerate all thumbnail sizes from the restored image. | |
| $new_metadata = wp_generate_attachment_metadata( $this->id, $original_image->filename ); | |
| if ( $new_metadata ) { | |
| $this->wp_metadata = $new_metadata; | |
| wp_update_attachment_metadata( $this->id, $this->wp_metadata ); | |
| $this->sizes = array(); | |
| $this->parse_wp_metadata(); | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/class-tiny-image.php` around lines 774 - 825, Update restore_backup() to
reuse get_original_image() for the restored original image instead of
duplicating the ORIGINAL_UNSCALED/ORIGINAL lookup. After
wp_generate_attachment_metadata() succeeds, rebuild $this->sizes from the new
attachment metadata so get_image_sizes() and get_image_size() immediately
reflect regenerated, added, removed, or renamed thumbnails; retain the existing
metadata update and success behavior.
Comment on lines
+22
to
+64
| const dialog = document.getElementById(dialogID); | ||
| if (!dialog) { | ||
| return; | ||
| } | ||
|
|
||
| const attachmentId = trigger.data('id'); | ||
| const container = document.querySelector(`[data-tiny-media-id="${attachmentId}"]`); | ||
|
|
||
| dialog.showModal(); | ||
|
|
||
| dialog.addEventListener('cancel', async (e) => { | ||
| e.preventDefault(); | ||
| const spinner = dialog.querySelector('.spinner'); | ||
| try { | ||
| if (spinner) { | ||
| spinner.style.visibility = 'visible'; | ||
| } | ||
| const result = await restoreBackup(attachmentId); | ||
| dialog.close(); | ||
|
|
||
| // refresh thickbox | ||
| const modal = container.querySelector('.modal'); | ||
| const ajaxContent = document.getElementById('TB_ajaxContent'); | ||
| if (modal && ajaxContent) { | ||
| modal.append(...ajaxContent.children); | ||
| } | ||
|
|
||
| container.innerHTML = result; | ||
| if (typeof tb_remove === 'function') { | ||
| tb_remove(); | ||
| } | ||
| } catch (err) { | ||
| const errorEl = dialog.querySelector('.tiny-dialog-error'); | ||
| if (errorEl) { | ||
| errorEl.textContent = err.responseText || 'Failed to restore backup.'; | ||
| errorEl.hidden = false; | ||
| } | ||
| } finally { | ||
| if (spinner) { | ||
| spinner.style.visibility = 'hidden'; | ||
| } | ||
| } | ||
| }, { once: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Pressing Escape (or any dialog-close-request) triggers the destructive restore action.
The confirm ("Yes, Restore") button uses command="request-close". Per spec, that command fires a cancel event on the dialog — the same event that fires when the user presses the Escape key to dismiss the dialog. Since the only listener attached is on dialog's cancel event, a user pressing Escape to back out of the confirmation will inadvertently execute the restore (overwriting all compressed sizes), not cancel it.
Separately, a new one-time cancel listener is added on every click of "Restore Backup" (line 32), but it's never removed if the dialog is dismissed via the "Cancel" button (command="close", which does not fire cancel). Re-opening the same dialog (same DOM node, since only a successful restore replaces container.innerHTML) after a prior cancel therefore stacks multiple listeners — a subsequent confirm click would fire restoreBackup() once per accumulated listener.
Bind the confirm action to the button's own click instead of overloading the shared cancel event, and don't rely on { once: true } alone to prevent duplicate handlers.
🐛 Proposed fix
dialog.showModal();
- dialog.addEventListener('cancel', async (e) => {
- e.preventDefault();
- const spinner = dialog.querySelector('.spinner');
+ const confirmButton = dialog.querySelector('button[value="submit"]');
+ if (!confirmButton) {
+ return;
+ }
+
+ const onConfirm = async () => {
+ const spinner = dialog.querySelector('.spinner');
try {
if (spinner) {
spinner.style.visibility = 'visible';
}
const result = await restoreBackup(attachmentId);
dialog.close();
// refresh thickbox
const modal = container.querySelector('.modal');
const ajaxContent = document.getElementById('TB_ajaxContent');
if (modal && ajaxContent) {
modal.append(...ajaxContent.children);
}
container.innerHTML = result;
if (typeof tb_remove === 'function') {
tb_remove();
}
} catch (err) {
const errorEl = dialog.querySelector('.tiny-dialog-error');
if (errorEl) {
errorEl.textContent = err.responseText || 'Failed to restore backup.';
errorEl.hidden = false;
}
} finally {
if (spinner) {
spinner.style.visibility = 'hidden';
}
}
- }, { once: true });
+ };
+
+ confirmButton.addEventListener('click', onConfirm, { once: true });
});This also requires dropping command="request-close"/commandfor from the confirm button in src/views/compress-details-backup.php (lines 39-41) in favor of a plain type="button", so it no longer triggers the dialog's cancel event at all.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 48-48: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: container.innerHTML = result
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/admin.js` around lines 22 - 64, Move the restore workflow from the
dialog’s cancel listener to the confirm button’s click handler, so Escape only
dismisses the dialog and never calls restoreBackup. In the relevant admin.js
handler, explicitly remove any previously registered confirm handler before
adding or assigning the new one, rather than relying on { once: true }, so
cancelling and reopening cannot accumulate listeners. Update the confirm button
markup to use type="button" and remove command="request-close" and commandfor.
Comment on lines
+18
to
+49
| <?php if ( $backup ) { ?> | ||
| <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | ||
|
|
||
| <p> | ||
| <a href="<?php echo esc_attr( $backup ); ?>" target="_blank"> | ||
| <?php esc_html_e( 'View uncompressed file' ); ?> | ||
| </a> | ||
| | ||
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | ||
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | ||
| </a> | ||
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | ||
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | ||
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | ||
| <p class="tiny-dialog-error" hidden></p> | ||
|
|
||
| <div class="tiny-dialog-actions"> | ||
| <span class="spinner"></span> | ||
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | ||
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | ||
| </button> | ||
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | ||
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | ||
| </button> | ||
| </div> | ||
| </dialog> | ||
| <?php } else { ?> | ||
| <span> | ||
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | ||
| </span> | ||
| <?php } ?> | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unbalanced <p>/<dialog> markup in the "no backup" branch, plus a couple of WP-conventions nits.
- The
<p>opened at line 21 only exists inside theif ( $backup )branch, but its closing</p>at line 49 is emitted unconditionally after theif/else. When$backupis falsy, this leaves an orphaned</p>with no matching opener. - Separately, a
<dialog>element (flow content) is nested inside that<p>(which only permits phrasing content) — browsers will auto-close the<p>before the<dialog>, so the actual DOM won't match the source nesting. - Line 19:
echo esc_html_e(...)—esc_html_e()already echoes and returns nothing; wrapping it inechois redundant/incorrect usage. - Line 23:
esc_html_e( 'View uncompressed file' )is missing the'tiny-compress-images'text domain, so this string won't be picked up for translation. - Line 22:
esc_attr( $backup )for ahref— WordPress convention isesc_url()for URL attributes.
🛠️ Proposed fix
<?php if ( $backup ) { ?>
- <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p>
-
+ <p><?php esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p>
+
<p>
- <a href="<?php echo esc_attr( $backup ); ?>" target="_blank">
- <?php esc_html_e( 'View uncompressed file' ); ?>
+ <a href="<?php echo esc_url( $backup ); ?>" target="_blank">
+ <?php esc_html_e( 'View uncompressed file', 'tiny-compress-images' ); ?>
</a>
<a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>">
<?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?>
</a>
+ </p>
+ <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog">
+ ...
+ </dialog>
+ <?php } else { ?>
+ <p>
+ <span>
+ <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?>
+ </span>
+ </p>
+ <?php } ?>📝 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.
Suggested change
| <?php if ( $backup ) { ?> | |
| <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | |
| <p> | |
| <a href="<?php echo esc_attr( $backup ); ?>" target="_blank"> | |
| <?php esc_html_e( 'View uncompressed file' ); ?> | |
| </a> | |
| | |
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | |
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | |
| </a> | |
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | |
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | |
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | |
| <p class="tiny-dialog-error" hidden></p> | |
| <div class="tiny-dialog-actions"> | |
| <span class="spinner"></span> | |
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | |
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | |
| </button> | |
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | |
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | |
| </button> | |
| </div> | |
| </dialog> | |
| <?php } else { ?> | |
| <span> | |
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | |
| </span> | |
| <?php } ?> | |
| </p> | |
| <?php if ( $backup ) { ?> | |
| <p><?php esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | |
| <p> | |
| <a href="<?php echo esc_url( $backup ); ?>" target="_blank"> | |
| <?php esc_html_e( 'View uncompressed file', 'tiny-compress-images' ); ?> | |
| </a> | |
| | |
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | |
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | |
| </a> | |
| </p> | |
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | |
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | |
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | |
| <p class="tiny-dialog-error" hidden></p> | |
| <div class="tiny-dialog-actions"> | |
| <span class="spinner"></span> | |
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | |
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | |
| </button> | |
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | |
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | |
| </button> | |
| </div> | |
| </dialog> | |
| <?php } else { ?> | |
| <p> | |
| <span> | |
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | |
| </span> | |
| </p> | |
| <?php } ?> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/views/compress-details-backup.php` around lines 18 - 49, Fix the
conditional markup around the backup view in the template: remove the paragraph
wrapper that contains the flow-content dialog, ensure the no-backup branch emits
no orphaned closing tag, and keep the dialog as valid sibling markup. Also
update the “original upload” translation call to use esc_html_e() without echo,
add the tiny-compress-images text domain to “View uncompressed file,” and use
esc_url() for the $backup href.
Comment on lines
+116
to
+121
| <?php | ||
| } | ||
| /* translators: %s is the image filename */ | ||
| $modal_title = sprintf( esc_html__( 'Compression details for %s', 'tiny-compress-images' ), esc_html( $tiny_image->get_name() ) ); | ||
| ?> | ||
| <a class="thickbox message" name="<?php echo esc_attr( $modal_title ); ?>" href="#TB_inline?width=700&height=500&inlineId=modal_<?php echo absint( $tiny_image->get_id() ); ?>"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Double-escaping in the Thickbox name attribute.
$modal_title is built with esc_html()/esc_html__() and then escaped again with esc_attr() when output — for filenames containing &, <, > or quotes this double-encodes entities (e.g. & → & → &amp;), showing garbled text in the Thickbox title.
🐛 Proposed fix
/* translators: %s is the image filename */
- $modal_title = sprintf( esc_html__( 'Compression details for %s', 'tiny-compress-images' ), esc_html( $tiny_image->get_name() ) );
+ $modal_title = sprintf( __( 'Compression details for %s', 'tiny-compress-images' ), $tiny_image->get_name() );
?>
<a class="thickbox message" name="<?php echo esc_attr( $modal_title ); ?>" href="`#TB_inline`?width=700&height=500&inlineId=modal_<?php echo absint( $tiny_image->get_id() ); ?>">📝 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.
Suggested change
| <?php | |
| } | |
| /* translators: %s is the image filename */ | |
| $modal_title = sprintf( esc_html__( 'Compression details for %s', 'tiny-compress-images' ), esc_html( $tiny_image->get_name() ) ); | |
| ?> | |
| <a class="thickbox message" name="<?php echo esc_attr( $modal_title ); ?>" href="#TB_inline?width=700&height=500&inlineId=modal_<?php echo absint( $tiny_image->get_id() ); ?>"> | |
| <?php | |
| } | |
| /* translators: %s is the image filename */ | |
| $modal_title = sprintf( __( 'Compression details for %s', 'tiny-compress-images' ), $tiny_image->get_name() ); | |
| ?> | |
| <a class="thickbox message" name="<?php echo esc_attr( $modal_title ); ?>" href="`#TB_inline`?width=700&height=500&inlineId=modal_<?php echo absint( $tiny_image->get_id() ); ?>"> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/views/compress-details.php` around lines 116 - 121, Update the
modal_title construction and Thickbox name attribute around $modal_title to
escape the translated title exactly once for its HTML attribute context. Keep
the filename unescaped while formatting the translated string, then rely on
esc_attr() at output without changing the displayed title or surrounding link
behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Style