Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#5220 closed enhancement (fixed)

Overhaul implementation of xprofile field types

Reported by: djpaul's profile DJPaul Owned by: djpaul's profile DJPaul
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: has-patch dev-feedback early
Cc: donmik, vivek@…, shane@…

Description (last modified by DJPaul)

It's pretty hard to add a new type of profile field to BuddyPress; some plugins such as this one do a good job, but we make it hard because we hardcoded values and checks and templates across several places. Examples of new profile field types might be inspired by HTML5 input types such as number or email (#4694), or a birthday field (#4582), or advanced types like a map/co-ord mashup (#5028).

Places that need changing (currently via actions or filters) when you add a new profile field type are listed below. There may be more, as I'm still re-discovering where everything is:

  • Templates in /members/single/profile/edit.php, /members/register.php, BP_XProfile_Field->render_admin_form().
  • Logic in bp_core_screen_signup(), xprofile_screen_edit_profile() and xprofile_set_field_data().

I believe profile types should be implemented as a class; there's a lot of places where we can benefit from an object orientated approach to profile field types. And, eventually, I'd like it to be as easy to add a profile field type to BuddyPress as creating a Group extension is. No changes to other files, filters, or actions should be required.

Attachments (6)

5220.01.patch (120.9 KB) - added by DJPaul 10 years ago.
5220.02.patch (121.3 KB) - added by DJPaul 10 years ago.
5220.03.diff (130.8 KB) - added by imath 10 years ago.
5220.04.patch (121.5 KB) - added by DJPaul 10 years ago.
5220.04b.patch (5.2 KB) - added by r-a-y 10 years ago.
Requires 04.patch - Updated to fix unit test
5220.05.patch (125.6 KB) - added by DJPaul 10 years ago.

Download all attachments as: .zip

Change History (40)

#1 @DJPaul
10 years ago

  • Cc donmik added

#2 @DJPaul
10 years ago

  • Description modified (diff)

Fixing link in description

#3 @DJPaul
10 years ago

This weekend i've been iterating on some proof-of-concept patches that I'd written a couple of years ago. Find them on https://github.com/paulgibbs/BuddyPress/tree/xprofile-types, and I think this will give you a diff of the changes against core trunk. The idea is to move all of BP's repeated "what type of field is this" conditional checks into a central place to make the code less fragile, and make it be tested easier, and to give BuddyPress a framework for custom profile field types.

  • The patch/branch implements a new "number" profile field type.
    • Support for checkbox, datebox, and radio boxes haven't been re-implemented yet.
  • A new class, BP_XProfile_Field_Type, describes the types of values that a field type accepts and handles validation, and handles template output for the "edit profile" screens.
    • There are unit tests which should prove the validation works, though I have not been able to run those because of recent difficulties on my laptop with phpunit.
  • The wp-admin Users > Profile Fields screen has been updated to support the new approach (the "Field Type" dropdown).
  • The front-end "save profile" logic has also been updated to support the new approach.

Areas not identified or not listed haven't been changed yet, as I want to gather some feedback on this approach to see if it's popular, or if it helps guide us towards the best solution.

#4 @r-a-y
10 years ago

This looks great!

Paul, your link to generate the diff is missing the .diff / .patch suffix:
https://github.com/paulgibbs/BuddyPress/compare/buddypress:master...xprofile-types.diff

Once I've had a chance to test the patch, I'll add some additional feedback.

#5 @DJPaul
10 years ago

Ray, did you get a chance to look at this?

#6 @johnjamesjacoby
10 years ago

  • Keywords early added
  • Milestone changed from Awaiting Review to 2.0
  • Owner set to DJPaul
  • Status changed from new to assigned

This is pretty neat, and definitely a direction I think we should go in. Let's revisit the possibility of enhancing this in 2.0.

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.


10 years ago

#8 @sooskriszta
10 years ago

Related #4694, #5028 and #4796

