Make WordPress Core

Opened 11 years ago

Last modified 13 months ago

#25650 new 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:
Milestone: Awaiting Review 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 8 years ago.
First approach for a solution
25650_approach_2.patch (1.9 KB) - added by igmoweb 8 years ago.
25650-wp_upload_dir-fix-after-switch_to_blog.patch (7.7 KB) - added by ideag 2 years ago.
approach 3

Download all attachments as: .zip

Change History (24)

#1 @Denis-de-Bernardy
10 years ago

  • Keywords dev-feedback added

@nacin: Do we want this fixed in 3.9?

#2 follow-up: @igmoweb
10 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
10 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
9 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
9 years ago

  • Focuses multisite added

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


8 years ago

#9 @chriscct7
8 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
8 years ago

First approach for a solution

#10 @igmoweb
8 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.


8 years ago

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


7 years ago

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


6 years ago

#14 @desrosj
2 years ago

#40072 was marked as a duplicate.

#15 @ideag
2 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
2 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.


2 years ago

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


2 years ago

#19 @ideag
2 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 2 years ago by ideag (previous) (diff)

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


13 months ago

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


13 months ago
#21

  • Keywords has-unit-tests added
Note: See TracTickets for help on using tickets.