[buddypress-trac] [BuddyPress Trac] #5738: Required xprofile fields errors management in members/single/profile/edit.php

[buddypress-trac]

#5738: Required xprofile fields errors management in
members/single/profile/edit.php
-------------------------+-----------------------------
 Reporter:  WCUADD       |       Owner:
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  Future Release
Component:  XProfile     |     Version:
 Severity:  normal       |  Resolution:
 Keywords:  needs-patch  |
-------------------------+-----------------------------

Comment (by 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.

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