WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 13 months ago

Last modified 10 months ago

#23306 closed defect (bug) (fixed)

WP_Image_Editor::make_image should call wp_mkdir_p

Reported by: batmoo Owned by: ryan
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch dev-feedback reporter-feedback
Focuses: Cc:

Description

To play nicely with some replication plugin, WP_Image_Editor::make_image should make sure that the target folder exists before proceeding with accessing the image.

WordPress does something similar with wp_crop_image and similar functions.

Attachments (1)

23306.diff (576 bytes) - added by batmoo 15 months ago.

Download all attachments as: .zip

Change History (11)

batmoo15 months ago

comment:1 batmoo15 months ago

  • Keywords has-patch added

Patch calls wp_mkdir_p at the start of the method.

comment:2 markoheijnen15 months ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 3.6
  • Version set to 3.5

We can probably do that. Still WP_Image_Editor is a very abstracted class and the default folder is the folder itself.

So you can also see it that the developer should deal with it. Most likely because of you want to call wp_mkdir_p before you even going to load an image. This means that this patch only fixes 50%. What can be good enough.

Put it on the 3.6 milestone for consideration.

comment:3 markoheijnen15 months ago

  • Keywords reporter-feedback added

btw can you give an example of a plugin. I'm not sure what you mean with replication. Is that image or site based.
Also I'm missing why using that function. Seems like a file system operation without the need of resizing.

comment:4 batmoo15 months ago

Also I'm missing why using that function

WordPress core uses this function :) (Both WP_Image_Editor_GD and WP_Image_Editor_Imagick call the make_image method when a user is editing a file from Dashboard > Media > Edit).

The issue is the similar to the one fixed in [20384]; certain replication plugins (like the one we use on WordPress.com) don't guarantee that a file (or directory, in this case) will be available in the local filesystem and this helps prevent issues because of that.

comment:5 markoheijnen15 months ago

I meant wp_crop_image(). WP_Image_Editor is what I partly written myself so I do know that code.

My opinion still stands that it shouldn't be dealt inside WP_Image_Editor. Also if you really want to add it you can do that in the custom WP_Image_Editor you probably already have by now.

Still My questions stands in comment 3.

comment:6 batmoo15 months ago

Sorry, I should clarify. I don't have a custom class that extends WP_Image_Editor and my custom code is not interacting with that class anywhere.

I'm running into this problem when editing an image via the "Edit Image" interface in the WordPress Dashboard, i.e. using WordPress core functionality.

comment:7 markoheijnen15 months ago

Ah so no WordPress.com related thing. I still don't get the issue when editing an image through WordPress dashboard. Since all those images would be saved on the same directory where the main image is stored.

comment:8 ryan15 months ago

We try to remove assumptions from core that an image file will actually exist in the location where it is supposed to exist. If it does not exist we fetch the image using the url and pretend we loaded it from disk from the expected place. When we go to save it, the directory that contains the file that doesn't really exist often also doesn't exist. This has to do with replication plugins that put files in places like S3 or some other remote image/file store. We must assume that there is no guarantee of local storage consistency from one page load to another.

comment:9 ryan13 months ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In 23744:

Make WP_Image_Editor::make_image() safe for replication plugins by making sure the directory for the image being made exists.

Props batmoo
fixes #23306

comment:10 nacin10 months ago

FYI, #24459 is going to change this to only call wp_mkdir_p() when we're not dealing with a stream.

Note: See TracTickets for help on using tickets.