Skip to content

Core: move message processing from formatAndAdd to defaultMessage #1644

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Core: move message processing from formatAndAdd to defaultMessage #1644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ $.validator.format = function( source, params ) {
return $.validator.format.apply( this, args );
};
}
if ( params === undefined ) {
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it on purpose because of the line 1013 in this PR.

Should I omit it and change the line 1013 to the following:

message: this.defaultMessage( element, { method: "remote", parameters: "" } )

return source;
}
if ( arguments.length > 2 && params.constructor !== Array ) {
params = $.makeArray( arguments ).slice( 1 );
}
Expand Down Expand Up @@ -739,26 +742,29 @@ $.extend( $.validator, {
return undefined;
},

defaultMessage: function( element, method ) {
return this.findDefined(
this.customMessage( element.name, method ),
this.customDataMessage( element, method ),

// 'title' is never undefined, so handle empty string as undefined
!this.settings.ignoreTitle && element.title || undefined,
$.validator.messages[ method ],
"<strong>Warning: No message defined for " + element.name + "</strong>"
);
},
defaultMessage: function( element, rule ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a BC Break. Previusly the second argument was an string and now its an object.

Copy link
Contributor

@Maks3w Maks3w Jun 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

if (typeof rule === 'string') {
  rule = {method: rule}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right!
But just a note, undocumented methods are, in the first place, used internally, and only a bunch o methods are intended to be used elsewhere:

The validate method returns a Validator object that has a few public methods that you can use to trigger validation programmatically or change the contents of the form. The validator object has more methods, but only those documented here are intended for usage.

  • Validator.form() – Validates the form.
  • Validator.element() – Validates a single element.
  • Validator.resetForm() – Resets the controlled form.
  • Validator.showErrors() – Show the specified messages.
  • Validator.numberOfInvalids() – Returns the number of invalid fields.

There are a few static methods on the validator object:

  • jQuery.validator.addMethod() – Add a custom validation method.
  • jQuery.validator.format() – Replaces {n} placeholders with arguments.
  • jQuery.validator.setDefaults() – Modify default settings for validation.
  • jQuery.validator.addClassRules() – Add a compound class method.

...

But I can fix that in a followup proposal to maintain backward compatibility with code that still relaying on the old behavior.

Thanks for the note!

Copy link
Contributor

Choose a reason for hiding this comment

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

How can we write custom validation rules then?

Copy link
Member

Choose a reason for hiding this comment

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

Like any of the additional methos are written
https://github.com/jzaefferer/jquery-validation/tree/master/src/additional

Copy link
Contributor

Choose a reason for hiding this comment

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

None of them use a remote validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a PR soon with the following patch:

 src/core.js  | 8 +++++++-
 test/test.js | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/core.js b/src/core.js
index 743f028..b497c4f 100644
--- a/src/core.js
+++ b/src/core.js
@@ -803,6 +803,10 @@ $.extend( $.validator, {
        },

        defaultMessage: function( element, rule ) {
+           if ( typeof rule === "string" ) {
+               rule = { method: rule };
+           }
+
            var message = this.findDefined(
                    this.customMessage( element.name, rule.method ),
                    this.customDataMessage( element, rule.method ),
@@ -1067,10 +1071,12 @@ $.extend( $.validator, {
        },

        previousValue: function( element, method ) {
+           method = typeof method === "string" && method || "remote";
+
            return $.data( element, "previousValue" ) || $.data( element, "previousValue", {
                old: null,
                valid: true,
-               message: this.defaultMessage( element, { method: method } )
+               message: this.defaultMessage( element, method )
            } );
        },

diff --git a/test/test.js b/test/test.js
index 12541c2..05ec156 100644
--- a/test/test.js
+++ b/test/test.js
@@ -678,7 +678,7 @@ test( "option: errorClass with multiple classes", function() {

 test( "defaultMessage(), empty title is ignored", function() {
    var v = $( "#userForm" ).validate();
-   equal( v.defaultMessage( $( "#username" )[ 0 ], { method: "required", parameters: true } ), "This field is required." );
+   equal( v.defaultMessage( $( "#username" )[ 0 ], "required" ), "This field is required." );
 } );

 test( "#741: move message processing from formatAndAdd to defaultMessage", function() {

If there are other things I should take care off, please mention them and I will update before sending the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why change message: this.defaultMessage( element, { method: method } )

May add some kind of deprecation comment about the old parameter type so it could be removed on next major version

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense.
I'll do the update soon.
Thanks!

var message = this.findDefined(
this.customMessage( element.name, rule.method ),
this.customDataMessage( element, rule.method ),

formatAndAdd: function( element, rule ) {
var message = this.defaultMessage( element, rule.method ),
// 'title' is never undefined, so handle empty string as undefined
!this.settings.ignoreTitle && element.title || undefined,
$.validator.messages[ rule.method ],
"<strong>Warning: No message defined for " + element.name + "</strong>"
),
theregex = /\$?\{(\d+)\}/g;
if ( typeof message === "function" ) {
message = message.call( this, rule.parameters, element );
} else if ( theregex.test( message ) ) {
message = $.validator.format( message.replace( theregex, "{$1}" ), rule.parameters );
}

return message;
},

formatAndAdd: function( element, rule ) {
var message = this.defaultMessage( element, rule );

this.errorList.push( {
message: message,
element: element,
Expand Down Expand Up @@ -1004,7 +1010,7 @@ $.extend( $.validator, {
return $.data( element, "previousValue" ) || $.data( element, "previousValue", {
old: null,
valid: true,
message: this.defaultMessage( element, "remote" )
message: this.defaultMessage( element, { method: "remote" } )
} );
},

Expand Down Expand Up @@ -1393,8 +1399,8 @@ $.extend( $.validator, {
validator.showErrors();
} else {
errors = {};
message = response || validator.defaultMessage( element, "remote" );
errors[ element.name ] = previous.message = $.isFunction( message ) ? message( value ) : message;
message = response || validator.defaultMessage( element, { method: "remote", parameters: value } );
errors[ element.name ] = previous.message = message;
validator.invalid[ element.name ] = true;
validator.showErrors( errors );
}
Expand Down
4 changes: 4 additions & 0 deletions test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,10 @@ <h3></h3>
<label for="testForm21!#$%&'()*+,./:;<=>?@[\]^`{|}~">Input text</label>
<input type="text" name="testForm21!#$%&'()*+,./:;<=>?@[\]^`{|}~" id="testForm21!#$%&'()*+,./:;<=>?@[\]^`{|}~" required minlength="15">
</form>
<form id="testForm22">
<label for="tF22Input">Input text</label>
<input type="text" name="tF22Input" id="tF22Input" required minlength="5" data-msg-minlength="You should enter at least {0} characters.">
</form>
<form id="contenteditableForm">
<div contenteditable id="contenteditableNumberInvalid" data-rule-number="true" name="field1">ABC</div>
<div contenteditable id="contenteditableNumberValid" data-rule-number="true" name="field2">123</div>
Expand Down
12 changes: 11 additions & 1 deletion test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,17 @@ test( "option: errorClass with multiple classes", function() {

test( "defaultMessage(), empty title is ignored", function() {
var v = $( "#userForm" ).validate();
equal( v.defaultMessage( $( "#username" )[ 0 ], "required" ), "This field is required." );
equal( v.defaultMessage( $( "#username" )[ 0 ], { method: "required", parameters: true } ), "This field is required." );
} );

test( "#741: move message processing from formatAndAdd to defaultMessage", function() {
var v = $( "#testForm22" ).validate();
equal( v.defaultMessage( $( "#tF22Input" )[ 0 ], { method: "minlength", parameters: 5 } ),
"You should enter at least 5 characters.", "defaultMessage() now format the messages" );

$( "#tF22Input" ).val( "abc" );
v.form();
equal( v.errorList[ 0 ].message, "You should enter at least 5 characters." );
} );

test( "formatAndAdd", function() {
Expand Down