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.

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

Conversation

Arkni
Copy link
Member

@Arkni Arkni commented Nov 12, 2015

Fixes #741

@@ -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: "" } )

@staabm staabm closed this in a3cc0c0 Nov 24, 2015
@staabm
Copy link
Member

staabm commented Nov 24, 2015

thanks!

@Arkni Arkni deleted the issue-741 branch November 24, 2015 12:29
"<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

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.

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!

Arkni added a commit to Arkni/jquery-validation that referenced this pull request Jun 11, 2016
Arkni added a commit to Arkni/jquery-validation that referenced this pull request Jun 11, 2016
Arkni added a commit to Arkni/jquery-validation that referenced this pull request Jun 11, 2016
Arkni added a commit to Arkni/jquery-validation that referenced this pull request Jun 14, 2016
Arkni added a commit to Arkni/jquery-validation that referenced this pull request Jun 14, 2016
staabm pushed a commit that referenced this pull request Jul 15, 2016
As of now the signature of `validator.defaultMessage( element, string )`
is deprecated. Use `validator.defaultMessage( element, object )` instead
where objet is defined as
```
object = {
   method: "method name",
   parameters: "the given method parameters"
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants