Skip to:
Content

Opened 3 years ago

Closed 3 years ago

#6119 closed defect (bug) (fixed)

Imports cause PHP Warnings in bp_blogs_format_activity_action_new_blog_post

Reported by: DJPaul Owned by: imath
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Blogs Keywords: has-patch commit
Cc: imath

Description

When running a WordPress Import, the screen after you confirm the usernames generates a couple of PHP Warnings:

`

Notice: Undefined property: stdClass::$id in /srv/www/wordpress-develop/src/wp-content/plugins/buddypress/src/bp-blogs/bp-blogs-activity.php on line 127

Notice: Undefined property: stdClass::$id in /srv/www/wordpress-develop/src/wp-content/plugins/buddypress/src/bp-blogs/bp-blogs-activity.php on line 139
`

I used the WP theme unit test sample import for this. The line numbers may be different from trunk because I have am using a personal dev git branch. The backtrace shows they are coming from bp_blogs_format_activity_action_new_blog_post.

Attachments (3)

6119.patch (2.8 KB) - added by imath 3 years ago.
6119.02.patch (4.3 KB) - added by imath 3 years ago.
6119.03.patch (5.8 KB) - added by imath 3 years ago.

Download all attachments as: .zip

Change History (14)

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


3 years ago

#2 @imath
3 years ago

  • Component changed from Activity to Blogs
  • Keywords has-patch added

Hi DJPaul

Just checked with the WordPress importer plugin and did not manage to reproduce at first.
When importing the posts (exported from my personal blog), having the blogs component active, the activity are generated without any warnings due to BuddyPress. The only warnings are caused by the WordPress importer plugin (e.g.: Redefining already defined constructor for class WXR_Parser_Regex).

I'm sorry, i don't know about the WP theme unit test import tool you are talking about. On codex, i've found http://codex.wordpress.org/Theme_Unit_Test but when on import screen i don't get the 'Download and import file attachements' option mentioned at step 2 ??

So i've tested again with the WordPress importer plugin and the test file mentioned at step one and was able to reproduce.

The problem is due to the fact the imported post number 1169 has no title (edge case to test if the theme is placing the permalink on the post date)

Applying 6119.patch seems to fix the issue.

@imath
3 years ago

#3 @DJPaul
3 years ago

Right, that's what I was trying to explain.

If bp_blogs_format_activity_action_new_blog_post only happens when a new blog post is published, why would we ever already have the post title in the activity meta?

#4 @imath
3 years ago

bp_blogs_format_activity_action_new_blog_post not only happens when a new blog post is published. It also happens to dynamically generate activity action strings. See https://buddypress.trac.wordpress.org/ticket/3856#comment:20 at the 5th paragraph of boonebgorges 's explanations.

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


3 years ago

#6 @DJPaul
3 years ago

  • Keywords needs-patch added; has-patch removed

#7 @imath
3 years ago

  • Keywords has-patch added; needs-patch removed

in 6119.04.patch changed empty checks to isset ones when checking the post_title and post_url properties as recommended by boonebgorges in our slack discussion.

I've also added some comments to try to explain some points.

  • tested the "theme test" import file : no notice error
  • tested adding a new post without a title : no notice error
  • ran unit tests on non multi/multi modes : no failure

@imath
3 years ago

#8 @DJPaul
3 years ago

Thanks for your work on this ticket, imath. I think the comments help a lot to make the code more understandable.

I am reluctant to make this comment again given your hard work, but... :) I thought we decided when we spoke with Boone the other day that we did not need the following clause added:

// If activity already exists try to get the post title from activity meta 
} else if ( ! empty( $activity->id ) ) { 
  $post_title = bp_activity_get_meta( $activity->id, 'post_title' ); 

Chat link is https://wordpress.slack.com/archives/buddypress/p1421267309000163 Boone's reply is a few lines down:

If someone wants to update a post so that it has no title, we should respect that

Other than this last small point, the patch looks great.

#9 @imath
3 years ago

I haven't forgotten this part of our chat. And it's actually the case. If you test the patch you'll see that :
if i publish a post without a title, no notice
if i update a post having a title by removing the title, the activity action will be updated to inform the post has no title. This part is managed thanks to this function bp_blogs_update_post_activity_meta() line 467 to 468

You should check https://wordpress.slack.com/archives/buddypress/p1421267970000197 to understand why i've kept

// If activity already exists try to get the post title from activity meta 
} else if ( ! empty( $activity->id ) ) { 
  $post_title = bp_activity_get_meta( $activity->id, 'post_title' );

So in 6119.03.patch i've added a unit test to cover that possibility, but you'll see it will fail, that's because of #6127. You'll need to apply the patch i've added to 6127 to see test is not failing.

@imath
3 years ago

#10 @DJPaul
3 years ago

  • Keywords commit added

OK, I think I understand now. Thanks for taking the time to explain. Go for it!

#11 @imath
3 years ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 9369:

Fix notice errors when an activity is generated for a post having no title.

Post types activities are using dynamic action strings when the activity is displayed in the stream and when the activity is saved into the database. In this particular case, once the post is published, we are "faking" an activity object to which we add two extra properties : the post_title and the post_url. To avoid switching blogs too many times, we are saving the post title into an activity meta. To know if we must build the action string for insert or display, we are checking the post_title property of the activity object. When a post is published without a title, the empty() check was not right. Changing for an isset() check prevents the notice errors.

props boonebgorges

Fixes #6119

Note: See TracTickets for help on using tickets.