Last edited 10 years ago by sooskriszta (previous) (diff)

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.


10 years ago

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.


10 years ago

#11 @sooskriszta
10 years ago

  • Cc vivek@… added

#12 @DJPaul
10 years ago

I'm getting close to finishing work on a patch for this ticket, and have run into a few areas where I need help. These three functions have special-cases for various types of profile field:

  • xprofile_format_profile_field()
  • xprofile_filter_format_field_value()
  • xprofile_filter_link_profile_data()

Without going into my suggested implementation in detail yet, the latter two functions I intend to not change for this ticket because they are both implemented as filters, which will allow custom field types to manipulate data in a similar way.

However, I think xprofile_format_profile_field() needs some re-factoring because it's used inside two other functions and has special handling for the datebox field type. The phpDoc description even says "TODO: Should really be moved to filters", and it's been like that since sometime before the pre-1.0rc2 tag was created :)

Can anyone remember what that comment had in mind, or can anyone see a good way of restructuring this code? In the worse case, I will simply add a filter onto the return value of this function and call it a day.

#13 @boonebgorges
10 years ago

It's hard to know what kind of restructuring would work without knowing more about how your rewrite works. If you're planning to eliminate the function altogether, I think it'd be something like this (and this is what I assume the comment means):

add_filter( 'bp_get_member_profile_data', 'bp_unserialize_profile_field' );
add_filter( 'bp_get_member_profile_data', 'stripslashes_deep' );

Probably will also want to pass the field type along to the 'bp_get_member_profile_data' filter, and then have specific filter callbacks that detect type before bailing out. Alternatively, you could have dynamic filters:

$data = apply_filters( 'bp_get_member_profile_data', $data );
$data = apply_filters( 'bp_get_member_profile_data_' . $type, $data );

then add type-specific filters

add_filter( 'bp_get_member_profile_data_datebox', 'bp_format_time' );

Does that help?

@DJPaul
10 years ago

#14 @DJPaul
10 years ago

  • Keywords has-patch dev-feedback added

I've just put up a first patch for the ticket. You can also compare it to trunk over on Github https://github.com/paulgibbs/BuddyPress/compare/master...xprofile-types and I'd prefer any follow-up patches to be submitted to that xprofile-types branch as a pull request. But if you want to make a new patch from SVN, don't let me stop you -- I can merge manually back to Github, it'd just save me some time. Ta.

This patch is primarily intended to de-duplicate profile field templating and validation logic in order to make it easier for core and plugins to add new profile field types. Profile field templating was duplicated in the registration template, member/profile/edit template, and since recently, also in the new wp-admin user profile editor, as well as in parts of the wp-admin new/edit profile field screens.

The approach creates a new class for each profile field type; selectbox, textbox, textarea, and so on. They all share an abstract base class BP_XProfile_Field_Type which abstract away a lot of special behaviour that has been added over the years (e.g. some fields accepting null values, some accepting multiple default values, and more), and adds true field value validation.

As a demonstration of a new field type, I've implemented a new "Numbers" type. It behaves much the same as a regular textbox field does, but only accepts numbers. The new unit tests are the best example of how the new field value validation can be used.

I'm looking forward to code review, testing, and feedback.

#15 @boonebgorges
10 years ago

I've just had a good look through. Very nice work throughout. An enormous improvement on what is, IMO, one of the sloppiest parts of BuddyPress.

I've sent a pull request with a couple tiny things.

I don't really have any comments about approach or philosophy. The way you've structured the base class; your validation mechanisms; the pretty chainable syntax; your abstraction of quirks like 'supports_options' and 'accepts_null_value' - all of these are lovely. I'm already imagining the kinds of additional field types that I'm going to build. As far as I'm concerned, it's all but ready to go.

