Set the default mapping for jpeg->webp. #35
Conversation
eg. cat.jpeg, cat.jpg and cat.jpe (all valid jpegs) generate the same file names before this change, eg. cat-150x150.webp. adding the original file’s extension as a suffix on the generated filename neatly avoids this.
There was a problem hiding this comment.
@adamsilverstein My main feedback here is what I mentioned earlier, I think it would be more efficient to handle the output format in a separate function hooked into the filter.
In addition to that, we should then also add tests to this PR:
- For the new filter callback function.
- For the re-introduced logic to ensure a unique file name (we can probably reuse a test from the reverted commit for that).
|
Added a test for |
…-additional-changes
| @@ -67,6 +67,7 @@ public function testCropImageThumbnail() { | |||
| */ | |||
| public function testImageEditOverwriteConstant() { | |||
| define( 'IMAGE_EDIT_OVERWRITE', true ); | |||
| $files_that_shouldnt_exist = array(); | |||
There was a problem hiding this comment.
this test was failing with $files_that_shouldnt_exist undefined below (https://github.com/adamsilverstein/wordpress-develop-fork/pull/35/files#diff-7e3d8ea591299762db2be94e68100b267611327378a1e94a8fbe0cc1666538adL106), I'm going to debug why a little further
There was a problem hiding this comment.
@adamsilverstein A bit more feedback about the filter callback function, mostly minor stuff. We should have a bit more test coverage though.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
…com:adamsilverstein/wordpress-develop-fork into add/new-webp-approach-br-additional-changes
|
I figured out why the |
…-additional-changes
There was a problem hiding this comment.
@adamsilverstein Almost good to go now, except one tiny comment below, plus more tests like I mentioned in https://github.com/adamsilverstein/wordpress-develop-fork/pull/35/files#r962033794 still need to be added.
|
Updates:
Tests pass locally, rerunning across envs in WordPress#3162 |
…en WebP not supported
There was a problem hiding this comment.
@adamsilverstein LGTM, except for a few comments on the one test.
tests/phpunit/tests/media.php
Outdated
|
|
||
| if ( $editor->supports_mime_type( 'image/webp' ) ) { | ||
| $this->assertSame( 'image/webp', $saved['mime-type'] ); | ||
| $this->assertSame( 'canola-100x75.webp', $saved['file'] ); |
There was a problem hiding this comment.
Shouldn't this result in canola-100x75-jpg.webp? Why is that extra original extension suffix missing here? Is that a potential problem we need to fix?
There was a problem hiding this comment.
I looked into this a little. I think it is because of the way the test is structured with generate_filename called with no arguments. Our code in this function relies on $extension being passed, which is what happens when core generates subsized images. I'll see if we can improve this and also test the core calls to create the -scaled and -rotated names which may not be adding the additional suffix, but probably should.
There was a problem hiding this comment.
Turns out this was happening because of the way the test was structured. By first generating the filename, then passing that to save the code was bypassing the for altered extension - that only happens when you don't pass a filename to save, see https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-image-editor-imagick.php#L717-L719
Fixed this in c0367e1 by altering the test slightly, calling save without first generating the filename. Essentially if you pass a filename to save, it uses that filename, that is a requirement for being able to overwrite files.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
|
Once tests pass I'm going to commit so the feature lands before beta. we can address any edge case issues raised around file naming or add more tests during the beta. |
|
merged, closing |
See WordPress#3161