WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

Last modified 6 days ago

#51665 closed defect (bug) (fixed)

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

Reported by: eemitch Owned by: johnbillion
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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

Attachments (5)

Screen Shot 2020-10-29 at 12.01.34 PM.png (1.9 MB) - added by eemitch 4 weeks ago.
My function to create thumbnail images
51665.diff (497 bytes) - added by mikeschroder 4 weeks ago.
Run wp_mkdir_p before write.
51665.2.diff (1.6 KB) - added by johnbillion 4 weeks ago.
51665.3.diff (2.2 KB) - added by johnbillion 4 weeks ago.
51665.4.diff (1.8 KB) - added by mikeschroder 3 weeks ago.
use realpath() in tests

Download all attachments as: .zip

Change History (32)

@eemitch
4 weeks ago

My function to create thumbnail images

#1 @johnbillion
4 weeks ago

  • Milestone changed from Awaiting Review to 5.6
  • Version set to trunk

Milestoning for visibility

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 weeks ago

#4 @hellofromTonya
4 weeks ago

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…

#5 @hellofromTonya
4 weeks ago

  • Keywords reporter-feedback added

@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.

#6 @eemitch
4 weeks ago

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.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


4 weeks ago

#8 @hellofromTonya
4 weeks ago

  • Keywords dev-feedback needs-testing added; reporter-feedback removed

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 weeks ago

#10 @davidbaumwald
4 weeks ago

  • Keywords dev-feedback needs-testing removed

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

#11 @mikeschroder
4 weeks ago

I was able to reproduce this change in behavior.

It looks like it happened in r49230.

See: #42663

#12 @mikeschroder
4 weeks ago

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.

@mikeschroder
4 weeks ago

Run wp_mkdir_p before write.

#13 @mikeschroder
4 weeks ago

  • Keywords has-patch needs-testing needs-unit-tests added
  • Owner set to mikeschroder
  • Status changed from new to assigned

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.

Last edited 4 weeks ago by mikeschroder (previous) (diff)

#14 @p00ya
4 weeks ago

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.

#15 @johnbillion
4 weeks ago

Introduced in [49230]

@johnbillion
4 weeks ago

#16 @johnbillion
4 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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
4 weeks ago

#17 follow-up: @johnbillion
4 weeks ago

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.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 weeks ago

#19 in reply to: ↑ 17 @hellofromTonya
3 weeks ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 weeks ago

#21 @mikeschroder
3 weeks ago

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.

@mikeschroder
3 weeks ago

use realpath() in tests

#22 @mikeschroder
3 weeks ago

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?

#23 @johnbillion
3 weeks ago

  • Keywords commit added; needs-testing removed

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().

This ticket was mentioned in PR #708 on WordPress/wordpress-develop by johnbillion.


3 weeks ago

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

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

#25 @johnbillion
3 weeks ago

  • Owner changed from mikeschroder to johnbillion
  • Status changed from assigned to accepted

#26 @johnbillion
3 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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

#27 @p00ya
6 days ago

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.

Note: See TracTickets for help on using tickets.