A couple random notes I took while looking through:

  1. The markup for the checkbox and radiobutton types doesn't seem to be quite right. The '<label>' element is wrapped around the input, and doesn't have a 'name' attribute. I know we already do this in a number of places in BP, but I think it's poor form and this wolud be a good chance to fix it.
  2. I came across some bugs with the javascript in the admin section when creating a new field. Some are present in trunk (field type options fields only appear onchange, so if you load the page with one selected because of your browser cache, they're not there). But some are new. There's an [x] to remove the first option on a supports_option field, but the js attached to it is broken because of some odd selectors (in trunk the x is not there, though perhaps it should be). A fairly small issue of course.
  3. I see in is_valid() that you've chosen not to validate if no rules are matched, even when no rules have been set. Then, in most of the generic fields, you've set_format() with a generic /.+$/ rule. A couple questions about this. One, this means that any new field_type will have to register at least one always-true regular expression in order for anything to validate. Do we want this? What patterns are we trying to encourage here - just to get plugin devs to think about validation? Because it'd be just as easy for us to default to validated = true, or to skip when no rules are registered. Two, /.+$/ will fail for empty strings. I know there's separate 'required' logic - is this an unnecessary restriction?
  4. The "(required)" text seems to be repeated almost verbatim in almost every instance of edit_field_html(). Maybe it can be abstracted?

Thanks for your great work on this!

#16 @DJPaul
10 years ago

Thanks for taking a detailed look at the patch; I appreciate it. I left a comment on your github pull request as it seems there was a small mistake in using _ instead of an _e function in a few lines, but all the other suggestions look fine. I'll keep an eye on github and merge the pull request in once resolved.

The markup for the checkbox and radiobutton types doesn't seem to be quite right.
The "(required)" text seems to be repeated almost verbatim in almost every instance of edit_field_html().

I don't disagree that these stand out as pain points, but so far in this patch, I've taken the approach of being very conservative about not changing code that doesn't strictly need to be changed for the purpose of implementing the new field type classes. Mainly I didn't want to add all this new stuff and change some of the existing templating markup in one go. I think I would prefer dealing with these in separate tickets so we can get more opinions on those specific parts.

The "(required)" text seems to be repeated almost verbatim in almost every instance of edit_field_html().

For this specifically, if it's abstractable, sure. I think however the real problem is that this probably sucks as a translatable string (no comments or context, plus it's a bit weird), and improving that would be a higher priority.

There's an [x] to remove the first option on a supports_option field, but the js attached to it is broken because of some odd selectors

I'll check this out and get it fixed; thanks.

I see in is_valid() that you've chosen not to validate if no rules are matched, even when no rules have been set...

I don't have a strong opinion which way we do this. I *would* like developers to make decisions about the formats that their custom field types accept.

Two, /.+$/ will fail for empty strings... is this an unnecessary restriction?

I can't see the issue. I've just added more tests. What type of profile field are you thinking of? e.g. For textboxes, it uses /.*/ instead.

#17 @DJPaul
10 years ago

There's an [x] to remove the first option on a supports_option field, but the js attached to it is broken because of some odd selectors

I'll check this out and get it fixed; thanks.

Fixed in eb365c5. I'll update the patch here once the other fixes land.

EDIT: actually, removed the entire link in 65d678d since that made more sense. The [x] fields are added in with the JS.

Last edited 10 years ago by DJPaul (previous) (diff)

@DJPaul
10 years ago

#18 @DJPaul
10 years ago

Uploaded 5220.02.patch

  • Includes Boone's changes sent via Github
  • I added another unit tests and a handful of more assertions to existing tests to better test the behaviour of whether a field accepts an empty string or not.

@imath
10 years ago

#19 @imath
10 years ago

Hi Paul,

