[wp-trac] [WordPress Trac] #26823: wp_get_image_editor->multi_resize()

[WordPress Trac]

#26823: wp_get_image_editor->multi_resize()
--------------------------+------------------
 Reporter:  pbearne       |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  3.9
Component:  Media         |     Version:  3.5
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:
--------------------------+------------------

Comment (by pbearne):

 Replying to [comment:46 DH-Shredder]:
 > Sorry it took me a while to get back to this -- I'd planned on posting
 another patch, but instead I'll give a couple suggestions:
 > - On the new test, I'd rather not see us counting on the number of files
 actually generated being correct.  We're on the edge on "unit tests" as
 far as generating files at all, and I'd suggest just checking that the
 array output by `multi_resize()` is empty.  Part of the reason here is
 parallelism of tests, leading to:
 > - It's probably better if we avoid writing out files with the same name
 for different tests (GD vs Imagick) -- if they're run in parallel, there
 are going to be conflicts.
 > - It seems there are some code standards/comment/formatting changes that
 were lost between the patch I posted and the one you attached.  The most
 important one is required if-statement brackets in the primary (non-test)
 set of changes. If you'd like to handle making things consistent, that's
 fine.  If you'd rather I do, I can port your test additions and post
 another patch.
 >
 > On your point regarding changing 0/0 behaviour, I'd think it's proper to
 expect no output if you don't provide a width or height for a resize
 function.

 Removed the file count

 I have created 2 new test files (imagick_waffles.jpg / gd_waffles.jpg) for
 re-size tests will o solve the conflicts.

 Is this OK?

 If we want to go all in we could copy the base file to random name pass at
 the start of the test and pass into the arrays and delete at the end, but
 this a bit of a rewrite as we should do it for all tests :-)

 I have pulled your patch and re-edited it to added the new tests so I hope
 this now OK


 I hear you about the 0/0 behavior if this was added we should/would need
 to use a keyword (MAX) so that is clear what is happing, It does feel
 strange that this part of a "resize" function so maybe it should in a
 max_square_crop() function but this is code creep so this is not good
 either.

 So as there isn't a pressing usecase/need for this we should leave it
 alone.

 Paul

--
Ticket URL: <https://core.trac.wordpress.org/ticket/26823#comment:47>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform