wp_get_image_editor->multi_resize()

[pbearne]

The example code shows passing null as a width/height

​http://bhoover.com/wp_image_editor-wordpress-image-editing-tutorial/

But the code does not support NULL's as it exits the function if both width/height are not set.

I have created a patch though I am not sure that is the right way to do this but it does work

All I have done is check for NULL and set it to PHP_INT_MAX-1 so that it is not sized on the axis

Paul

[pbearne]

Patch for multi_resize_null_support

[alconebay]

I've edited the tutorial at the link provided (bhoover.com) to use "9999" instead of "NULL", for now. I'll keep an eye on this ticket. Thanks!

This is the tutorial example pbearne is referring to before I changed it:

$sizes_array = array(
// #1 - resizes to 100x100 pixel, square-cropped image
array ('width' => 100, 'height' => 100, 'crop' => true),
// #2 - resizes to 200/100 pixel max width/height, non-cropped image
array ('width' => 200, 'height' => 100, 'crop' => false),
// #3 - resizes to 200 pixel max height, non-cropped image
array ('width' => NULL, 'height' => 200, 'crop' => false),
// #3 - resizes to 450 pixel max width, non-cropped image
array ('width' => 450, 'height' => NULL, 'crop' => false)
);
$resize = $img->multi_resize( $sizes_array );

[pbearne]

Replying to alconebay:
I believe a better change to the the example would be to use PHP_INT_MAX-1 as 9999 is a possible image size :-)

[kirasong]

Thanks for the ticket and patch!

You are correct in that width and height can't both be set to null, but current code does support one or the other set to null, so that the image is auto-sized according to its current ratio.

This is handled within image_resize_dimensions, and we could absolutely benefit from improved documentation both within the base WP_Image_Editor and extending (GD/Imagick) classes for clarification.

I'd suggest taking a look at the ​inline documentation standards for a few pointers on style for comments within core.

[alconebay]

Replying to DH-Shredder:

pbearne is referring to these lines:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-image-editor-gd.php#L208
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-image-editor-imagick.php#L275.

They kill the resize if either height or width are missing. Both have to be present for the resize to continue. Changing the condition on these lines from "AND" to "OR" allows the resize to continue with only one dimension present:

if ( ! ( isset( $size_data['width'] ) || isset( $size_data['height'] ) ) )

[pbearne]

Replying to alconebay:

I thought about changing the AND to OR but after looking at the later call's it looked like passing NULL to them would cause problems so I felt that change one null to the int_max and requiring other to be an INT was the best way to handle this.

I feel it right the the function allow NULL for unrestricted is the right call this feel right to me.

The Question is should all the re-sizes work this way as well?

Paul

[pbearne]

If it felt that all resize functions should allow a NULL and we are happy with the code I submitted

I can add a patch to make the change to the other calls

[markoheijnen]

I dislike the need of PHP_INT_MAX. I think that should be difference since image_resize_dimensions can handle it.

[pbearne]

Replying to markoheijnen:

I dislike the need of PHP_INT_MAX. I think that should be difference since image_resize_dimensions can handle it.

I agree that it would nice if that function handled the NULL's

I read through the code and couldn't workout how it would handle NULL's being passed in and the comments only refer to INT :-)

I could set 9999 or some other random number but this could fail in an edge case so I felt that PHP_INT_MAX was the best as if the image is that big than that something else would break first

Paul

[kirasong]

Replying to alconebay:

Ah, apologies. Indeed, you are correct on the &&s there being an issue.
The primary reason that check is there is to avoid warnings when we access the array keys, rather than any sort of range checking.

The intended behaviour with resize() is to be able to pass null as one of height or width (and this works).

As @markoheijnen explains, image_resize_dimensions() should handle null just fine (because null <= 0), and as such, resize() does as well. If there's an error in image_resize_dimensions(), it bubbles up.

However, right now, the signature of multi_resize() (and hence the check mentioned) keeps you from taking advantage of this. I'd be up for changing it to allow null on one of height or width, to bring it in line with resize()'s behaviour.

If we do this, though, I'd prefer to see it simply pass null to resize(), if the key is missing or null (and update docs to reflect this), rather than any sort of PHP_INT_MAX trickery.

[pbearne]

OK

I will change the patch to handle one being NULL.
And do some testing.
Are there unit test for this?

I also feel we should return an error message in the return array if both values are NULL as this is a black box otherwise (I was getting an empty array).
What do you feel?

Paul

[markoheijnen]

The only unit tests as what I remember are for image_resize_dimensions() and the resize stuff.

We should not return errors in multi_resize() since this function is there specially for WordPress internal usage. The data is directly stored to the meta data. This is also the reason why the path of the image is being removed.

[markoheijnen]

We could however add a check to add_image_size(). Not sure what others think about that.

[pbearne]

Patch with no check for missing height and width

[pbearne]

the patch with NULL/0 value for both height and width

[pbearne]

Replying to markoheijnen:

We could however add a check to add_image_size(). Not sure what others think about that.

In add_image_size() NULL or a string its gets mapped to 0 by the ABSINT() So what check should we add?

This showed me a way to fix the main bug
If we wrap the height and width with ABSINT() we can remove the isset() test (patch 2) or better we can change the test to check if both height and width or null or 0 (patch 3)

Which shall we use?

Done a quick bit of testing and both work I feel that patch 3 is better as it returns earlier if no re-size would happen

Paul

[markoheijnen]

We could do a check there if width and height or 0/null then it returns false.

The absint isn't needed and how it is done is redundant since you are calling multiple times.
if ( ! isset( $size_data['width'] ) && ! isset( $size_data['height'] ) ) should be enough then. Didn't test it.

[pbearne]

Replying to markoheijnen:

We could do a check there if width and height or 0/null then it returns false.

The absint isn't needed and how it is done is redundant since you are calling multiple times.
if ( ! isset( $size_data['width'] ) && ! isset( $size_data['height'] ) ) should be enough then. Didn't test it.

The Problem is that this returns false for NULL's and this should be a valid input as it is valid for wp_get_image_editor->resize() both should to match

paul

[markoheijnen]

It will fix your bug since when width = null and height is 500 then it will continue. It will not when both values are null and that is good and should not change.

[pbearne]

Replying to markoheijnen:

It will fix your bug since when width = null and height is 500 then it will continue. It will not when both values are null and that is good and should not change.

The current test

 if ( ! ( isset( $size_data['width'] ) && isset( $size_data['height'] ) ) )
   continue;

skips the rest of the foreach if either value is null what no what we want

Have I got this wrong?

Paul

[markoheijnen]

There is a difference between my line and the current code. Current if statement one if one of the values is null then it will the image. In my if statement both values need to be null before it will skip the image. See two explanation marks

[alconebay]

Replying to markoheijnen:

There is a difference between my line and the current code. Current if statement one if one of the values is null then it will the image. In my if statement both values need to be null before it will skip the image. See two explanation marks

I just tested your line. It works.

[pbearne]

Replying to alconebay:

Replying to markoheijnen:

There is a difference between my line and the current code. Current if statement one if one of the values is null then it will the image. In my if statement both values need to be null before it will skip the image. See two explanation marks

I just tested your line. It works.

OK I will the create the patch

[pbearne]

fixed patch

[pbearne]

New patch with markoheijnen's fix added.

Now lets see if we can get committed :-)

Paul

[nacin]

That patch will work fine if 'width' or 'height' are set to null, but it will throw a notice if 'width' or 'height' are omitted entirely. So, we should either enforce that 'width' and 'height' are to be passed (even if as null), or we should work to avoid a notice, thus allowing 'width' or 'height' to be omitted in lieu of being passed as null.

Unit tests would be great either way to ensure A) it works and B) no notice is thrown.

[pbearne]

Hi all

I have done have the testing

Would love someone could check them

Paul

[pbearne]

class patch

[pbearne]

test patch

[nacin]

Great. Just needs review and we should be able to get this into 3.9.

[pbearne]

thanks Nacin

I have created the imagemaqic half and posted them as well

The code is basically the same

All in one patch file.

let me know if you need it a different format.

Paul

[pbearne]

combined patches and test for 3.9

[kirasong]

Thanks, pbearne!

I was going to ask about the Imagick tests. I'm looking over this.

[pbearne]

Replying to DH-Shredder:

Thanks, pbearne!

I was going to ask about the Imagick tests. I'm looking over this.

Do you need the code for the Gmagick Plug-in

​https://plugins.trac.wordpress.org/browser/gmagick/trunk/editors/gmagick.php

Is Gmagick going to make it in to core?

Paul

[nacin]

Replying to pbearne:

Is Gmagick going to make it in to core?

No plans at this time. The PHP extension is essentially unused in the wild.

[wonderboymusic]

26823.diff​ rehabilitates the whitespace. The files needs to be unlink()'d t the end of each test. Need opposing assertions in the tests - meaning, you need to assert more things than just one assertEquals() in each test.

[kirasong]

wonderboymusic: Thanks.

Working through cleaning it up here as well, for another pass, and consolidating a couple tests.

pbearne: Question -- it looks like you have a few comments in the tests on sizes that don't line up with the actual size changes being performed. Should I mark it up according to the actual sizes passed?

[pbearne]

Replying to wonderboymusic:

26823.diff​ rehabilitates the whitespace. The files needs to be unlink()'d t the end of each test. Need opposing assertions in the tests - meaning, you need to assert more things than just one assertEquals() in each test.

