Improve error handling for xprofile fields (and non-xprofile fields) during registration and profile edit

[WCUADD]

When a required xprofile field is missing in the register/sign up process, then an error message ("missing field") is returned associated with the specific field thanks to the call to bp_get_the_profile_field_errors_action() in the $field_type->edit_field_html().

This is not the case in the update profile process, only a global error message is shown. Could this be standardize reproducing the required xprofile fields errors management in the sign up process ?

The error is catched during the sign up validation in bp_core_screen_signup() (bp-members-screens.php) and the error caracterised in line 142

add_action( 'bp_' . $fieldname . '_errors', create_function( '', 'echo apply_filters(\'bp_members_signup_error_message\', "<div class=\"error\">" . stripslashes( \'' . addslashes( $error_message ) . '\' ) . "</div>" );' ) );

The same errors process management should occur when updating profile in members/single/profile/edit.php.

The errors in updating profile are catched in xprofile_screen_edit_profile() (bp-xprofile-screens.php) on line 88.

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

but only a global message is emitted on line 96 same file

bp_core_add_message( __( 'Please make sure you fill in all required fields in this profile field group before saving.', 'buddypress' ), 'error' );

There is no error message associated to the particular missing field, as for the sign up process. This could be dealt very easily by modifying the line 88 as follows for example :

if ( xprofile_check_is_required_field( $field_id ) && empty( $_POST['field_' . $field_id] ) && $_POST['field_' . $field_id] !== '0'){
    $errors = true;
    add_action( 'bp_field_'.$field_id.'_errors', 'mandatory_field_error' );
}

where mandatory_field_error() could be a function defined as follows (better with a filter). This function could be factorised with the one in bp_core_screen_signup() (line 142 bp-members-screens.php) which was created only there, but could be used elsewhere as proposed in this ticket.

function mandatory_field_error( ) {
    $message = '<div class="error">'.__( 'This is a required field', 'buddypress' ).'</div>';
    echo (apply_filters( 'mandatory_field_error', $message ))
    return;
}

Please note the correction in the test

if ( xprofile_check_is_required_field( $field_id ) && empty( $_POST['field_' . $field_id] ) && $_POST['field_' . $field_id] !== '0')

relevant to [ticket 5731]​https://buddypress.trac.wordpress.org/ticket/5731#comment:9

[WCUADD]

This ticket relates to this one #5595 where the missing required field error message has been taken into account at the level of the registration process (it was missing echoing the error message).

This ticket deals with catching the same type of errors during the update profile process (instead of registration) as echoing the error message is already planned with the #5595 fix.

[boonebgorges]

Good suggestion. I agree that this should be standardized.

This function could be factorised with the one in bp_core_screen_signup() (line 142 bp-members-screens.php) which was created only there, but could be used elsewhere as proposed in this ticket.

Yes, this is a good idea. There should be a separate function for this so that it can be reused.

If you are willing to have a go at a patch for this, we can consider it for 2.1. See ​http://make.wordpress.org/core/handbook/working-with-trac/submitting-a-patch/ for info on creating a svn-formatted patch. (Directions for BP are similar; use a checkout of ​https://buddypress.svn.wordpress.org/trunk.)

[WCUADD]

Unfornately, I was not able to factorize the function which prints the nature of the field error, because it is a parametrized function (the parameter is the string $error_message). This concerns bp_core_screen_signup() function in bp_members_screens.php.

As far as I know, using an add_action hook only permits to specify the number of arguments of the function that will be applied to the hook (here 'bp_' . $fieldname . '_errors' hook). The arguments themselves are given when executing the hook do_action('bp_' . $fieldname . '_errors') that is to say in the templates register.php (for the non-xprofile fields) and then in the edit_field() function (for the xprofile fields).

It seems to me that it would be more consistent if the two processes (registration and profile update) could operate from an unique basis with advantages for further maintenance, evolution, and flexibility. But it also seems more difficult than I thought at first glance.

Just to be sure of a few things, I tried to point the differences between the two processes (registration and profile update) with regard to required xprofile fields checks and the way these errors are rendered.

The differences concern:

• the fact that it has to be extended to other fields than xprofile fields (in the registration case)
• the fact that the nature of the error is only stored in the registration process (in a global object $bp->signup->errors['fieldname'] that could potentially be accessed further on)
• the use of a function which aims to print the html error message relating to the specific field which is associated to a hook which name is parametrized with the field name 'bp_' . $fieldname . '_errors'. The create_function mecanism inside the hook seems an elegant way to parametrize the function applied to the hook and to give the value of the parameter at the same time (bypassing the regular add_action/do_action way of working)? This will allow to render the error attached to the specific field. I'm not sure that this function was necessary because of the previous point, as the nature of the error is globally stored?
• a local boolean variable $errors vs. a global defined steps variable $bp->signup->step to test if the form validation has succeded
• as a consequence, the fact that profile update process doesn't allow to print a localized error message to a specific field, but only a global error message for the form thanks to the bp_core_message mecanism.
• (the errors are catched for profile update only if the form has been submitted. Why are the required xprofile checks not performed when first editing the profile?)

