Skip to:
Content

Opened 2 years ago

Last modified 23 months ago

#6931 reopened defect (bug)

Cover Image location is incorrect for blogs other than the primary blog

Reported by: rbaccaro Owned by:
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version: 2.4.0
Component: Media Keywords: 2nd-opinion
Cc:

Description

Scenario:

  • multisite
  • BP is network activated
  • define( 'BP_ENABLE_MULTIBLOG', true ) is set
  • fresh and new installation
  • only BP installed as plugin
  • default theme

The image cover path and url are using the current blog location as for example
"/uploads/sites/[BLOG_ID]/buddypress/members/[USER_ID]/cover-image/".

When the same user goes to a different blog, his cover image is not fetched, making the same user to be able to have more than one cover image, visiting different blogs.

I think BP's intention is to have a single cover image per user/group across the network of blogs, and therefore the upload location for the primary blog should be used for any blog in the network.

IMHO, it does not make sense to have more than one location for cover images, since wp_users and wp_usermeta are tables with fields shared across the network as well as xprofile component.

BP had this same issue before when introduced avatar as solved by this ticket https://buddypress.trac.wordpress.org/ticket/2317.

For example, on my testing:

Blog 1: CORRECT path for a network activated BP:

– /wp-content/uploads/buddypress/members/[USER_ID]/cover-image/....-bp-cover-image.png

Blog 2: WRONG path for a network activated BP:

– /wp-content/uploads/sites/[BLOG_ID]/buddypress/members/[USER_ID]/cover-image/....-bp-cover-image.png

Note that "/sites/[BLOG_ID]" is included on blog 2, which makes impossible to share the cover image across the network.

IMHO, BP needs to remove '/sites/[BLOG_ID]' from upload path and url when is a multisite installation, BP network activated and BP_ENABLE_MULTIBLOG is true.

I guess that the BP cover image file core that need to be fixed is located here
https://github.com/buddypress/BuddyPress-build/blob/master/bp-core/classes/class-bp-attachment-cover-image.php

Attachments (2)

6931.patch (1.2 KB) - added by imath 2 years ago.
6931.02.patch (1.3 KB) - added by imath 2 years ago.

Download all attachments as: .zip

Change History (21)

#1 @imath
2 years ago

  • Component changed from Component - XProfile to Component - Attachments
  • Keywords has-patch 2nd-opinion added; needs-patch removed
  • Milestone changed from Awaiting Review to 2.6
  • Priority changed from highest to normal
  • Severity changed from critical to normal

Hi @rbaccaro

Thanks for your feedback. First i disagree with you when you say :

BP had this same issue before when introduced avatar as solved by this ticket #2317

I advise you to check again your avatars, this is what i get with a fresh install :
https://cldup.com/641VTXAz6O.png

So it's not a something *specific* to cover images, but something *generic* to BuddyPress Attachmments (i'm including avatars of course)

Second, i understand your point, but i'm not sure every community administrator will want this set up, and will want it for any object using the BP Attachment API.

Third, if you want to be sure to already have it in your community for Avatars, Cover Images and every object using the BP Attachment API, you can add the following gist inside your bp-custom.php file:
https://gist.github.com/imath/91d56292673e6de61597

Fourth, i've added a patch we can discuss on during next dev-cycle, i mostly agree about using common uploads dir in case of a multiblog setup, just want to make sure we all do :)

@imath
2 years ago

#2 @rbaccaro
2 years ago

Hi @imath,

Thank you for your inputs. My feedback is only 2 cents compared to what BP community hard work gave me already.

1) Avatar: I see. But if I remove this function from bp-custom.php, the avatar is not fetched for the same user in different blogs, note that I have network activated BP and define( 'BP_ENABLE_MULTIBLOG', true ), sorry if I mentioned the wrong ticket, but this is the code:

add_filter('bp_core_avatar_upload_path', 'nfm_bp_avtar_upload_path_correct', 1);

function nfm_bp_avtar_upload_path_correct($path){
    if ( is_multisite() ){
     //   $path = ABSPATH . get_blog_option( BP_ROOT_BLOG, 'upload_path' );
                $path = ABSPATH . 'wp-content/uploads/';
    }
    return $path;
}

add_filter('bp_core_avatar_url', 'nfm_bp_avatar_upload_url_correct', 1);

