Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Photon: do not apply Photon on BuddyPress avatar change pages #57

Closed
wants to merge 2 commits into from

Conversation

@jeherve
Copy link
Member

commented Jan 7, 2014

We would need to disable Photon on BuddyPress Pages that allow you to change your avatar; Photon does indeed break the avatar cropping process.

photon-running

See this thread for more details.

@jeherve

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

@paulgibbs Do you think there would be another way to fix this issue in BuddyPress as well? Also, are there any other conditionals that should be added here.

@paulgibbs

This comment has been minimized.

Copy link

commented Jan 7, 2014

I'll think about it. Since you've got a filter there, it's probably nicer to use that rather than add a conditional in.

BTW: this will cause a fatal error on sites without BP active, because you've assumed those functions already exist, I think.

@jeherve

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

BTW: this will cause a fatal error on sites without BP active, because you've assumed those functions already exist, I think.

Good point. Oops!

The filter would be a nice addition to BuddyPress then!

@paulgibbs

This comment has been minimized.

Copy link

commented Jan 7, 2014

On reflection, I think Jetpack needs to add the filter as it's introducing the thing (Photon) that's causing the problem with BuddyPress. I'd suggest something like this, though it hasn't been at all tested: https://gist.github.com/paulgibbs/8306927

@jeherve

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2014

I tried the following, but the avatar cropping process still used Photon:

diff --git a/class.photon.php b/class.photon.php
index 71acc5f..ee49320 100644
--- a/class.photon.php
+++ b/class.photon.php
@@ -55,6 +55,9 @@ class Jetpack_Photon {
                add_filter( 'the_content', array( __CLASS__, 'filter_the_content' ), 999999 );
                add_filter( 'get_post_gallery', array( __CLASS__, 'filter_the_content' ), 999999 );

+               // BuddyPress' avatar cropping
+               add_action( 'bp_ready', array( $this, 'skip_buddypress_avatar' ) );
+
                // Core image retrieval
                add_filter( 'image_downsize', array( $this, 'filter_image_downsize' ), 10, 3 );

@@ -319,6 +322,19 @@ class Jetpack_Photon {
        }

    /**
+        * Do not apply Photon to BuddyPress' avatar cropping pages
+        *
+        * @uses bp_is_user_change_avatar, bp_is_group_admin_page, add_filter
+        * @return null|true
+        */
+   public function skip_buddypress_avatar() {
+               if ( ! bp_is_user_change_avatar() && ! bp_is_group_admin_page( 'group-avatar' )  )
+                       return;
+
+               add_filter( 'jetpack_photon_override_image_downsize', '__return_true' );
+   }
+
+   /**
         ** CORE IMAGE RETRIEVAL
         **/
@blobaugh

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2014

What is the status on this? Is it working as expected now?

I am not incredibly familiar with BuddyPress. Can you provide easy steps to repro (or the one that used to repro) this so I can test it with BuddyPress?

@jeherve

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2014

What is the status on this? Is it working as expected now?

Unfortunately not.

Can you provide easy steps to repro

  1. Install Jetpack and activate Photon
  2. Install and activate the BuddyPress plugin
  3. On your site's frontend, go to the "Members" page
  4. On that page, click on your username
  5. On the following page, click on Profile
  6. You will then be presented with a "Change avatar" option.
@ghost ghost assigned blobaugh Jan 15, 2014
@blobaugh

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2014

I will setup a BuddyPress install today and see what I can see

@blobaugh

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2014

To summarize the conversation I had with @jeherve about this issue: "Everything I tried led me straight back to your code. I think the issue lies with how the upload is handled. On upload the url is being saved as the Photon url. The cropper needs a local image to work."

@paulgibbs is there an easy way to determine when BuddyPress is dealing with the image upload to disable Photon?

blobaugh added a commit that referenced this pull request Jan 23, 2014
…hange

Currently when changing an avatar the photo is sent to Photon before the image cropping
step. This causes cropping to fail. This commit disables Photon before cropping the image.
See #57
@blobaugh

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2014

@jeherve I think I solved it. In a hacky way, but it seems to work on my tests.

The code is in the photon-bp branch. I created a new one as this PR has lots of code in it already and I wanted a clean slate.

https://github.com/Automattic/jetpack/compare/photon-bp

Test it and let me know how it works for you.

@jeherve

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2014

Test it and let me know how it works for you.

Seems to work for me too.

@blobaugh

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2014

Calling it good for now then.

Not merging any code from this PR

@blobaugh blobaugh closed this Jan 23, 2014
@jeherve jeherve deleted the jeherve:1737130-photon-bp branch Jan 23, 2014
@rightpalava

This comment has been minimized.

Copy link

commented Feb 6, 2014

Could you possibly help me out - really need to get the avatars working in BP and this looks to be the fix, but I can't fathom where these files in the '3rd-party' directory are supposed to live. Thank you!

@rightpalava

This comment has been minimized.

Copy link

commented Feb 6, 2014

I've actually found that re-ordering the order in which my scripts are called has helped, as in fixed this issue. Not ideal because I'll have to go through them all now to find the conflict, but essentially taking my javascript out of the footer has fixed it!

@imath

This comment has been minimized.

Copy link

commented Mar 29, 2015

Hi,

I think this check :

public function skip_buddypress_avatar() {
               if ( ! bp_is_user_change_avatar() && ! bp_is_group_admin_page( 'group-avatar' )  )
                       return;

might not be enough. When creating a group, you also have an avatar step :

if ( bp_is_group_create() && bp_is_group_creation_step( 'group_avatar' ) )
// group creation avatar step
@jeherve

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2015

@imath We ended up doing this differently, and hooking into bp_core_pre_avatar_handle_upload instead:
https://github.com/Automattic/jetpack/blob/master/3rd-party/buddypress.php

If that fix doesn't work anymore, let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.