wp_get_image_editor() ->save stopped creating the directory in 5.6-beta2-49360

[eemitch]

I have been using this method in my plugin to create image thumbnails. These are placed in a hidden directory (.thumbnails). I recently discovered that the directory is no longer being created using ->resize in 5.6-beta2-49360

It works fine on 5.5.1

[eemitch]

My function to create thumbnail images

[johnbillion]

Milestoning for visibility

[hellofromTonya]

Notes from today's core scrub:

@johnbillion:

51665 needs testing for verify that's it's valid, first of all

@metalandcoffee:

I don’t mind testing it if you’re busy John
I’d like to ask them for steps to reproduce though…

[hellofromTonya]

@eemitch Thank you for reporting this issue.

Can you please share the steps of how to reproduce it? These steps help in the process of identifying the root cause.

[eemitch]

Sure..

This is a breakdown of my code…

// Define the file path --> .thumbnails does not exist
$eeThumbsPATH = ABSPATH . $eeSFL_Config['FileListDir'] . $eeDir . '.thumbnails/'; // Path to thumbs for this folder
		
		
// Thank WordPress for this easyness.
$eeFileImage = wp_get_image_editor($eeFileFullPath); // Try to open the file

if (!is_wp_error($eeFileImage)) { // Image File Opened
	            
	$eeFileImage->resize($this->eeFileThumbSize, $this->eeFileThumbSize, TRUE); // Create the thumbnail
	            
	$eeFileNameOnly = str_replace('eeScreenshot_', '', $eeFileNameOnly); // Strip the temp term from video screenshots
			
	$eeSFL_Log['SFL'][] = 'Creating Thumb: ' . $eeThumbsPATH . 'thumb_' . $eeFileNameOnly . '.jpg'; 
	            
	$eeFileImage->save($eeThumbsPATH . 'thumb_' . $eeFileNameOnly . '.jpg'); // Save the file <— THIS NOW FAILS IF .thumbnails/ DOES NOT EXIST
	        
}

Note that .thumbnails is intended to be a hidden folder.

This works in the 5.5.1, but not the 5.6-beta2-49360

I’ve added a few line to now detect if .thumbnails exists, and create if not. So my plugin no longer requires wp_get_image_editor -> save to create the directory.

[davidbaumwald]

Removing needs-testing because there's no patch to test yet and dev-feedback as there no discussion on a solution just yet either.

[kirasong]

I was able to reproduce this change in behavior.

It looks like it happened in r49230.

See: #42663

[kirasong]

Another note:
In testing, this looks like it happens with the creation of any folder, not just hidden ones.

The code to reproduce still does as described when thumbnails is used, rather than .thumbnails.

[kirasong]

Run wp_mkdir_p before write.

[kirasong]

It looks like it is because this behavior no longer happens now that make_image() isn't used by Imagick:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-image-editor.php?rev=49230#L490

Something like this looks to do the trick in initial testing: 51665.diff​

Automated tests would be good, too, to prevent future regressions.

cc: @p00ya for another opinion.

[p00ya]

Fix looks good at first glance, though wp_mkdir_p is a scary beast.

Updating the unit tests to cover this case would be a bonus.

[johnbillion]

Introduced in [49230]

[johnbillion]

Struggling a bit with this. I can't get the test to create the directory. 51665.2.diff​ includes a more comprehensive patch and a test.

Assistance appreciated.

[johnbillion]

Ok here we go. Issue was caused by ../ in the test image directory path.

51665.3.diff​ fixes the issue and adds a passing test that fails on trunk.

My remaining concern is whether this is the correct location for the fix or if it can be moved to a higher/lower level.

[hellofromTonya]

Replying to johnbillion:

My remaining concern is whether this is the correct location for the fix or if it can be moved to a higher/lower level.

Capturing notes from ​Slack, needs decision from the Media team.

cc @mikeschroder @antpb

[kirasong]

Thanks @johnbillion!

I like the additional error reporting you added.

I picked this spot because it best matches the logic from the previous code path.

Unless we re-evaluate make_image() again, I'm not sure of a better place to put it. Current opinion is that it's fine to start here, and if a better place emerges later, make a change.

If anyone else can think of a better spot, then excellent!

Thanks for the test as well -- I'm noticing an issue when running the entire test suite locally. Getting a few errors, starting with:

...........................................................  5664 / 11471 ( 49%)
...PHP Warning:  Uncaught require_once(/srv/www/wordpress-develop/public_html/build/wp-content/srv/www/wordpress-develop/public_html/tests/phpunit/includes/../data/themedir1/custom-internationalized-theme/functions.php): failed to open stream: No such file or directory

/srv/www/wordpress-develop/public_html/tests/phpunit/tests/l10n/localeSwitcher.php:444
/srv/www/wordpress-develop/public_html/tests/phpunit/tests/l10n/localeSwitcher.php:444

If I remove the tests, it passes, and if I add back the change in tests/phpunit/includes/bootstrap.php, it fails.

Will look into it, but thought I'd mention here in case anyone can track it down sooner.

[kirasong]

use realpath() in tests

[kirasong]

51665.4.diff​ resolves the failures in 51665.3.diff​ by using realpath() in the new test, rather than in bootstrap.php.

I'm not sure if this is the ideal way to do this, but it preserves the expectations other tests currently have.

@johnbillion Does this seem okay to you, or would you prefer to resolve it a different way?

[johnbillion]

Alright let's do that in the test instead of changing the bootstrap and I'll open a separate ticket to investigate why that test data directory path uses ../ instead of dirname().

Trac ticket: https://core.trac.wordpress.org/ticket/51665

PR for running the test suite on Travis and GHA before committing.

[johnbillion]

In 49542:

Media: Restore the ability of WP_Image_Editor_Imagick->save() to create a missing directory when needed.

Props eemitch, mikeschroder, hellofromTonya, p00ya, johnbillion

Fixes #51665

[p00ya]

FYI I've tested with 5.6-beta4, the gcs-media-plugin and a directory that didn't previously exist. Works and thanks for the fix.

However, the patch in this ticket does not affect the wp_is_stream branch of the if statement. While the particular stream wrappers I've been testing with don't need parent directories of a new file to be created first, other stream wrappers might. file_put_contents won't implicitly create directories either. So we might want to move the wp_mkdir_p up above the if(). Also note this won't be a regression (because no streams would have worked prior to 5.6), so I think it's fine to fix in 5.7.