bp_core_avatar_url() wonky in sub-sites since WP 3.5

[r-a-y]

WP 3.5 changed a number of things relating to uploads.

1. Changes to wp_upload_dir() in multisite - see ​#WP19235. In particular, see ​[WP22222].
2. On brand-new 3.5 installs, WP does not populate the 'upload_path' DB option as in previous WP versions.

Point no. 2 will cause bp_core_avatar_url() to generate the wrong URL when you're using multisite and you're not on the BP root blog:
​https://buddypress.trac.wordpress.org/browser/tags/1.7/bp-core/bp-core-avatars.php#L867

To solve this, 01.patch mirrors the changes in r5897 that were made to the bp_core_avatar_upload_path() function for bp_core_avatar_url().

[boonebgorges]

Thanks, r-a-y. The technique looks good.

I suggest that instead of the first block, we just do:

if ( is_multisite() ) {
    switch_to_blog( bp_get_root_blog_id() );
}

switch_to_blog() bails if we attempt to switch to the current blog, so the BP-level check is redundant.

[luccame]

I applied the patch to my MS test install and it fixes the display of missing avatar on sub-sites.

Just for reference, I was previously using the code provided by needle in this related ticket:
​http://buddypress.trac.wordpress.org/ticket/4252#comment:11

[r-a-y]

In 02.patch, I've created a new function - bp_core_get_upload_dir() to grab the wp_upload_dir() data and cache it since we were using it in both bp_core_avatar_url() and bp_core_avatar_upload_path(). This was done to prevent using switch_to_blog() more than once on a page load.

This function is then used in those respective functions to get their values.

Let me know what you think.

[boonebgorges]

This looks good at a glance, but I'm worried about breakage. Is it possible to write tests for it? I don't think our suite supports BP_ENABLE_MULTIBLOG at the moment, but at least some of the other cases should be doable.

[r-a-y]

Is it possible to write tests for it?

I tried writing a unit test for this, but a few things are iffy.

1) BP (and WP core) unit tests don't properly emulate a page load for multisite. I've added some code to somewhat address this in our go_to() method. (See unittests.patch)

2) The other problem is BP avatar's reliance on constants like BP_AVATAR_URL and BP_AVATAR_UPLOAD_PATH as a fallback.

The go_to() method usually tries to wipe out global variables to start clean and this works for the most part, but you can't wipe out defines.

By the time BP loads up in PHPUnit, the defines are already set.

This causes a problem when trying to use the go_to() method with a sub-site URL because bp_core_avatar_url() will just use the define instead of trying to get to the next conditional like a normal new page load would.

• Unit test will stop here - ​https://buddypress.trac.wordpress.org/browser/tags/1.7/bp-core/bp-core-avatars.php#L847
• And never gets a chance to parse this in multisite - ​https://buddypress.trac.wordpress.org/browser/tags/1.7/bp-core/bp-core-avatars.php#L865

This gives the illusion that the unit test will pass, but in reality, it should not.

[DJPaul]

If the WP core unit tests don't handle a multisite situation correctly, guess where I think the fix should go ;)

The other problem is BP avatar's reliance on constants like BP_AVATAR_URL and BP_AVATAR_UPLOAD_PATH as a fallback.

Let's introduce wrapper functions for these settings in 1.8, and deprecate direct use of the constant. For the unit tests, we can then add/use a filter.

[johnjamesjacoby]

Replying to DJPaul:

Let's introduce wrapper functions for these settings in 1.8, and deprecate direct use of the constant. For the unit tests, we can then add/use a filter.

I like this idea. The first patch seems fine enough for 1.7.1, and the second should work for 1.8. Then we should deprecate the constants that make sense to, and replace them with functions/filters like Paul said above.

[r-a-y]

In 6963:

Fix bp_core_avatar_url() when used on sub-sites (1.7-branch).

Mirrors changes made in r5897 to bp_core_avatar_upload_path().

See #4948.

[r-a-y]

In 6964:

Introduce bp_core_get_upload_dir().

This function fetches data from the BP root blog's upload directory.

Handy for multisite instances because all uploads are made on the BP root
blog and we need to query the BP root blog for the upload directory data.

This ensures that we only need to use switch_to_blog() once to get what we
need.

Use this function in bp_core_avatar_upload_path() and bp_core_avatar_url().

Fixes #4948.

[r-a-y]

Thanks for the feedback, guys.

I've:

• Added 01.patch to 1.7-branch
• Added 02.patch to trunk
• Created a new ticket to deprecate the avatar constants - #4965

[johnjamesjacoby]

These changes broke avatars that were previously working on multisite installations for me. Also, returning an empty string for the upload directory is potentially dangerous. Will need to give these another look for 1.7.2.

[johnjamesjacoby]

My only work-around was to manually set BP_AVATAR_URL to point to the correct location, so let's not deprecate anything completely until I/we can figure out what's happened.

[boonebgorges]

If the WP core unit tests don't handle a multisite situation correctly, guess where I think the fix should go ;)

Agreed in theory, but we're already overriding WP_UnitTestCase::go_to(), so upstream fixes wouldn't help us in the short term anyway. Let's fix in our own testcase, and also suggest upstream changes, and maybe someday we'll be able to join forces with WP core tests more efficiently.

[boonebgorges]

In 7058:

Improvements to BP_UnitTestCase::go_to() for multisite

When using go_to() to ape a pageload on a secondary site in multisite, a
number of global variables (such as $current_blog and $current_site, which
are normall set in ms-settings.php) must be reset for that secondary blog.
Having the globals set correctly is important for certain kinds of MS-specific
unit tests, such as test_avatars_on_non_root_blog().

As a convenience, this changeset introduces BP_UnitTestCase::go_to_root(),
which allows for the easy resetting of the relevant globals at the end of a
testcase where go_to() has been used on a secondary blog.

See #4948

Props r-a-y

[boonebgorges]

In 7060:

Adds unit test for avatar directory detection on non-root blogs.

See #4948

Props r-a-y

[boonebgorges]

In 7061:

Adds missing declaration from previous commit

See #4948

[johnjamesjacoby]

Moving to 1.8.1.

[johnjamesjacoby]

I forget what site was complaining, so moving this to 2.0.

[boonebgorges]

johnjamesjacoby - Would you mind chiming in here with info about what still needs to be fixed? It's not clear from the thread above just what remains broken.

[boonebgorges]

Seeing as the problematic site has somehow disappeared :) I'm going to close this ticket and assume it's a fluke not caused directly by BP. If anyone experiences the problem again, please post here with details.