text/number profile fields not rendered if value == 0

[DJPaul]

For a number or text field type, if a user edits their profile and sets the value of that field to "0", the profile can be saved fine, but when viewing that user's profile page, the profile field with value "0" is skipped. I assume there's a bad false check somewhere, and I also assume this was a regression introduced in BP 2.0 (thus the milestone), but I haven't checked yet.

[DJPaul]

This seems to have been a problem for a long time. I tested back to 1.5 (wasn't easy to go further) and was still able to recreate on BP-Default.

[DJPaul]

While it's not a new issue, I'm pretty impressed that we've not seen this issue until now. I bet it goes back to xprofile's original implementation. The tl;dr is that we're being a bit lazy validating if variables have no values by relying on empty().

From php.net:

The following things are considered to be empty:

"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)
$var; (a variable declared, but without a value)

As a result, if an xprofile field's value is set to 0 or "0", we think the value is not set.

I am unsure the best way to patch this, but see 5371.01.patch for one idea; empty() does helpfully check for a lot of cases of values that xprofile would want to see as empty, so I thought that maybe adding an IF to check for 0 or "0" would result in less code change than if we did something like the following, even though it looks kinda janky: if ( $value && $value !== false && $value !== array() && ... )

[r-a-y]

Good catch!

This also works:
​http://stackoverflow.com/a/410200

It's a little bit more simpler.

[DJPaul]

Ooh, that's an appealing looking almost-hack relying on PHP's type crappiness. I can't immediately see any downsides to it, though we'd probably want to leave a comment near where we used that to indicate that we were using != instead of !== on purpose to avoid accidentally breaking this during any future code review.

[djpaul]

In 8554:

xProfile: fix profile fields not rendering if their value is "0".

This was caused by a reliance on the empty() function in a few places, which was causing us to
not handle "0" (both as int and string) as a valid field value. In turn, this was causing the
profile field template loop to skip rendering the affected field.

Fixes #5731, props DJPaul and r-a-y.

[WCUADD]

This is about rendering normal value "0" in xprofile field
but line 89 in bp-xprofile-screens.php (and line 109 in bp-members-screens.php) - trunk version

if ( $is_required[$field_id] && empty( $_POST['field_' . $field_id] ) ) {
   $errors = true;
}

seem to reject normal value "0" entry causing error while registering or updating profile?
If confirmed and not already fixed, perhaps it should be a new ticket?

[WCUADD]

I wanted to make a patch to #5738 (standardise required xprofile fields errors) but this ticket had to be solved first to go further with #5738.

There were 5 places in the code that prevent a required xprofile field to be set to '0' value. See 5731.02.patch.

First workflow (register with backcompat):

• L106:bp-members-screens.php in bp_core_screen_signup(): an empty($_POST['field_' . $field_id]) check prevents to validate the signup screen
• L170:bp-members-screens.php in bp_core_screen_signup(): another empty( $_POST['field_' . $field_id] ) check prevents to store the '0' value in $usermeta (which will be stored in wp_signups table)
• L327:bp-members-classes.phpin add_backcompat(): another empty( $usermeta["field_{$field_id}"] ) check prevents to set '0' value in wp_bp_xprofile_data table

Second workflow (register without backcompat that is to say with define('BP_SIGNUPS_SKIP_USER_CREATION', 1) in wp_config.php):

• L1691:bp-members-functions.php in bp_core_activate_signup(): an empty( $current_field ) check prevents to set '0' value in wp_bp_xprofile_data table

Third workflow (update profile):

• L89:bp-xprofile-screens.php in xprofile_screen_edit_profile(): an empty( $_POST['field_' . $field_id] ) check prevents to validate the edit screen

I ran quite a number of tests and with these modifications within these usecases, I've been able to save '0' value in required xprofile fields.

Perhaps there are other usecases / workflows that still contain this problematic empty() check resulting in misinterpreting '0' value for integer or string required xprofile field?

[boonebgorges]

Thanks for the patch, WCUADD.

We're ending up with a proliferation of type checks in the form submission handlers, which to me is a code smell. What is the purpose of doing empty() checks here in the first place? As I see it, we are just trying to figure out whether there's a value in the $_POST array for us to use (mainly to avoid PHP notices). For that purpose, isset() will work just fine. Type validation and other such business should be left to the validation methods in the field types themselves. Swapping out ! empty() for isset() in these cases will let empty string values through. But (a) for some field types, an empty string is a valid input, and (b) for those where it's *not*, the is_valid() check will fail, which will produce a failed form submit.

This approach seems much more sane to me. What do others think?

[WCUADD]

I omitted to tell that I follow the guidelines proposed by the first fix here in changeset 8554 by only adding AND $x != '0' to the empty($x) check. The empty() problem can be solved with different methods as you pointed. That's the reason why I indicated all the places that currently prevent to set '0' value in required xprofile fields.

Note that this problem can have a lot of consequences when BP is in production. For example, asking for the number of children living in the household (in an intentional communities purpose) as a required field prevent users to register! Two solutions were considered : either to keep off the required caracter of the data (not recommanded in this case) either to modify the core files.

It seems to me that there are three levels of checks for an xprofile field (from the lowest level relevant to data type to the highest level relevant to the data semantics)

• compliance with the type (linked with is_valid() method)
• compliance with the 'required' caracter (determined by the administrator when creating the xprofile field) which is a different check than the compliance with the field type.
• another level which has not be treated: to allow the developper to add his own control / intelligence over the xprofile fields. Of course, BP does not have to deal with all the fields cases as some specific plugins can deal with, but it would be appreciated to give the possibility to the developper to add his own check routines which are therefore very dependant from the data semantics (ie postal codes, telephone numbers, or whatever...). What I'm thinking here is to add a simple hook to allow other types of controls to be added. This has relation with the way errors are rendered which was the subject of #5738.

[DJPaul]

Replying to boonebgorges:

Swapping out ! empty() for isset()

It sounds like it would work, though I haven't read through the proposed changes in all the locations that @WCUADD's helpfully researched.

[boonebgorges]

WCUADD - Thanks for the clarifications. Your patch is definitely in keeping with what's in r8554. What I'm suggesting is that the route taken in r8554 is somewhat crazy :)

