Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add #validators option, with validation result boolean in #valid #35

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bendoh
Copy link
Contributor

@bendoh bendoh commented Mar 18, 2016

Summary

Adds support for '#validator' option, which can be a callable, a regular expression, or an array of both. Sets #valid boolean flag on all elements, which becomes false when the #validator expression fails. The element value is not propagated when #valid is false. Addresses the question in #34.

Risk

  • Trivial
  • Low
  • Medium
  • High

How to Test

... We really need to put together tests for this module!

@balbuf balbuf self-assigned this Mar 18, 2016

// Validate element value and place result in #valid
if( !empty( $element['#validator'] ) ) {
$validators = (array) $element['#validator'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So [ 'class or object', 'method' ] is a valid callable, and since it is already an array, (array) will not change it and thus the iteration will be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's a good point. I'll update it to check callability first before iterating.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably just do:

if( is_callable( $element['#validator'] ) ) {
    $validators = [ $validators ];
}

@balbuf
Copy link
Contributor

balbuf commented Mar 18, 2016

Lookin fine AF! However, I'm concerned that this doesn't provide an easy way to provide feedback for invalid fields. Maybe the form renderer should add an 'invalid' class to any element containers that are ['#invalid'] === true. Also, I think there should be a quick way to reference if a form has any invalid elements, e.g. add a property to the form array that tells how many invalid elements there are, so you can quickly decide if you want to process the form or display it again.

Another thought: what about error messaging? It would be nice to have some way to provide a corresponding message for each validation function/regex. Maybe the user can handle this on their own by having an array of booleans that correspond to the value of each of their tests for that element. Make sense?

@bendoh
Copy link
Contributor Author

bendoh commented Mar 18, 2016

Yeah, I just wanted to get the core aspect in before worrying about the front-end. The validators, when regular expressions, can be surfaced to the front-end for application in JavaScript, as well. However, when they are PHP functions used for validation, I'm not sure what the right approach is – call back via AJAX to check validity using that function, or just assume that those are only intended to be processed when the form is submitted?

@balbuf
Copy link
Contributor

balbuf commented Mar 18, 2016

I think an ajax callback makes sense! It would be pretty snappy and it wouldn't get called that many times if it was onchange.

@balbuf
Copy link
Contributor

balbuf commented Mar 18, 2016

Same for regex - since PHP/JS regex is not 1-to-1.

@balbuf
Copy link
Contributor

balbuf commented Mar 18, 2016

But maybe you can disable the ajax validation on a per-form basis. I think if you wanted to focus on the backend validation only for this PR then that's cool, but I didn't want the front end consideration to be forgotten about!

@bendoh
Copy link
Contributor Author

bendoh commented Mar 18, 2016

Added some more features to this with some exposure in the front-end.

Add comment about how to express validators and check validity in the
front-end.

Add .wp-form-element-invalid class to invalid elements when rendering.

Increment $form['#invalid_count'] for each invalid element found in
processing.
@@ -713,6 +737,11 @@ static function render_element( $element, &$values, $form = null ) {
$markup .= $element['#label_position'] == 'after' ? $label : '';
}

// Add validation message if element value is not valid
if( !$element['#valid'] ) && $element['#invalid_message'] ) {
$markup .= self::make_tag( 'span', array( 'class' => 'wp-form-element-invalid-message' ), $element['#invalid_message'] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See I think you should be able to have a different message for each validation check so you can give targeted feedback about exactly what is wrong with the value. Also I think there should be an option for the form to either default back to the previously saved value or default (if there is one); OR show the incorrect value that the user tried to submit. Because I think the latter is more friendly for the user, but it currently employs the former.

Ben Doherty added 4 commits March 18, 2016 19:05
* unit-tests:
  Add test harness, suite and tools
  Add initial test harness and test case
  Add PHPUnit composer dependency
* master:
  Put build status in title
  D'oh, missed a spot!
  Make test file 5.3 compatible, too!
  Make 5.3-compatible!
  PHP-5.4-ism "[]" -> PHP 5.3 "array()"
  Add build status badge to README.md
@balbuf
Copy link
Contributor

balbuf commented Apr 11, 2016

All checks have failed!

@marcaddeo
Copy link

@balbuf can we close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants