WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 5 months ago

#26992 new defect (bug)

Failing to make new yyyy/mm for uploads affects existing URLs (WP_CONTENT_URL, wp_get_attachment_url and GUID)

Reported by: tivnet Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.7
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

What happens:

  • I have the WP_CONTENT_DIR and _URL pointing somewhere, not where the WP is installed.
  • There was an error creating upload folder yyyy/mm (permission issues)
  • wp_get_attachment_url() could not go through the if ( ($uploads = wp_upload_dir()) && false === $uploads['error'] ) block and came to the "not recommended to rely upon" $url = get_the_guid( $post->ID );

The GUID returned completely ignored my settings. It started with the default /wp-content/. Obviously. Those attachments were made before I moved the wp-content, and nobody fixed the GUIDs.

Result: all images broken.

I believe, something has to be changed here, so when the upload directory for new attachments cannot be created, it won't affect the existing attachments.


Code Path:
wp_get_attachment_image_src -> image_downsize -> wp_get_attachment_url

Attachments (1)

wp_get_attachment_url.patch (1.0 KB) - added by wpdavis 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 follow-up: @jeremyfelt
6 years ago

It seems that a fix for this scenario would need to account for an individual site having multiple base content URLs. We could skip the check for the valid upload directory, but I'm not sure what side effects would be caused. On the other hand, fixing the permissions issue would resolve it immediately (I think).

IMO, this is not a decision that WordPress core can make. If a site structure is moved and the permissions are not configured properly, guessing intent would be difficult.

The wp_get_attachment_url filter is available for modifying the URL on a post by post basis. This could come in useful if some attachments have different URLs than others.

#2 in reply to: ↑ 1 @tivnet
6 years ago

Replying to jeremyfelt:

... fixing the permissions issue would resolve it immediately (I think).

Yes, of course. The problem is that new month [usually] comes at night. Only in a few hours, in the morning you notice that all the links are broken.

So, again, why do we need to check for the valid upload directory is nobody is uploading anything right now? The request was "what's the URL of an existing attachment", and not "Where do I put this uploaded file". I think.

#3 @jtwg
6 years ago

I filed this ticket which is similar: #28097

Still broken in 3.9

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
6 years ago

  • Component changed from Permalinks to Media
  • Version changed from 3.8 to 2.7

Introduced in [8796]. See also [9177].

#5 @wonderboymusic
5 years ago

#28097 was marked as a duplicate.

#6 @wpdavis
5 years ago

#23144 was marked as a duplicate.

#7 @wpdavis
5 years ago

  • Keywords has-patch added

Confirmed: This is a stupid, annoying thing. Attached is a patch that only checks for a permissions error if it's a newly-uploaded file. Else the script is just using the basedir piece of wp_upload_dir, and the fact that there is a permissions error is completely irrelevant.

Edit: Not trying to call anyone stupid, just frustrated after spending 15 minutes tracking that one down.

Last edited 5 years ago by wpdavis (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by wpdavis. View the logs.


5 years ago

#9 @wpdavis
5 years ago

FYI, this is the original ticket that introduced the code change: #7622

I see no discussion in there as to why to check for the error if just replacing the basedir with baseurl. I also don't see any cases where wp_upload_dir() returns an error other than if the directory isn't writeable.

fwiw, we're using this patch on production right now without problems.

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


4 years ago

#11 @wpdavis
4 years ago

  • Keywords needs-testing added
Note: See TracTickets for help on using tickets.