I've just tested your patch. Great work. Just found some details, i'm attaching in 5220.03.diff.

  1. Most important of the details. We've already discussed about it previously: in wp-admin/profile, unlike on the front end all group of fields are displayed on the same page, so the input name for field_ids must be field_ids[] else, you won't save all profile fields. If you have 3 groups of fields, only the fields of the third group of fields should be saved, 1 and 2 not..
  2. While editing the choices for a "multiple" value field like checkbox, radio.. The click on the [x] is hiding the option, but does not reset the value. As a result the option is saved.
  3. Multiple select: when you choose more than one option, it's not saving all the options just one and the clear link is not working. Reason is the name of a multiselect must be name[]. So i've just went back on current trunk for this one.
  4. Maybe you should use the bp_parse_args() function introduced by jjj ;)

#20 follow-up: @DJPaul
10 years ago

Thanks imath. So far, I've merged in three of your changes: bc324f0 and b3578f1 and 879b5c5.

This patch -- https://gist.github.com/paulgibbs/9476911 -- is that related to point 2 above? If so, I don't see how this is a new problem introduced this patch, as I haven't touched that JS file. If it's a new bug, I need to understand how it was introduced, so I have left it out for now so we can test BuddyPress trunk to see if this is an existing bug.

The multiple select change you spotted is interesting. I made that change on purpose to take the special handling of the multiselectbox out of the core function, so I've fixed it in a slightly different way; see 879b5c5.

Thanks again for taking a look.

@DJPaul
10 years ago

#21 @DJPaul
10 years ago

Uploaded a new version of the patch; it's up to date with trunk who those who prefer SVN over Git.

#22 in reply to: ↑ 20 @imath
10 years ago

Replying to DJPaul:

This patch -- https://gist.github.com/paulgibbs/9476911 -- is that related to point 2 above?

Yes, you're right it's an existing bug. Just posted #5453 about it.

The multiple select change you spotted is interesting. I made that change on purpose to take the special handling of the multiselectbox out of the core function, so I've fixed it in a slightly different way; see 879b5c5.

Much more elegant :)

Thanks again for taking a look.

yw.

#23 follow-up: @r-a-y
10 years ago

Finally got a chance to test this and like Boone and imath said, there's some really great stuff here! Way easier for devs to add new xprofile types!

I came across a problem with the new 'number' field not being able to accept 0 as a value.

The majority of the problem lies during the saving of the xprofile data and not Paul's code.

To fix this problem:

  • I've altered BP_XProfile_Field_Type_Number so it accepts null values
  • The xprofile screen function was setting empty values as an array. This led to the value being saved as a serialized array instead of integer zero. To fix this, I moved this weird behavior out of the screen function and into xprofile_set_field_data() where the field type is being called. There, I do an explicit field type check for either a checkbox or a multiselectbox, then set the value as an array if it matches these conditions. I did this based on the PHP comments.
  • Finally, in BP_XProfile_ProfileData::save(), I removed the empty() check while leaving the strlen() check. There are probably reasons why an empty check was there in the first place, but this was causing problems with saving integer zero. This can cause other unintended problems, however if we don't like this, we can also change the empty() check to $this->value != ''.

In my mind, we should move most of the logic from xprofile_set_field_data() to BP_XProfile_ProfileData::save() since Paul's BP_XProfile_Field_Type class has a validation routine that is actually useful before saving xprofile field data. We should then remove BP_XProfile_ProfileData::save()'s stringent requirements on updating an existing field and removing an existing field and let Paul's class handle validation exclusively.

Note: The included unit test will pass even without 04b.patch. This is because I can't emulate what happens during the screen function's form submission routine before it gets passed to xprofile_set_field_data().


Some other things I encountered that are not regressions, but are bugs.

  • Date field cannot reset back to nothing
  • There are no inline error messages if validation fails for a specific field. Unfortunately, after we save the fields when editing the profile, we use bp_core_redirect() so we can't use hooks. I tried working out a patch that addressed this, but didn't have enough time to work out the kinks. Either way, this should be placed in another ticket anyway and not this one.

#24 @DJPaul
10 years ago

