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

[tivnet]

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

[jeremyfelt]

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.

[tivnet]

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.

[jtwg]

I filed this ticket which is similar:
https://core.trac.wordpress.org/ticket/28097

Still broken in 3.9

[SergeyBiryukov]

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

[wonderboymusic]

#28097 was marked as a duplicate.

[wpdavis]

#23144 was marked as a duplicate.

[wpdavis]

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.

[wpdavis]

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.

[SirLouen]

Replying to tivnet:

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.

I have to agree with @tivnet. The fallback behaviour is too unexpected in this scenario, it feels like left at random, not all use cases, like these are covered correctly.

For code correctness, leaving this permissions' scenario to such fallback is inadequate. Surely, with permission sorting this might not happen, but with the right fallback logic, this should be restrained.

I'm not 100% convinced about @wpdavis patch, though. It can solve the problem, but it's broadening the error condition to new unexpected behaviour. If we could add some Unit Tests before modifying this logic (needs-unit-tests), it would feel more safe overall, looking for a better condition.

The thing here is that, regardless or not, of having an error (permission error), doing such GUID fallback, when not desired is not ideal. In fact, I would like to see Test Info (needs-test-info), on when falling back to GUID is the right thing. I've been reviewing unit tests for a while and I can't see this in action.

I would like to hear other experienced members on media opinions (2nd-opinion)