function nfm_bp_avatar_upload_url_correct($url){
    if ( is_multisite() ){
        $url = get_blog_option( BP_ROOT_BLOG, 'siteurl' ) . "/wp-content/uploads";
    }
    return $url;
}

2) MS installation: true, not everyone, but our multisite setup is more common than we may think, IMHO, for example, it is used to support multilingual versions of a specific website, because we can set different languages for different blogs, instead of installing a plugin, as WMPL, for a single site. But I agree, probably, it is not common to add BP as network and multiblog activated on the top of that, but that is the challenge.

3) Gist: thank you so much! I will try and let you know.

4) Sure and agree, and since you mentioned, yes, the /uploads/buddypress/cover-image/ dir for any blog would be a great idea as well as move the /avatar/ dir under /uploads/buddypress/ as you already mentioned here https://buddypress.trac.wordpress.org/ticket/6925#comment:3

Thank you,

Last edited 2 years ago by rbaccaro (previous) (diff)

#3 @rbaccaro
2 years ago

  • Milestone 2.6 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Hi @imath,

...

3) Yes, your gist located here https://gist.github.com/imath/91d56292673e6de61597
worked just fine.

...

Thank you again,

#4 @imath
2 years ago

  • Milestone set to 2.6
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hi @rbaccaro

With the gist, i think you don't need the code you shared in #comment:2 it should work for every object using the BP Attachment API (avatars/ cover images...)

I actually think we need to improve this part but take the time to check changes introduced would not have side effects on plugins extending the Attachment API etc..

I think for multiblog we can do something like what i'm suggesting in the patch attached to this ticket. So i'll leave the ticket open so that others can comment/ give their feedback.

Thanks again for your feedback :)

#5 @rbaccaro
2 years ago

Hi @imath,

Yes, you are right, your gist located here https://gist.github.com/imath/91d56292673e6de61597
works without the avatar filter mentioned in #comment:2, item 1.

Thanks again,

#6 follow-up: @espellcaste
2 years ago

@imath What about creating an upload function where developers can overwrite or change to their liking?

BP_Attachments allows to define a custom folder where the file will be uploaded but only Avatar class has such a function to change the url path.

This new function would take care of backward comparability and specific user cases such as the one outlined by @rbaccaro.

#7 in reply to: ↑ 6 @rbaccaro
2 years ago

Hi @espellcaste, that would be great. Thanks for the input.

Replying to espellcaste:

@imath What about creating an upload function where developers can overwrite or change to their liking?

BP_Attachments allows to define a custom folder where the file will be uploaded but only Avatar class has such a function to change the url path.

This new function would take care of backward comparability and specific user cases such as the one outlined by @rbaccaro.

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


2 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#10 @imath
2 years ago

  • Keywords commit added

if no objections, i'll commit the patch tonight.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


2 years ago

#12 @imath
2 years ago

  • Keywords needs-patch added; has-patch commit removed

After some more thoughts about it: i think the patch needs some more work. Today Multiblog means: each blog has its own BuddyPress uploads dirs (avatars/buddypress...) The patch is changing this, so there's a good chance it will generate some mess to existing installs.

We should probably keep it the way it is unless a filter is triggered to use a common folder.

#13 @espellcaste
2 years ago

I agree, but what about a filter to change the uploads dirs of a added/custom component? As I mentioned, the avatar component makes it pretty easy to accomplish, the other or custom ones, don't.

#14 @imath
2 years ago

@espellcaste i'm not sure to understand. If you're talking about this > https://buddypress.trac.wordpress.org/browser/trunk/src/bp-core/classes/class-bp-attachment-avatar.php#L72

I'd say any component using the BP Attachment class can override this method and add custom filters. If i misunderstood, can you be more precise ? like what functions/filters you have in mind.

@imath
2 years ago

#15 @imath
2 years ago

  • Keywords has-patch added; needs-patch removed

02.patch makes it possible to use common uploads dirs for multiblog configs doing so:

add_filter( 'bp_multiblog_common_upload_dir', '__return_true' );

By default, each blog will have their own uploads dir, just like it was so far.

#16 @espellcaste
2 years ago

Never mind, gonna test your patch!

#17 @imath
2 years ago

  • Milestone changed from 2.6 to Future Release

#18 @DJPaul
2 years ago

  • Component changed from Component - Attachments to Media

#19 @DJPaul
23 months ago

  • Keywords has-patch removed

@imath how do you feel about the .2.patch? Is it something we need?

Note: See TracTickets for help on using tickets.