Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#6545 closed defect (bug) (fixed)

Provide new XProfile field object with WPDB's insert_id

Reported by: Offereins Owned by: boonebgorges
Milestone: 2.4 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords:
Cc: lmoffereins@…

Description

For my plugin for #5192 I stumbled upon this missing logic during the creation of a new XProfile field. While this is done correctly for XProfile field groups, using wpdb->insert_id, the logic in BP_XProfile_Field::save() does not do so. It does not update the field object with the newly created field's id. This is unfortunate when you'd like to use the field's object in later hooks.

I suggest this easy fix by replacing:

if ( ! empty( $this->id ) ) {
  $field_id = $this->id;
} else {
  $field_id = $wpdb->insert_id;
}

// Other code. No other use of $field_id here.

return $field_id;

with

if ( empty( $this->id ) ) {
  $this->id = $wpdb->insert_id;
}

// Other code. No other use of $field_id here.

return $this->id;

in class-bp-xprofile-field.php.

PS. Excuse me for not providing a correct patch file at the moment.

Attachments (1)

6545.diff (2.7 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 follow-up: @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 2.4

Thanks for the patch. Unfortunately, this change by itself won't work, as there are a number of other empty( $this->id ) checks in the method.

#2 @boonebgorges
5 years ago

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

In 9983:

Set the id property of a new BP_XProfile_Field object during the save() method.

Props Offereins.
Fixes #6545.

#3 in reply to: ↑ 1 @Offereins
5 years ago

Replying to boonebgorges:

Thanks for the patch. Unfortunately, this change by itself won't work, as there are a number of other empty( $this->id ) checks in the method.

While this is true, the second empty-check results in the same: it uses either $this->id or $wpdb->insert_id. This check would become redundant since it just means $parent_id = $this->id. Am I right? Less is more :)

#4 @boonebgorges
5 years ago

Oh right, in that case, I suppose it would. That said, it obfuscates the code a bit to be doing those checks when unneeded. (Setting $this->id also would've result in the child node tree being deleted, though it would've had no real effect.) In any case, fixed now :)

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


5 years ago

#6 @Offereins
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry to say, but the fix doesn't solve my issue. The 'xprofile_field_after_save' hook has already fired with the field object before your new code steps in. Setting $this->id should at least happen some 10 lines earlier: https://buddypress.trac.wordpress.org/browser/trunk/src/bp-xprofile/classes/class-bp-xprofile-field.php?rev=9983#L371

@boonebgorges
5 years ago

#7 @boonebgorges
5 years ago

Thanks, Offereins. Can you please look at 6545.diff to see if it will accomplish what you want?

#8 @Offereins
5 years ago

Looks good to me.

#9 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 9986:

Better setting of id property in BP_XProfile_Field::save().

This ensures that the property is set before the after_save hook is fired.

Props Offereins.
Fixes #6545.

Note: See TracTickets for help on using tickets.