Skip non-thumbnailed PDFS & other images rather than fail. Check retu… #48
Conversation
.travis.yml
Outdated
| - php: 5.6 | ||
| env: WP_VERSION=3.9 | ||
| - php: 5.6 | ||
| env: WP_VERSION=4.2 |
There was a problem hiding this comment.
This will be overwritten the next time we run wp scaffold package-tests.
Rather than run the entire test suite for these versions, can we use a Scenario Outline to run the same tests against multiple WP versions?
There was a problem hiding this comment.
Yes, much better solution, will do.
There was a problem hiding this comment.
Unfortunately I can't see how to do this currently, without adding a new "Given a WP version X install" type step, so I'll just leave the extra WP versions for the mo, at least until just before merge.
There was a problem hiding this comment.
Here's how I'd test behavior on a specific WP version:
Given a WP install
And I run `wp core download --version=<version> --force`
There was a problem hiding this comment.
Ah very good, will do.
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.
You can upload them and then they will magically be available: https://github.com/wp-cli/wp-cli.github.com/tree/master/behat-data |
src/Media_Command.php
Outdated
| } | ||
|
|
||
| Utils\report_batch_operation_results( 'image', 'regenerate', count( $images->posts ), $successes, $errors ); | ||
| self::report_batch_operation_results( 'image', 'regenerate', $count, $successes, $errors, $skips ); |
There was a problem hiding this comment.
Rather than fork this utility, let's instead add a $skips parameter to Utils\report_batch_operation_results(). We can pass the $skips value through, and it will display if WP-CLI is up to date (or degrade gracefully if not).
There was a problem hiding this comment.
Okay, I'll open a PR on wp-cli for it.
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. |
Skip non-thumbnailed PDFS & other images rather than fail. Check retu…
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, $skipsrather 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$skipsarg 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 theUtilsversion instead.A default behaviour test
media-regenerate.feature:3has been added. Also tests for SVGmedia-regenerate.feature:847, PDF with/without thumbnailsmedia-regenerate.feature:988etc, audio/video with/without cover artmedia-regenerate.feature:1125etc, and all togethermedia-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.