Thanks Ray. Great to have a third great review and different/valid feedback, too. I'll get to look at this hopefully this week sometime, and get the fixes onto Github once I've dived into those original units. It's a bummer that the unit tests didn't reveal the problem. Perhaps Boone can lend his experience here and help us get a failing test before we fix it.

In my mind, we should move most of the logic from xprofile_set_field_data() to BP_XProfile_ProfileData::save()

We can look into this in the future, or at least after this ticket gets committed/closed. :)

Version 0, edited 10 years ago by DJPaul (next)

#25 @boonebgorges
10 years ago

Note: The included unit test will pass even without 04b.patch. This is because I can't emulate what happens during the screen function's form submission routine before it gets passed to xprofile_set_field_data().

Maybe I'm doing something wrong, but the test is failing for me before applying the other fixes in 04b.patch. Note: you are asserting with a field called 'Age' but the field is being created with the name 'Pens'.

#26 @r-a-y
10 years ago

Note: you are asserting with a field called 'Age' but the field is being created with the name 'Pens'.

Yeah, that's a typo. I've reuploaded 04b.patch.

The test is failing for me before applying the other fixes in 04b.patch

Just retested. You're right! When I posted, I inadvertently tested this unit test before applying Paul's xprofile patch.

@r-a-y
10 years ago

Requires 04.patch - Updated to fix unit test

#27 @shanebp
10 years ago

  • Cc shane@… added

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.


10 years ago

#29 in reply to: ↑ 23 @DJPaul
10 years ago

  • Finally, in BP_XProfile_ProfileData::save(), I removed the empty() check while leaving the strlen() check.

FWIW I tracked this back down to r2547

#30 @DJPaul
10 years ago

Over in githubland, I've found and fixed a bug where you couldn't save negative numbers in the number field type, and have been working through testing the changes from r-a-y's patch. They all look solid so far, and I made a change in the wp-admin xprofile code where I could port across one of the changes, tooa similar change

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.


10 years ago

@DJPaul
10 years ago

#32 @DJPaul
10 years ago

Added 05.patch, which pending final testing, I will be committing later today. It includes a couple more unit tests and more assertions for existing tests. I caught a few more bugs like negative numbers not being accepted by the number field, and some interesting differences between the tests passing integers to the xProfile functions, but the app passing integers as strings. I've put in a quick bodge for that for now, but it points to an area of future improvement (giving the field type class a chance to pre-sanitise the input for a particular field).

Notably I re-implemented Ray's fixed for 0 values as I don't want to set the number class as accepts_null_values. That's already kind of hacky and it's intended specifically for the dropdown box etc, and I don't want it to be used just to be able to store 0 as the field value. :) Recent commits r8163 and r8165 have formed part of the preparation for this patch.

Last edited 10 years ago by DJPaul (previous) (diff)

#33 @djpaul
10 years ago

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

In 8178:

xProfile: re-architecture profile field types and de-duplicate the templating and validation logic to make it easier for core and plugins to add new profile field types.

Until now, it's been pretty hard to add new types of profile field to BuddyPress. There are a couple of plugins that do a good job, but BuddyPress makes it much harder than it should be because, historically, we've hardcoded values and checks in templates throughout the project. For example, profile field templating was duplicated in the registration template, the member/profile/edit template, in parts of the wp-admin xProfile screens, and in the new wp-admin extended profile editor.

This change implements a new approach that creates a class for each profile field type; selectbox, textbox, textarea, and so on. They all share an abstract base class BP_XProfile_Field_Type which consolidates a lot of special behaviour that had been added to BuddyPress over the years (e.g. some fields accept null values, some accept multiple default values), and adds true field value validation. Unit tests are included.

We've also implemented a new "Numbers" field type with these changes. It behaves much the same as a regular textbox field does, but it only accepts numbers.

Fixes #5220 and #4694

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


9 years ago

Note: See TracTickets for help on using tickets.