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

Skip non-thumbnailed PDFS & other images rather than fail. Check retu… #48

Merged
merged 6 commits into from Oct 12, 2017

Conversation

2 participants
@gitlost
Contributor

gitlost commented Oct 2, 2017

Fixes #25
Fixes #43

For discussion.

Skips rather than fails when PDF thumbnails are not available or when images such as SVGs are encountered.

Some re-factoring is involved re changing process_regeneration() to take extra args $successes, $errors, $skips rather than return a boolean success/fail, and bailing at various stages incrementing one of them.

But the main functionality change is checking the return values of WP core functions re #43, and acting accordingly.

Utils\report_batch_operation_results() has been cloned and adapted locally to add a $skips arg and to report errors as failures in the return string, which is probably good elsewhere but would lead to breaking tests probably and maybe BC concerns (though can we assume its value isn't being parsed in command pipes?). Anyway it might make sense to change the Utils version instead.

A default behaviour test media-regenerate.feature:3 has been added. Also tests for SVG media-regenerate.feature:847, PDF with/without thumbnails media-regenerate.feature:988 etc, audio/video with/without cover art media-regenerate.feature:1125 etc, and all together media-regenerate.feature:1222.

Edit should add that test audio and video files have temporarily been put on my server pending upload to wp-cli.org.

Show outdated Hide outdated .travis.yml Outdated
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Oct 4, 2017

Member

For discussion.

Is there specific feedback you're looking for?

There's a lot of code churn, so it's difficult to comment on the diff directly. However, my historic opinion of this code is that it's been a mess, so I welcome the work cleaning it up. I feel reasonably confident in the test coverage around the functionality, so not terribly concerned about breakage.

Edit should add that test audio and video files have temporarily been put on my server pending upload to wp-cli.org.

You can upload them and then they will magically be available: https://github.com/wp-cli/wp-cli.github.com/tree/master/behat-data

Member

danielbachhuber commented Oct 4, 2017

For discussion.

Is there specific feedback you're looking for?

There's a lot of code churn, so it's difficult to comment on the diff directly. However, my historic opinion of this code is that it's been a mess, so I welcome the work cleaning it up. I feel reasonably confident in the test coverage around the functionality, so not terribly concerned about breakage.

Edit should add that test audio and video files have temporarily been put on my server pending upload to wp-cli.org.

You can upload them and then they will magically be available: https://github.com/wp-cli/wp-cli.github.com/tree/master/behat-data

Show outdated Hide outdated src/Media_Command.php Outdated
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Oct 4, 2017

Contributor

Is there specific feedback you're looking for?

Apart from that given, just whether the behaviour as here implemented was what was expected/desired. But looking back over the discussion in issue #25, I suppose it is.

Contributor

gitlost commented Oct 4, 2017

Is there specific feedback you're looking for?

Apart from that given, just whether the behaviour as here implemented was what was expected/desired. But looking back over the discussion in issue #25, I suppose it is.

@danielbachhuber danielbachhuber added this to the 1.1.1 milestone Oct 12, 2017

@danielbachhuber danielbachhuber merged commit 630747d into master Oct 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the issue_43 branch Oct 12, 2017

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