I am still getting my head around unit tests (So I am not much above a copy and past kid) and using this to learn.

Can point / provide an example?

and I will re-code

I see that I should be deleting the created images in each test but had not worked out how.

/**
	 * Test resizing an multi resizong image, and then load the image to check size
	 * 
	 */
	public function test_multi_resize_check_is_resized() {

		$file = DIR_TESTDATA . '/images/waffles.jpg';
		$expected_output_file = DIR_TESTDATA . '/images/waffles-50x33.jpg' ;
		@unlink( $expected_output_file );

		$gd_image_editor = new WP_Image_Editor_Imagick( $file );
		$gd_image_editor->load();
		$sizes_array =	array(
                			array ('width' => 50, 'height' => 50 )
        				);
		$resized = $gd_image_editor->multi_resize( $sizes_array );
		
		$this->assertTrue(file_exists( $expected_output_file ), 'File created');

		$gd_image_output = new WP_Image_Editor_Imagick( $expected_output_file );
		$gd_image_output->load();


		$this->assertEquals( array( 'width' => 50, 'height' => 33  ), $gd_image_output->get_size() );
		@unlink( $expected_output_file );
	}

Should I be testing the file create each time or is the one time OK?

What else would nice to test here I am trying not to test the function this calls?

Many Thanks

Paul

[pbearne]

Replying to DH-Shredder:

wonderboymusic: Thanks.

Working through cleaning it up here as well, for another pass, and consolidating a couple tests.

pbearne: Question -- it looks like you have a few comments in the tests on sizes that don't line up with the actual size changes being performed. Should I mark it up according to the actual sizes passed?

Yes or remove (copied code)

[pbearne]

Ping me if I can help with the coding

Paul

[kirasong]

Previous Patch, plus setting null rather than 0 for missing values, Consolidated Tests, and updated documentation

[kirasong]

Attached 26823.2.diff​.

This contains the previous patch from @pbearne, plus:

• Updated documentation all around
• Whitespace rehabilitation (close to the patch from @wonderboymusic, with additional style modifications)
• In editors, if a width/height is missing, set null rather than 0 for missing values, to make it clear in code that we're not just setting a 0 width or height.
• Consolidated Tests
• Tests now unlink images after creation
• New test helper function to verify image dimensions (this works, but it's possible this should be done outside of an editor, with PHP directly)

If it's immediately clear to a unit test wizard how we might do these tests without the code duplication that exists, I will not complain in the slightest -- since the only difference in most of these tests (between the editors) is the class being called.

[kirasong]

This is still missing a test with both width and height set to null (and no image being generated as a result).

As I may not get to this tonight, feel free to add one if you get to it first. :)

[pbearne]

Replying to DH-Shredder:

This is still missing a test with both width and height set to null (and no image being generated as a result).

As I may not get to this tonight, feel free to add one if you get to it first. :)

Will do

I feel that we make sure that we don't have file with the expected value in the target folder so I feel we should add this before the function is run

		// loop the exected output and unlink and file found so we know 
		// this files are created by test
		foreach( $resized as $key => $image_data ){
			$image_path = DIR_TESTDATA . '/images/' . $image_data['file'];
			@unlink( $image_path );
		}

I see a couple of other values that could be used

//will create and image with the size in the filename
			array(
				'width'  => 9999, # Arbitrary High Value
				'height' => 9999, # Arbitrary High Value
			),
// will this crop the image to the max size
			array(
				'width'  => 9999, # Arbitrary High Value
				'height' => 9999, # Arbitrary High Value
				'crop'   => true,
			),
//will create and image with the size in the filename
   		        array(
				'width'  => 0, # Arbitrary High Value
				'height' => 0, # Arbitrary High Value
			),
// will this crop the image
			array(
				'width'  => 0, # Arbitrary High Value
				'height' => 0, # Arbitrary High Value
				'crop'   => true,
			),

[kirasong]

Replying to pbearne:

Replying to DH-Shredder:

This is still missing a test with both width and height set to null (and no image being generated as a result).

As I may not get to this tonight, feel free to add one if you get to it first. :)

Will do

Awesome, thanks!

I feel that we make sure that we don't have file with the expected value in the target folder so I feel we should add this before the function is run

It seems pretty safe to assume that the files won't be there when we start it, since if there are files left, it means there was a failure somewhere. If it passes once, the files get cleared up. In other unit/integration tests in core that I checked, we currently only remove filesystem content after the test is complete, rather than pre-cleaning.

However! I'm willing to defer on that point to someone who has done more testing than I.

I see a couple of other values that could be used

//will create and image with the size in the filename
			array(
				'width'  => 9999, # Arbitrary High Value
				'height' => 9999, # Arbitrary High Value
			),
