[buddypress-trac] [BuddyPress Trac] #5220: Overhaul implementation of xprofile field types

[buddypress-trac]

#5220: Overhaul implementation of xprofile field types
------------------------------------------+-----------------------
 Reporter:  DJPaul                        |       Owner:  DJPaul
     Type:  enhancement                   |      Status:  assigned
 Priority:  normal                        |   Milestone:  2.0
Component:  XProfile                      |     Version:
 Severity:  normal                        |  Resolution:
 Keywords:  has-patch dev-feedback early  |
------------------------------------------+-----------------------

Comment (by boonebgorges):

 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!

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5220#comment:15>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac