Skip to content

Conversation

@MiljenkoR
Copy link

This PR is to address #95

Related wp-cli/wp-cli#5955

Looking to update test to cover new code

@MiljenkoR MiljenkoR requested a review from a team as a code owner June 13, 2024 12:08
@codecov
Copy link

codecov bot commented Mar 31, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/Media_Command.php 75.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@swissspidy swissspidy linked an issue Dec 2, 2025 that may be closed by this pull request
Copy link

Copilot AI left a 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.

Comment on lines +724 to +726
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 ) );
Copy link

Copilot AI Feb 3, 2026

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.

Suggested change
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() ) );

Copilot uses AI. Check for mistakes.
Comment on lines +724 to +726
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 ) );
Copy link

Copilot AI Feb 3, 2026

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add reason for skipping regeneration

3 participants