Make WordPress Core

Opened 11 years ago

Last modified 4 months ago

#25650 assigned defect (bug)

When switching between blogs, wp_upload_dir 'baseurl' and 'url' may be pointing to the current blog not the switched one

Reported by: igmoweb's profile igmoweb Owned by: ideag's profile ideag
Milestone: Future Release Priority: normal
Severity: normal Version: 3.6.1
Component: Upload Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

Only tested on subfolder multisite (I don't have a way to test on a subdomain install right now).

It seems very similar to ticket #23483 but I'm not sure if it should be treated the same:

Example with two blogs in a network:

  • www.mydomain.com/my-source-blog (ID:5)
  • www.mydomain.com/my-destination-blog (ID:15)

I discover this while trying to move a post with images from a blog to another. The post is copied and images inside the post are copied too.

Starting point: The current blog ID is 15 (my-destination-blog). I'm trying to get upload URLs from the current blog:

switch_to_blog( 5 );
$source_upload_dir = wp_upload_dir();
$source_upload_baseurl = $source_upload_dir['baseurl'];
restore_current_blog();

$destination_upload_dir = wp_upload_dir();
$destination_upload_baseurl = $destination_upload_dir ['baseurl'];

At this point, $source_upload_baseurl is:

http://www.mydomain.com/my-destination-blog/wp-content/uploads/sites/5

Where should be:

http://www.mydomain.com/my-source-blog/wp-content/uploads/sites/5

$destination_upload_baseurl is fine.

It seems that if get_option( 'upload_url_path' ) returns false, wp_upload_dir() makes use of WP_CONTENT_URL constant that is set to the current blog URL at the beggining of the execution (in default-constants.php):

define( 'WP_CONTENT_URL', get_option('siteurl') . '/wp-content');

This value obviously does not change when switching a blog.

Attachments (3)

25650_approach_1.patch (2.3 KB) - added by igmoweb 9 years ago.
First approach for a solution
25650_approach_2.patch (1.9 KB) - added by igmoweb 9 years ago.
25650-wp_upload_dir-fix-after-switch_to_blog.patch (7.7 KB) - added by ideag 3 years ago.
approach 3

Download all attachments as: .zip

Change History (30)

#1 @Denis-de-Bernardy
11 years ago

  • Keywords dev-feedback added

@nacin: Do we want this fixed in 3.9?

#2 follow-up: @igmoweb
11 years ago

I'm not completely sure but in case it helps, I think a fix would be replace current 1578 line in wp-includes/functions.php:

$url = WP_CONTENT_URL . '/uploads';

For something like

$url = get_option('siteurl') . '/wp-content/uploads';

But the problem now would be that the user could not change the content url by himself using WP_CONTENT_URL constant. Or at least, new issues would come, I guess.

#3 in reply to: ↑ 2 @Denis-de-Bernardy
11 years ago

Replying to igmoweb:

But the problem now would be that the user could not change the content url by himself using WP_CONTENT_URL constant. Or at least, new issues would come, I guess.

Yeah, it would likely be a regression. :-)

Something along the lines of filtering the uploads url might work, though (see #23221).

#4 @pavelevap
10 years ago

I probably ran into the same issue with subdomain install, but I am not sure...

Main site is on mydomain, secondary site is on subdomain.mydomain. After switch_to_blog(2) and calling the_post_thumbnail() there is problem with image URL on main site:

http://mydomain/wp-content/uploads/sites/2/2014/10/picture.jpg (does not exist)

But it should be:

http://subdomain.mydomain/wp-content/uploads/sites/2/2014/10/picture.jpg

#5 @yo-l1982
10 years ago

#30209 was marked as a duplicate.

#6 @flynsarmy
10 years ago

Here's a workaround I'm using until this is fixed:

add_action('switch_blog', function($new_blog, $prev_blog_id) {
	update_option( 'upload_url_path',  get_option('siteurl') . '/wp-content/uploads');
}, 10, 2);

#7 @jeremyfelt
10 years ago

  • Focuses multisite added

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


9 years ago

#9 @chriscct7
9 years ago

  • Keywords needs-patch added; dev-feedback removed

Bug Scrub 1/8/2015 notes:

  • The suggestion was made to introduce a function for which the current content directory can be retrieved at any time, thus solving the switch_to_blog bug. The suggested title for this function, which would replace the use of the constant of the same name is wp_content_dir()

@igmoweb
9 years ago

First approach for a solution

#10 @igmoweb
9 years ago

Hi, I attached a patch for a first approach to the problem ( 25650_approach_2.patch, forget about the first one). This was what I had in mind for this. Obviously, this is not fully working but it fixes the main problem.

So the idea is to stop trusting WP_CONTENT_URL constant and add a global $wp_content_url (maybe a static one is better, not sure right now). The global is actually based on the constant so the user can still define it in some way. When a switch between blogs is made we can use this global to change the content URL. Right now, I'm just replacing the sites URLs, what can be bad but it's a start.

A dev feedback would be handy in here.

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


9 years ago

This ticket was mentioned in Slack in #core-multisite by drew. View the logs.


8 years ago

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


7 years ago

#14 @desrosj
3 years ago

#40072 was marked as a duplicate.

#15 @ideag
3 years ago

  • Keywords has-patch added; needs-patch removed

I took a stab at this during WordCamp Europe 2022 Contributors day. I did the following things:

  1. replaced the use of WP_CONTENT_URL in _wp_upload_dir() with a call to content_url()
  2. added additional logic to content_url() method to check if switch_to_blog() has been used and if the WP_CONTENT_URL has been set in the default-constants.php file, rather than explicitly in wp-config.php
  3. added an indication to default-constants.php to signify that the WP_CONTENT_URL wss set by WP itself rather than explicitly in wp-config.php. I'm achieving that by defining an additional constant, WP_DYNAMIC_CONTENT_URL. I don't entirely like this part, but I think it was the best compromise in terms of readability, reliability, performance and testability that I could find.
  4. updated tests to test for both the explicit WP_CONTENT_URL setting and the dynamic WP_CONTENT_URL setting.


#16 @ideag
3 years ago

I had another idea to identify if WP_CONTENT_URL was explicitly set in wp-config.php. It goes along these lines (at the top of content_url() method):

$url = WP_CONTENT_URL;
if ( ms_is_switched() ) {
    $original_blog_id = $GLOBALS['_wp_switched_stack'][0];
    $dynamic_content_url = get_blog_option( $original_blog_id, 'siteurl' ) . '/wp-content';
    if ( WP_CONTENT_URL === $dynamic_content_url ) {
        $url = get_option( 'siteurl' ) . '/wp-content';
    }
}

The upside for this it allows to avoids creating a new constant in the step 3 of the above patch.
The downside is this adds an additional loop of switch_to_blog() inside the get_blog_option() method and an additional call to get_option(), so it should have a bigger performance fingerprint, it uses a global variable and is in general harder to read and understand and I'm not sure I can write a test that would not rely on the same code we are testing.
Also, most significantly, this would fail if someone explicitly set their WP_CONTENT_URL to the upload url of the main site. Can't find a way around that yet.

This ticket was mentioned in Slack in #core-multisite by arunas. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-multisite by arunas. View the logs.


3 years ago

#19 @ideag
3 years ago

The above patch only affects the behavior of _wp_upload_dir() function in one specific case:

  1. upload_url_path option is not explicitly set for a site;
  2. At least one of these are true:
    1. upload_path option is not set;
    2. upload_path option is set to 'wp-content/uploads' (the default for that option);
    3. upload_path option value is equal to ABSPATH (which I don't think is really feasible ?)

In that specific case I'm replacing WP_CONTENT_URL constant with a call to content_url(), which (before patching) always resolves back to the WP_CONTENT_URL constant, but additionally sets the url scheme with set_url_scheme() and is filterable via the content_url filter hook.

This patch further changes the behaviour of content_url() in one specific case. On multisite, when switch_to_blog() is called and was WP_CONTENT_URL NOT explicitly defined in wp-config.php, it does not rely on the constant (which was set back when we have not switched to a different blog yet and does not make sense anymore), but calls get_option('siteurl') for the current blog directly.

Behaviour of content_url() in different scenarios:

No Multisite? ms_is_switched() WP_CONTENT_URL Current behaviour Patched behaviour
1 No No Explicitly defined in wp-config.php WP_CONTENT_URL WP_CONTENT_URL
2 No No Dynamically defined in default-constants.php WP_CONTENT_URL which resolves to get_option( 'siteurl' ) . '/wp-content' WP_CONTENT_URL which resolves to get_option( 'siteurl' ) . '/wp-content'
3 Yes No Explicitly defined in wp-config.php WP_CONTENT_URL WP_CONTENT_URL
4 Yes No Dynamically defined in default-constants.php WP_CONTENT_URL which resolves to get_option( 'siteurl' ) . '/wp-content' WP_CONTENT_URL which resolves to get_option( 'siteurl' ) . '/wp-content'
5 Yes Yes Explicitly defined in wp-config.php WP_CONTENT_URL WP_CONTENT_URL
6 Yes Yes Dynamically defined in default-constants.php WP_CONTENT_URL which resolves to get_option( 'siteurl' ) . '/wp-content' where get_option() gets data from the original site get_option( 'siteurl' ) . '/wp-content' where get_option() gets data from the switched-to site

I think it would be OK to stop relying on the constant in all cases where it is set dynamically in the default-constants.php file as it does resolve to the same get_option() call anyway, but I was trying to limit this patch as much as possible to just fix the issue at hand. If you think it is wise to make it broader, I can expand the patch to cove those cases as well.

Last edited 3 years ago by ideag (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by arunas. View the logs.


21 months ago

This ticket was mentioned in PR #4475 on WordPress/wordpress-develop by @ideag.


21 months ago
#21

  • Keywords has-unit-tests added

#22 @ideag
8 months ago

#49335 was marked as a duplicate.

#23 @spacedmonkey
8 months ago

  • Milestone changed from Awaiting Review to 6.7
  • Owner set to ideag
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.


5 months ago

#25 @Ankit K Gupta
5 months ago

This ticket was discussed in this week's test-scrub session for WP 6.7, and we have approximately two weeks for the Beta 1 release. Any details on testing this ticket would be greatly appreciated!

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


4 months ago

#27 @desrosj
4 months ago

  • Milestone changed from 6.7 to Future Release

Unfortunately, this did not get the attention it deserves during the 6.7 cycle. With RC1 due out any moment, I'm going to punt.

Note: See TracTickets for help on using tickets.