Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 4 years ago

#6937 closed defect (bug) (fixed)

fatal error when trashing emails

Reported by: DJPaul Owned by: r-a-y
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.5.0
Component: Blogs Keywords: has-patch commit
Cc: imath

Description

When clicking “rebuild emails…” and clicking “empty trash” in Emails -> All emails -> Trash
i get error:
Fatal error: Call to undefined function bp_blogs_delete_activity() in ..../public_html/wp-content/plugins/buddypress/bp-blogs/bp-blogs-functions.php on line 914

From https://buddypress.org/support/topic/fatal-error-bp_blogs_delete_activity-after-clicking-empty-trash-in-emails/

Attachments (2)

6937.patch (4.0 KB) - added by imath 5 years ago.
6937.activity.patch (9.0 KB) - added by r-a-y 5 years ago.

Download all attachments as: .zip

Change History (30)

#1 @DJPaul
5 years ago

This might be if the activity component is disabled (bp-blogs-activity.php is only loaded then). Have asked reporter to confirm.

#2 follow-up: @DJPaul
5 years ago

I'm guessing this was introduced possibly by the post type comment tracking change.

#3 follow-up: @DJPaul
5 years ago

See also https://buddypress.org/support/topic/php-fatal-error-possible-bug/ for similar but maybe the same root cause.

#4 @mi423
5 years ago

https://buddypress.org/support/topic/fatal-error-bp_blogs_delete_activity-after-clicking-empty-trash-in-emails/#post-250533

Activity is disabled.

Enabled components:
Profile extended
Account settings,
Private messages,
Notifications,
Post track? (third from botom ;) srr translated on the go... )

Last edited 5 years ago by mi423 (previous) (diff)

#5 in reply to: ↑ 3 @nyodulf
5 years ago

Replying to DJPaul:

See also https://buddypress.org/support/topic/php-fatal-error-possible-bug/ for similar but maybe the same root cause.

The activity streams component is disabled on the site where that error occurred.

#6 in reply to: ↑ 2 ; follow-up: @imath
5 years ago

Thanks a lot for your comments @mi423 I'm currently working on a patch.

What i'm amazed of is : how come you didn't noticed it in previous versions of BuddyPress ?

I'm pretty sure this could happen with BuddyPress 2.4.3 having the same components activated.

I mean in the end bp_blogs_delete_activity() is using bp_activity_delete_by_item_id() which is located in the Activity component.

I'm guessing this was introduced possibly by the post type comment tracking change.

@DJPaul The only change that could have caused this (although i think it's there for quite a while) is the fact we include /bp-blogs/bp-blogs-activity.php only if the Activity component is active. The function bp_blogs_remove_post() wasn't touched during 2.5.0 dev cycle.

#7 in reply to: ↑ 6 @mi423
5 years ago

Replying to imath:

What i'm amazed of is : how come you didn't noticed it in previous versions of BuddyPress ?

I'm pretty sure this could happen with BuddyPress 2.4.3 having the same components activated.

I have no idea :) I do not remember if I've seen this error before.

#8 @imath
5 years ago

@DJPaul @mi423

Sorry I spoke too quick.

I see now what caused this. A bunch of if ( ! bp_is_active( 'activity' ) ) { checks were removed inside functions such as bp_blogs_delete_activity() because the whole file is now loaded only if the activity component is active.

I now see it was a really bad idea.

Last edited 5 years ago by imath (previous) (diff)

#9 @imath
5 years ago

  • Keywords has-patch needs-testing added

Here's a first patch. I'm going to run some tests and will add a new comment to explain my choice once done.

@imath
5 years ago

#10 @imath
5 years ago

  • Keywords needs-testing removed

So here's a detailled explanation

In r10544 we did two things:

  • Only load bp-blogs/bp-blogs-activity.php if the Activity Component was active
  • Remove the bp_is_active( 'activity' ) checks inside bp_blogs_record_activity() and bp_blogs_delete_activity()

This means before this commit, the file was loaded and the checks were making sure the activity component was active before using an Activity component function. Reason why this trouble appeared in 2.5.0 and not before.

I keep on thinking only loading the bp-blogs/bp-blogs-activity.php if the Activity Component is active is a good improvement as it avoids loading a bunch of code that is not used anyway. But this means we now need to do the bp_is_active( 'activity' ) before using bp_blogs_record_activity() or bp_blogs_delete_activity().

That's what is doing 6937.patch. I've tested it having the Blogs component active and the Activity one deactivated on multisite/non multisite and it's fixing the issue.

#11 @mi423
5 years ago

6937.patchworks for me.
No error on empting trash ;)

cheers
GJ

#12 follow-up: @r-a-y
5 years ago