// will this crop the image to the max size
			array(
				'width'  => 9999, # Arbitrary High Value
				'height' => 9999, # Arbitrary High Value
				'crop'   => true,
			),
//will create and image with the size in the filename
   		        array(
				'width'  => 0, # Arbitrary High Value
				'height' => 0, # Arbitrary High Value
			),
// will this crop the image
			array(
				'width'  => 0, # Arbitrary High Value
				'height' => 0, # Arbitrary High Value
				'crop'   => true,
			),

Agreed that that sort of additional tests are a good idea. However, we have to be cautious with having those in the same (long) test. If there's more than one image in a multi_resize call that intentionally does not create a new file or Array entry, it will make it harder to tell which one failed, and for what reason, when we run the batch.

As a last note, I realize it was probably just a quick copy/paste (which I've certainly done before), but I'd suggest being careful that the inline comments match what you're trying to test (0 is not an arbitrarily high value) -- it helps to make it as clear as possible which behaviour that is being tested, so that if/when the test fails, it's obvious what went wrong. :)

[pbearne]

patch with null and 0 test

[pbearne]

I have added a test for null and zero's

I test that we get an empty array and that file count in folder doesn't change.

But I feel that the re-size function doesn't do the right thing. If you pass both H and W as null/0 with a crop flag an rectangular image should be crop to a square image at the maxmim possible size.

What do you think?

Paul

[pbearne]

Do we need to do anything else to get this patched

[pbearne]

Bump for a 3.9 commit

[kirasong]

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.

[pbearne]

Replying to 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

[pbearne]

new patch

[pbearne]

new images for \tests\phpunit\data\images

[pbearne]

replacement file, first had a var_dump()

[pbearne]

Bump geting close to missing 3.9 :-)

[tomauger]

Replying to pbearne:

Bump geting close to missing 3.9 :-)

Paul what's missing? Do you just need to get eyes on the unit tests?

[pbearne]

Its ready

the patch is totally done and I believe the tests are good as well so yes eyes on the unit tests

the test will need the comments fixing to remove the @26823 so that they run but that it

If anything I have over tested

Paul

[kirasong]

You don't need to worry about bumping/missing the release at this point.
We have a couple of weeks of beta, and it's in the 3.9 milestone, so it'll get more attention.

[kirasong]

Chatted about this ticket with @tierra.

Let's do the following: Use the same filename, but instead do deletes within tearDown() so that we're sure they get removed, even if a test fails.

Then, let's get this committed.

[pbearne]

I will get to this ASP (today / week-end)

[pbearne]

Patch with deletes within a tearDown() function

[pbearne]

Hi All

I hope this patch is OK I had a bit of a problem apply the last patch back on a build so I had t hand copy some of the code so I hope I got it right

The tests all pass with no errors so it looks good

The delete is now in the tear down I am deleting all files that match waffles-*.jpg

I have changed a couple of the other tests to use waffles.jpg so that the tear down function removes then as well.

Paul

[helen]

pbearne: It looks like your latest patch lost the cleanup from DH-Shredder. Committers usually do a least some amount of cleanup on patches before commit, but I think in this case it'd be good for you to take a little more care with coding standards and give it another shot. DH-Shredder's patch still applies for me on trunk, so you can still use it as a starting point.

[pbearne]

Hi Helen

I have just looked at the two patches and the only diff in coding standards is that have wrapped single line if's in {} to make it clear there scope.

Can you point me to an example so I can fix them?

Paul

[kirasong]

Cleaned up tests; moved helper to base; re-applied changes to editors.

[wonderboymusic]

In 27794:

In multi_resize() image editor methods, assert that null can only be passed for one of the arguments, not both. Add a lot more unit test assertions to ensure this.

Props pbearne, DH-Shredder.
Fixes #26823.

[kirasong]

Hey pbearne!

I chatted with Helen and Scott about this. Basically, yes -- the part you mention was the primary bit that needed to be reverted.

It's an important practice to make sure the previous patch is applied before continuing to make changes because it takes a long time to go through and diff diffs when things aren't clean.

In the interest of this not missing 3.9, I worked with both of them to get it cleaned up and in. Please feel free to ping me in IRC if you ever run into questions. Thanks for all your work!

[pbearne]

Thanks for the help / advice in getting this in.

I live in git to much and never installed your patch just kept creating patches from my fork. SVN I love you :-(

It in that is what counts

Paul

Replying to DH-Shredder:

Hey pbearne!

I chatted with Helen and Scott about this. Basically, yes -- the part you mention was the primary bit that needed to be reverted.

It's an important practice to make sure the previous patch is applied before continuing to make changes because it takes a long time to go through and diff diffs when things aren't clean.

In the interest of this not missing 3.9, I worked with both of them to get it cleaned up and in. Please feel free to ping me in IRC if you ever run into questions. Thanks for all your work!