I like your "three level of checks" breakdown, but for our purposes here, it's worth noting that each of these should be handled on a field-type basis. That is, all of these checks belong in the BP_XProfile_Field_Type classes, and *not* in the form handlers. So this speaks to the sanity of my suggested approach.

So, long run: the ! empty() checks should be replaced with isset(). However, we have a very small amount of time before 2.1, so I think we need to decide what to do in the short run. Is it feasible to do all the necessary swaps and tests in the next week or so? We would need someone to write the patch. If it's *not* feasible, are the current bugs too severe to ship in 2.1? (It seems to me that they are relative edge cases, but I'd value someone else's thoughts.)

[WCUADD]

I'd like to add a comment even if I'm not sure that it can be valuable for your collective thoughts at this point.

Managing the '0' value case for required xprofile fields is clearly an edge case. Nevertheless, for those who meet this problem, it really seems a bug as it is not easily understandable why the users can't register when fixing a (semantic) valid '0' value in a required xprofile field.

Another side effect is when updating profile with a consequent number of fields in a group fields: the only screen difference between failed or successful profile update is a global message. From support experience, I can tell that users stayed puzzled and didn't understand it, as they took care to enter values in all required fields, and that another substantial proportion of users did even not see the warning (although highlighted with CSS), they thought that it was OK and left the screen and much later on, they realized they lost their data. Support returns were it was very discouraging.

Correcting the bug is nearly impossible without going deep into the core code, leading to the solution to keep off the required caracter of the fields that can semantically take the '0' value, but it is a very simple and easy way to cope with it.

Far away from me to suggest that this problem should be corrected for 2.1, I'm just exposing the user point of vue if it can be helpful in some way. In my humble opinion, it sounds to me much more like something which is important but with low priority.

However, what I'm militantly in favour of is to give the developper the possibility to add its own control routines if needed, what I described as "semantic checks" in comment:12. It's obvious that BP does not have to include all possible checks and plugins can be usefully relied on this. But often, it seems a waste to use a plugin that can deal with a lot of cases when you only need to add a few simple additional checks. And this could be possible with a simple "open door".

When modifying the code to rationalize the field validation process, do you think it could be desirable / possible to add a hook mecanism for this purpose (possibility for a developper to add his own semantics checks) ? Thank you for reading and examining this suggestion. Please forgive me in advance if this is not the right place to mention this possible enhancement, as this ticket has given rise to field validation process in a wider way.

[boonebgorges]

Thanks, WCUADD.

what I'm militantly in favour of is to give the developper the possibility to add its own control routines if needed

This is, in essence, the purpose of the BP_XProfile_Field_Type class that was introduced in BP 2.0. One of the aspects of field types is the validation regex; see eg line 1469 ​https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-xprofile/bp-xprofile-classes.php#L1456. If a plugin developer needs a field with a validation routine that is unlike any of the field types that BP ships with, she can introduce a new field type using BP_XProfile_Field_Type, with her own regex. Moreover, we provide the ability for plugins to filter the is_valid() check on existing profile fields; see line 2703 ​https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-xprofile/bp-xprofile-classes.php#L2672. If you have specific ideas for how the architecture could be more flexible for plugin devs, please do share them.

At the moment, there are bugs in the form-handling code, that are independent of the field type API and are blocking it from being used properly in some cases (since the ! empty() checks are coming before the field_type is even referenced). My proposal to replace these checks with isset() will effectively let any value pass through the form handler, so that the field type API can function as designed.

[WCUADD]

Replying to boonebgorges:
I thank you for spending time to explain me the actual developments. I really appreciate it. It is becoming clearer and of course I don't have any suggestion to make it more flexible than that. It is already much more flexible than all I could imagine :) I had already seen the is_valid method, but it was coming later in the code, I wasn't able to imagine how to use it and above all, how to connect it with the way errors are rendered. This part is still somehow obscure to me, and, if I'm not taking advantage of your great patience, perhaps you could help me to understand what I'm missing. I'm going to post in #5738 because it is much more related to it.

[DJPaul]

Trying to bring things back on trac (har har):

I'm not sure that the comment that caused this ticket to be re-opened is accurate: ​https://buddypress.trac.wordpress.org/ticket/5731#comment:8
I am able to edit my profile and enter a 0 into a field and have it save, without any errors. I didn't test account registration.

I think we need to decide what to do in the short run.

My previous testing revealed this wasn't a regression (at least, since 1.5), and as anything else seems to be inside enhancement territory, let's move this into 2.2.

[DJPaul]

I would appreciate if someone else could test the issue here ​https://buddypress.trac.wordpress.org/ticket/5731#comment:8 and confirm if the behaviour is still broken or not. I had been unable to recreate the problem described.

[hnla]

Just ran a very hasty test on BP beta, single install 2012, adding a new number field and updating in user profile with '0' however saw no issues it saved and displayed without problem.

[DJPaul]

Good enough for me. Thanks Hugo.