I keep on thinking only loading the bp-blogs/bp-blogs-activity.php if the Activity Component is active is a good improvement as it avoids loading a bunch of code that is not used anyway.

Darn it! This was my fault! :(

Here's an alternate patch that moves the activity function calls from bp-blogs-functions.php to bp-blogs-activity.php.

Benefits are no more bp_is_active() checks and code is hooked. The code changes may be too extreme for v2.5.1, so I'm thinking we roll with imath's fix for v2.5.1 and look at my patch for v2.6.0 instead.

To whomever commits, feel free to give me anti-props! :)

#13 in reply to: ↑ 12 @imath
5 years ago

Replying to r-a-y:

Darn it! This was my fault! :(

I should have checked too, so we're on the same boat :)

I really like 6937.activity.patch 's approach. Thanks for adding it.

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


5 years ago

#15 @DJPaul
5 years ago

🚣

#16 @DJPaul
5 years ago

  • Keywords commit added

What's the proposed plan? imath's patch for 2.5.1, r-a-y's for 2.6, or what?

I have not tested either patch but both patches look good, so please proceed.

#17 @imath
5 years ago

I think it's safest to go with 6937.patch for 2.5.1 as @mi423 confirmed it was fixing the issue.

Once committed, i'll leave this ticket open so that we can improve it the way @r-a-y suggested in his patch.

#18 @imath
5 years ago

In 10631:

Make sure the Activity Component is active before using functions relative to it inside the Blogs component.

r10544 introduced a side effect on the BuddyPress Emails feature when the Blogs component is activated and the Activity one is deactivated.

This changeset removed some bp_is_active() checks inside bp_blogs_delete_activity() and bp_blogs_record_activity() in favor a conditional load of the whole bp-blogs/bp-blogs-activity.php file.

Unfortunately, an issue appeared when the Activity component is not active because these functions are used when some WordPress hooks are fired, for instance when a post type is deleted.

For the 2.5 Branch we are bringing back the bp_is_active() checks before using the two functions mentioned earlier. For next release, we will carry on moving all the code specific to the activity component inside the bp-blogs/bp-blogs-activity.php file.

See #6937 (branch 2.5)

#19 @imath
5 years ago

In 10632:

Make sure the Activity Component is active before using functions relative to it inside the Blogs component.

r10544 introduced a side effect on the BuddyPress Emails feature when the Blogs component is activated and the Activity one is deactivated.

This changeset removed some bp_is_active() checks inside bp_blogs_delete_activity() and bp_blogs_record_activity() in favor a conditional load of the whole bp-blogs/bp-blogs-activity.php file.

Unfortunately, an issue appeared when the Activity component is not active because these functions are used when some WordPress hooks are fired, for instance when a post type is deleted.

For the 2.5 Branch we are bringing back the bp_is_active() checks before using the two functions mentioned earlier. For next release, we will carry on moving all the code specific to the activity component inside the bp-blogs/bp-blogs-activity.php file.

See #6937 (trunk)

#20 @imath
5 years ago

  • Keywords commit removed
  • Milestone changed from 2.5.1 to 2.6

#21 @DJPaul
5 years ago

Is 6937.activity.patch ready for review/testing, or does it require further work?

#22 @DJPaul
5 years ago

  • Keywords needs-patch added; has-patch removed

Let's refresh Ray's patch and get it in.

@r-a-y
5 years ago

#23 @r-a-y
5 years ago

  • Keywords has-patch added; needs-patch removed

activity.patch is refreshed.

Before committing, this patch does introduce three new functions for hook purposes:

  • bp_blogs_record_activity_on_site_creation()
  • bp_blogs_delete_new_blog_activity_for_site()
  • bp_blogs_delete_activity_for_site()

Let me know if any of these functions should be renamed to something more palatable.

#24 @DJPaul
5 years ago

Is this done? Can we commit it?

#25 @r-a-y
5 years ago

See what I wrote in comment:23. I just need a quick check on the names of the new functions before committing.

#26 @DJPaul
4 years ago

  • Keywords commit added

@r-a-y Names seem fine, nice and descriptive!

#27 @r-a-y
4 years ago

In 10730:

Blogs: Introduce new parameter - $no_activity to 'bp_blogs_new_blog' action.

See #6937.

#28 @r-a-y
4 years ago

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

In 10731:

Blogs: Move activity-related code from bp-blogs-functions.php to bp-blogs-activity.php.

Instead of hardcoding activity code and needing to check for the Activity
component in bp-blogs-functions.php, this commit moves this activity code
code to bp-blogs-activity.php and also hooks this code to appropriate
blog actions.

This allows developers to easily remove said activity hook if needed.

Fixes #6937.

Note: See TracTickets for help on using tickets.