Skip to:
Content

Opened 12 months ago

Closed 3 weeks ago

#4948 closed defect (bug) (fixed)

bp_core_avatar_url() wonky in sub-sites since WP 3.5

Reported by: r-a-y Owned by: r-a-y
Milestone: 1.7.1 Priority: normal
Severity: normal Version: 1.6.4
Component: Core Keywords: needs-unit-tests
Cc: mercijavier@…

Description

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().

Attachments (3)

4948.01.patch (1.1 KB) - added by r-a-y 12 months ago.
4948.02.patch (5.2 KB) - added by r-a-y 12 months ago.
unittests.patch (5.9 KB) - added by r-a-y 12 months ago.

Download all attachments as: .zip

Change History (27)

r-a-y12 months ago

comment:1 boonebgorges12 months ago

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.

comment:2 luccame12 months ago

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

comment:3 r-a-y12 months ago

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.

comment:4 boonebgorges12 months ago

  • Keywords needs-unit-tests added

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-y12 months ago

r-a-y12 months ago

comment:5 r-a-y12 months ago

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.

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

comment:6 follow-up: DJPaul12 months ago

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.

comment:7 in reply to: ↑ 6 johnjamesjacoby12 months ago

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.

comment:8 r-a-y12 months ago

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.

comment:9 r-a-y12 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:10 r-a-y12 months ago

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

comment:11 johnjamesjacoby12 months ago

  • Milestone changed from 1.7.1 to 1.7.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:12 johnjamesjacoby12 months ago

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.

comment:13 boonebgorges11 months ago

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.

comment:14 boonebgorges11 months ago

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

comment:15 boonebgorges11 months ago

In 7060:

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

See #4948

Props r-a-y

comment:16 boonebgorges11 months ago

In 7061:

Adds missing declaration from previous commit

See #4948

comment:17 mercime11 months ago

  • Cc mercijavier@… added

comment:18 r-a-y11 months ago

  • Milestone changed from 1.7.2 to 1.7.3

comment:19 johnjamesjacoby9 months ago

  • Milestone changed from 1.7.3 to 1.8.1

Moving to 1.8.1.

comment:20 boonebgorges9 months ago

  • Milestone changed from 1.8.1 to 1.8.2

comment:21 johnjamesjacoby5 months ago

  • Milestone changed from 1.8.2 to 2.0

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

comment:22 boonebgorges6 weeks ago

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.

comment:23 DJPaul6 weeks ago

  • Keywords has-patch removed

comment:24 boonebgorges3 weeks ago

  • Milestone changed from 2.0 to 1.7.1
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.