A few assomptions:

• It seems to me that rendering errors process should be factorized either if it is registration process or profile update process but also whatever type of checks is performed on the xprofile field. As a consequence, the rendering errors process should not only relate to the required xprofile fields checks, but to all types of checks?
• if I correctly understand #5731 discussion, all types of checks should be gathered in the BP_XProfile_Field_Type classes. @boonebgorges, as you stated in ticket:5731#comment:14

  it's worth noting that each of these types of checks 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.
  

and a few consequences:

• Does that mean that required xprofile fields controls should be performed inside the BP_XProfile_Field_Type classes? If so, a BP_XProfile_Field_Type method should check the required character of the field? This is not included for the moment or I did not see it? Going this direction tends to say that the required field check should be like a separate function called by is_valid() check?
• Then, are the tests if ( $is_required[$field_id] && empty( $_POST['field_' . $field_id] ) ) still needed in the two screen functions (even with empty() replaced with !isset())? Perhaps the whole first part of the screen validation should not exist (the part leading to save-details step and the corresponding one in profile update)?
• if all types of checks are performed somehow within the is_valid() method, it would say that they are performed when trying to set the field data in xprofile_set_field_data() which is relatively late to detect errors?

If I all misunderstood and if the purpose is not to gather all types of checks within the BP_XProfile_Field_Type classes, if the purpose is to keep the required xprofile checks within the screen functions, then replacing empty() by !isset() will not be sufficient for the single fields cases as $_POST['field_'.$field_id] is always defined as a string '' (length=0) for them.

I know that I'm missing not something but a lot of things ;) I'd be grateful if someone explains them to me, but I'd truly understand if not possible. I'm aware that it is already an effort to read my english and I apologize for it.

Nevertheless, I finish with developing all this, as I've spent some time trying to understand and to think about it.

So what about the rendering errors process?

• Am I wrong thinking that it should belong the BP_XProfile_Field_Type classes in a similar way as the is_valid() method? As the nature of the error should be catched somewhere within the is_valid() method, it has to be transmitted from this point, either the nature of the error being stored in a global object, either calling the 'bp_' . $fieldname . '_errors' hook directly from here, keeping the create_function mecanism to parametrize the error message?

• If not keeping the create_function mecanism, it amounts to store the nature of the error in a global object in order to give it in the do_action(). But in this case, is there still a need to use the 'bp_' . $fieldname . '_errors' hook? Perhaps because of the register template that can not be changed due to compatibility constraints?

• If each error is not stored in a global object, then how to reconstruct the notion of success or failure at the global form validation level? One failing field validation causes the form validation failing => So it could be with a local variable?

Another detail I don't understand, in the xprofile_screen_edit_profile() screen function, in bp-xprofile-screens.php:L143 :

// Redirect back to the edit screen to display the updates and message
bp_core_redirect( trailingslashit( bp_displayed_user_domain() . $bp->profile->slug . '/edit/group/' . bp_action_variable( 1 ) ) );

This is executed when there is no errors, but it leads to execute twice the screen function, second time with no changes. It seems unnecessary, but there's surely one good reason I wasn't able to find.

Thanks for reading if coming till here.

[boonebgorges]

Thanks for the comments, WCUADD. You've spelled out some of the architectural problems well. I'm changing the name of this ticket to reflect the task more accurately, and I'm going to increase the priority and put it in the 2.2 milestone.

My short response: this set of issues is turning out to be a major annoyance, but to fix it will require a fair amount of refactoring of the registration process as well as some additions to BP_XProfile_Field_Type.

Slightly longer response:

It sounds to me like you are *not* misunderstanding things. The registration system was written largely separately from the xprofile edit system, and the two were pieced together in a quick-and-dirty way to make them work. The introduction of BP_XProfile_Field_Type is making this already existing problem more obvious, because all our nice type-specific is_valid() routines are in stark contrast to the total inability to display field-specific error messages.

Am I wrong thinking that it should belong the BP_XProfile_Field_Type classes in a similar way as the is_valid() method?

I've been thinking the same thing. Field types should be able to register their own custom error messages. In fact, if this is the case, we may be able to do away with some of the global juggling that we're currently doing (since we can create new BP_XProfile_Field_Type objects on the fly).

if all types of checks are performed somehow within the is_valid() method, it would say that they are performed when trying to set the field data in xprofile_set_field_data() which is relatively late to detect errors?

Yes. We don't actually record the xprofile data during registration - we store it in wp_signups. This means that, during registration, we're going to have to run is_valid() manually on the submitted data.

the fact that it has to be extended to other fields than xprofile fields (in the registration case)

Right. Let's design it to work the way we want it for xprofile field types first. Then, we can build wrappers that mimic this architecture for error reporting related to email address, password, etc.

So, let's think big for 2.2. I've got some vague ideas for how we could build out BP_XProfile_Field_Type and use it to do some of the work required here, but WCUADD (and others!), you should feel free to put your ideas here too, either in the form of comments or rough patches.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/