-
Notifications
You must be signed in to change notification settings - Fork 39
Add skip verbosity for PDF and empty metadata #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates wp media regenerate skip reporting to make it clearer why certain attachments are not regenerated, specifically targeting PDFs and attachments with empty metadata (per media-command issue #95).
Changes:
- Adds informational log output when regeneration is skipped for PDF attachments in the “no editor available” path.
- Adds informational log output when regeneration is skipped and attachment metadata is empty/non-array in the same “no editor available” path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WP_CLI::log( sprintf( 'Skipping PDF file (ID %d)', $att_id ) ); | ||
| } elseif ( ! is_array( $metadata ) || empty( $metadata ) ) { | ||
| WP_CLI::log( sprintf( 'No metadata (ID %d)', $att_id ) ); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new log lines are emitted specifically in the image_no_editor WP_Error branch, but the messages don’t mention the actual reason ($image_sizes->get_error_message() / no editor available). In particular, “Skipping PDF file” implies all PDFs are skipped, and “No metadata” isn’t the cause of skipping here. Consider logging an info-level message that includes the underlying image_no_editor reason (and optionally the file type/metadata state) so the output accurately explains why regeneration was skipped.
| WP_CLI::log( sprintf( 'Skipping PDF file (ID %d)', $att_id ) ); | |
| } elseif ( ! is_array( $metadata ) || empty( $metadata ) ) { | |
| WP_CLI::log( sprintf( 'No metadata (ID %d)', $att_id ) ); | |
| WP_CLI::log( sprintf( 'Skipping PDF file (ID %d) due to: %s', $att_id, $image_sizes->get_error_message() ) ); | |
| } elseif ( ! is_array( $metadata ) || empty( $metadata ) ) { | |
| WP_CLI::log( sprintf( 'Skipping file with no metadata (ID %d) due to: %s', $att_id, $image_sizes->get_error_message() ) ); |
| WP_CLI::log( sprintf( 'Skipping PDF file (ID %d)', $att_id ) ); | ||
| } elseif ( ! is_array( $metadata ) || empty( $metadata ) ) { | ||
| WP_CLI::log( sprintf( 'No metadata (ID %d)', $att_id ) ); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds new user-visible skip reason output (PDF + empty metadata), but there are existing Behat scenarios in features/media-regenerate.feature for skipped SVG/PDF regeneration that don’t assert these new messages. Please add/extend a scenario to cover the new log lines so regressions in skip-reason reporting are caught.
This PR is to address #95
Related wp-cli/wp-cli#5955
Looking to update test to cover new code