Nouveau: re-factor bp_nouveau_signup_form() bp field error messages handling

[hnla]

Currently the function renders the base xprofile registration form and handles display of form field errors on username & email by fetching the BP function bp_signup_email_errors rendering via a do_action().

The problem is that this approach spits out plainish text not in keeping with the style and markup for the other template notices, further styling to try and render a similar view is difficult to achieve.

The re-factor does two things:

#1 Removes the do_action and instead fetches the error message from the BP global to test for issset() and then pass the string to a further function to build a display version.

#2 Builds a new helper function to accept a string and wrap it in suitable markup to mirror the other site messages and feedback, this is re-usable for any odd site messages that may fall outside the main template notices array.

This is patched rather than original inclination to merge as there are two concerns with it's implementation to address before accepting it.

• Use of buddypress()->signup->errors[$name] rather than perhaps setting the global function to a var first - is this ok to do

• Adding another function when technically we/I have accepted/announced 'Feature Freeze'. I don't consider this a feature or an enhancement though more a problem not spotted before needing addressing sooner rather than in a later release, but would like agreement on that?

[boonebgorges]

I don't have strong feelings about how the global errors object is accessed. If what you're doing here is in keeping with what happens elsewhere in BP, it seems OK to me.

I trust your judgment on this being a bug rather than a new feature.

A question about the decision itself. I have used the generic bp_{$name}_errors hook in the past to render custom error messages on various registration fields. It looks like this will no longer be necessary, as long as the global error is registered? See ​https://buddypress.trac.wordpress.org/browser/tags/2.9.2/src/bp-members/bp-members-screens.php?marks=198#L183 - for the purposes of Nouveau, this add_action() call will do nothing, right?

[hnla]

A question about the decision itself. I have used the generic bp_{$name}_errors hook in the past to render custom error messages on various registration fields. It looks like this will no longer be necessary

To my mind I see that create_function() as being redundant, seems we're building a series of functions in advance of needing them. For my purposes it seemed commonsensical to back up to buddypress()->signup->errors where I knew I had the required strings, use directly to pass to one function to build out markup around the string.

What I guess I worry about now though is that you mention using the hook to generate custom messages on reg fields, as my approach I think does break this ( guess I could dedicate the new function to the reg fields and add in a apply_filter for access?) If the error string is not in the BP global it can't be used or the bp global string would need changing.

for the purposes of Nouveau, this add_action() call will do nothing, right?

Yes this anonymous function to create those add_actions for errors won't be used - at least not on the reg screen - I ought to check other form fields though, which I haven't done tbh.

[hnla]

Must remember this is only about the signup reg screen and not wander off worrying about possible other instances.

To clarify: Nouveau will loop over buddypress()->signup->errors[$name] so as long as any custom requirements are written to the bp global we'll render them.

If plugins/themes hook to apply_filters('bp_members_signup_error_message', ) then likely I've broken that.

[hnla]

In updating the error message handling it's become apparent that we have another oddity in that we have no acknowledgement of an error via class token for the inputs.

Patch '-2' re-configures the functionality building a class array to loop out user set classes ( N.B it's not clear where users/ devs might set these initial classes from but assuming they can from some point??) to include an 'invalid' class set in the bp global signup error array check and merge in with the class array if it exists.

[hnla]

I considered adding filter capability to the new function but it over complicated things and I couldn't picture the use case for filtering really, I propose to commit these changes tomorrow unless a good arg/example presented that not being able to hook or filter these messages causes problems with existing code.

To recap Nouveau only looks at buddypress()->signup->errors now for the message keys to loop over and display so if they aint in there they won't be displayed.

[DJPaul]

Looks 90% fine, when you commit this, please tweak a few of the spacing:

• Things like errors[$name] should be errors[ $name ]
• Your multi-line comment starting "In case people are adding classes" is so short, you should just keep this on one line.
• ( count( $classes ) > 2 )? ' ' : ''; should be ( count( $classes ) > 2 ) ? ' ' : ''; .
• The if statement in nouveau_error_template() in needs { } those around it.
• The If we only have one token then lets trim the empty space bit seems like a vanity and doesn't add anything of significance to the output.

And when adding a dynamic attribute to something that has some values hardcoded, like so:
class="bp-messages bp-feedback <?php echo esc_attr( $type ); ?>"

It's neater to include all the values in the esc_attr() call:
class="<?php echo esc_attr( 'bp-messages bp-feedback ' . $type ); ?>"

[hnla]

@DJPaul Thanks, formatting adjustments effected. I have had to once again re-visit the class token requirement as the previous iteration had issue running in the foreach loop as the final $classes array was pushing out the 'invalid' class for all elements $classes needed to be emptied at start of new iteration of loop so changed approach significantly ( count() has gone and wasn't liked anyway ).

'-03' is the final approach, if someone could just glance over for obvious formatting etc, I think it tests out ok for all cases I can think of and will commit it later today if time.

[DJPaul]

Since you kind-of asked, I had a second pass at the patch. See -4 and compare it with your -3:

I took out some whitespace changes, some redundant comments, tweaked the $class variable concatenation to make it more readable (it was hard to see we were setting a class= value at a quick glance) and remove a redundant variable, and removed the "todo" in nouveau_error_template() as those are realistically never going to get looked at and are better of in an issue tracker.

[hnla]

@DJPaul Cool, we'll go with your changes, the $class variable concatenation wasn't mine, if that reads better now good stuff.

I don't agree necessarily with removing the comment I added as to why I had stated the empty $class at top of loop, it didn't need to be and shouldn't be perceived falsely as defining the var but as essential to the actual correct processing. If we follow this notion then telling me that 'private' ! == $value is to not display something is stating the bloody obvious and also should be removed :) But hey ho I despair in general at what programmers nowadays consider passes for explanatory comments, we're all mind readers I guess!

Agree with the @todo it won't get done, I've addressed an issue, going further than that at this stage not about to happen. As to tracker or inline it's a debate recently on dev.to iirc and split on inline, meaning it's always locked to function, and line number drifting when referenced from a ticket.

Thanks for the changes @DJPaul I'll commit your patch in a while and close this out.

[hnla]

In 11770:

Nouveau: re-factor bp_nouveau_signup_form error handling

Commit:

1. Re-factors the signup template for new class token rendering & error checking & display of BP signup errors.
2. Adds new function to provide html template markup for error string rendering.

Previously the signup form function rendered the BP global signup->errors via the BP action/do_action, proving hard to style messages in keeping with other site messages.

Commit builds a new function to pass the BP global buddypress()->signup->errors[ $name ] to, rendering the message string in suitable html. This function may be re-used for any additional in page error messages falling outside the Nouveau feedback message array.

Additionally no means of easily identifying the error fields existed, they would display as per browser 'required=required' styling. If BP global errors are set in each loop pass we generate a class 'invalid' to use to style the input on and that matches to the HTML5 pseudo class.

Props hnla, DJPaul, boonebgorges

Fixes #7642