Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 17 months ago

#6658 closed defect (bug) (fixed)

Deleting xProfile field leaves meta behind

Reported by: garrett-eclipse Owned by: Offereins
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.3.3
Component: Extended Profile Keywords: commit
Cc: lmoffereins@…

Description

Hello,

I noticed when deleting xProfile fields that the associated meta (default_visibility & allow_custom_visibility) are left behind in the wp_bp_xprofile_meta table.

Cheers

Attachments (1)

6658.01.patch (1.9 KB) - added by Offereins 17 months ago.

Download all attachments as: .zip

Change History (15)

#1 @boonebgorges
4 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Thanks for the report! You're correct that we ought to clean up after ourselves here.

#2 @henry.wright
3 years ago

I did some testing with reference to what exactly remains in the database after a profile field is deleted.

  • The field's meta info (in table bp_xprofile_meta)
  • Each member's data relating to the deleted field (in table bp_xprofile_data)

#3 @DJPaul
3 years ago

Each member's data relating to the deleted field (in table bp_xprofile_data)

This isn't related to the meta table. I have a very faint memory that this might be intentional -- I think it might have been to do with otherwise destructive behaviour if you rename a xprofile field, then rename it back (you keep the data rather than loose it). Maybe JJJ or Boone remembers.

#4 @boonebgorges
3 years ago

[5598]. It only takes a click or two to delete a profile field. Deleting all the data associated with a profile field should be a much more involved process, with warnings and flashing lights, so that it's not performed accidentally. (A cleanup tool in core would be great, but it should be separate from - or an opt-in part of - the field-deletion process.)

#5 @needle
3 years ago

It's essential behaviour for any plugins that provide xProfile fields too. During the plugin upgrade process, the field definitions are lost - but at least the data remains.

I've had numerous support requests from people who upgraded my BP XProfile WordPress User Sync plugin rather than replacing the files as recommended. The procedure to reconnect the data with the fields created after the deactivation works most of the time, but not always.

My preference would be that fields could have a "hidden" flag so that fields don't need to be deleted on plugin deactivation, but I would definitely not want mandatory automatic deletion of field data when a field is deleted.

#6 @henry.wright
3 years ago

@boonebgorges have you tried deleting a GitHub repo? I noticed it asks you to type out the name. Maybe something like that where the admin has to type the name of the field before hitting delete.

#7 @DJPaul
3 years ago

I think a confirmation button/prompt which'd "delete all data" as well as a tool that nukes all orphaned data.

#8 @DJPaul
3 years ago

  • Keywords good-first-bug removed

#9 @r-a-y
2 years ago

#7521 was marked as a duplicate.

#10 @Offereins
17 months ago

  • Cc lmoffereins@… added

So, deleting metadata is still on the table, right? I'd like to take this on, but preferably someone can suggest which way is preferred:

Deleting metadata...

  • a) with a single $wpdb query or
  • b) using bp_xprofile_delete_meta() which fires subsequent hooks in delete_metadata

when...

  • i) right in BP_XProfile_Field::delete() or
  • j) hooked on the xprofile_field_after_delete action (since 3.0)?

#11 @boonebgorges
17 months ago

Thanks for working on this, @Offereins!

b) using bp_xprofile_delete_meta() which fires subsequent hooks in delete_metadata

I'd suggest this. bp_xprofile_delete_meta( $field_id, 'field' ) (no $meta_key) calls delete_metadata(), which assembles a single DELETE query for all metadata entries and fires all the hooks appropriately. This appears to be what we do with groups, so it'll be consistent.

right in BP_XProfile_Field::delete()

I guess I'd suggest this, since it's what we do with groups and activity. If there's good justification for doing it in a hook (like, making it unhookable), let's handle it separately, and change all object types over at once.

@Offereins
17 months ago

#12 @Offereins
17 months ago

  • Keywords needs-patch needs-unit-tests removed
  • Milestone changed from Awaiting Contributions to 3.0

Just added a patch following @boonebgorges's suggestions.

#13 @boonebgorges
17 months ago

  • Keywords commit added
  • Owner set to Offereins
  • Status changed from new to assigned

Patch looks good to me.

#14 @offereins
17 months ago

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

In 11847:

XProfile: delete field metadata when a field is deleted

Previously, field metadata remained in the database after a profile field
was deleted. Using bp_xprofile_delete_meta() all field metadata will
now be deleted while all relevant metadata hooks are fired.

Adds unit tests for default and custom field metadata.

Fixes #6658.

Note: See TracTickets for help on using tickets.