Skip to content
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

regenerate command runs on non-image files #25

Closed
mrwweb opened this issue Jun 21, 2017 · 12 comments
Closed

regenerate command runs on non-image files #25

mrwweb opened this issue Jun 21, 2017 · 12 comments

Comments

@mrwweb
Copy link

mrwweb commented Jun 21, 2017

When running this:

$ wp media regenerate --yes

The command runs on ALL attachments, regardless of file type.

This results in lots of awkward errors when run on audio, video, PDFs, etc. Here's an example:

...
Warning: 23/23 Couldn't regenerate thumbnails for "2012 Guest gallery proposal guidelines" (ID 20).
Error: Only regenerated 16 of 23 images.

In the above example, the media library contains 16 images and 7 non-image files.

It would seem wp regenerate media is naturally intended only for images and so it would make sense to only run it on a whitelist of filetypes. This would also avoid issues like this one.

@danielbachhuber
Copy link
Member

It would seem wp regenerate media is naturally intended only for images and so it would make sense to only run it on a whitelist of filetypes.

This makes sense to me, although we'd probably want to verify that it wasn't intentionally implemented this way.

Also, PDF should be a part of the whitelist, because it has thumbnails as of 4.7.

@mrwweb
Copy link
Author

mrwweb commented Jun 21, 2017

Also, PDF should be a part of the whitelist, because it has thumbnails as of 4.7.

That makes sense, though many servers don't generate the image, so there might need to be a check.

Another option would be to change the status message to something like

Attachment skipped: Not an image file (ID 20)

and

All 16 images regenerated. (7 non-images skipped.)

I'm not sure what the overall wp-cli way is on stuff like this (autoskip vs. clear messages), but I think either would be an improvement.

@danielbachhuber
Copy link
Member

I'm not sure what the overall wp-cli way is on stuff like this (autoskip vs. clear messages), but I think either would be an improvement.

Yes, this latter suggestion seems more appropriate than changing the current behavior.

@schlessera
Copy link
Member

Currently there seems to be 4 different paths to generate thumbnails:

  1. Image
preg_match( '!^image/!', $mime_type )
    && file_is_displayable_image( $file )
  1. Video
wp_attachment_is( 'video', $attachment )
    && ( current_theme_supports( 'post-thumbnails', 'attachment:video' )
        || post_type_supports( 'attachment:video', 'thumbnail' ) )
    && ! empty( $metadata['image']['data']
  1. Audio
wp_attachment_is( 'audio', $attachment )
    && ( current_theme_supports( 'post-thumbnails', 'attachment:audio' )
        || post_type_supports( 'attachment:audio', 'thumbnail' ) )
    && ! empty( $metadata['image']['data']
  1. PDF
'application/pdf' === $mime_type

As these might still change in the future, I wonder if it is not preferable to just rely on the WordPress functionality and simply mark all files as skipped that didn't produce a thumbnail.

@mrwweb
Copy link
Author

mrwweb commented Jun 24, 2017

The skipping approach makes a lot of sense as long as there continues to be a real error when regenerating fails.

@schlessera
Copy link
Member

Yes, that seems to be the main issue. I'm not sure there's a clean way to distinguish between these two cases if we only rely on WordPress functionality.

@RobertGres
Copy link

Also a huge problem is that if script stumbles upon non-image file, it generates an error, effectively terminating process after batch completion. This makes multisite application of this command impossible, since there are lots non-image attachments on each blog (and I'm talking hundreds of blogs)

I see that problem is in function Utils\report_batch_operation_results, but maybe you can create some sort of multisite vatiation for this command, which will report results only after all blogs were processed?

@diggy
Copy link
Contributor

diggy commented Aug 16, 2017

Ran into this issue, single site install but with a composite bash script containing various wp cli commands, command exiting with error due to PDFs:

Error: Only regenerated 359 of 394 images.

@danielbachhuber
Copy link
Member

To address this issue, I'd like to see:

  1. wp media regenerate adapted to only regenerate thumbnails for attachments that support thumbnails (however we determine that, e.g. checking mime).
  2. A core Trac ticket opened to create an explicit API for determining whether an attachment supports thumbnails. In the future, we can depend on this API instead of our own heuristics.

@tlehtimaki
Copy link

tlehtimaki commented Aug 30, 2017

Could this be partially solved with a flag something like --extensions=jpeg,jpg,png,gif or --onlyimages=true etc. it could be used to include only media matching the wanted extensions?

@danielbachhuber
Copy link
Member

In alignment with "Decisions, not options", I'd prefer to improve the default behavior instead of adding more options.

@gitlost
Copy link
Contributor

gitlost commented Oct 2, 2017

I was working on this over the weekend and have a PR which I'll push, as it's probably easier to discuss with code. It also addresses #43.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants