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

$.param() returns "null" and "undefined" literal if attribute is function #3005

Closed
jtrumbull opened this Issue Mar 16, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@jtrumbull
Contributor

jtrumbull commented Mar 16, 2016

var param = $.param({
  a: function () {},
  b: function () { return null; }
});

Will result in a=undefined&b=null

jsfiddle

The value == null check on Line 61 should happen after the ternary statement, since both value and the result of value() can be null. i.e.

        add = function( key, value ) {

            // If value is a function, invoke it and return its value
            value = jQuery.isFunction( value ) ? value() : value;
            value = value == null ? "" : value;
            s[ s.length ] = encodeURIComponent( key ) + "=" + encodeURIComponent( value );
        };
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 16, 2016

Member

Makes sense to me. Would you like to submit a PR, @jtrumbull?

Member

gibson042 commented Mar 16, 2016

Makes sense to me. Would you like to submit a PR, @jtrumbull?

@jtrumbull

This comment has been minimized.

Show comment
Hide comment
@jtrumbull

jtrumbull Mar 17, 2016

Contributor

@gibson042 Yeah, just wanted to confirm that there wasn't reasoning behind the current setup.

Contributor

jtrumbull commented Mar 17, 2016

@gibson042 Yeah, just wanted to confirm that there wasn't reasoning behind the current setup.

jtrumbull added a commit to jtrumbull/jquery that referenced this issue Mar 17, 2016

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 4, 2016

Member

This is a breaking change so it either needs to land ASAP or wait for 4.0.

Member

dmethvin commented Apr 4, 2016

This is a breaking change so it either needs to land ASAP or wait for 4.0.

@mgol mgol added the Behavior Change label Apr 4, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 4, 2016

Member

The PR looks good to me. Should we just land it?

Member

mgol commented Apr 4, 2016

The PR looks good to me. Should we just land it?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 4, 2016

Member

👍 from me

Member

dmethvin commented Apr 4, 2016

👍 from me

@gibson042 gibson042 closed this in 9fdbdd3 Apr 5, 2016

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 5, 2016

Member

I don't think this is a breaking change, since "the return value of a function is used instead of the function as a String" still holds (http://api.jquery.com/jQuery.param/ ). But please make an issue or PR in https://github.com/jquery/api.jquery.com if you disagree.

Member

gibson042 commented Apr 5, 2016

I don't think this is a breaking change, since "the return value of a function is used instead of the function as a String" still holds (http://api.jquery.com/jQuery.param/ ). But please make an issue or PR in https://github.com/jquery/api.jquery.com if you disagree.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 5, 2016

Member

Hmm, OK. But maybe it's still best to not backport this to 1.12/2.2.

Member

mgol commented Apr 5, 2016

Hmm, OK. But maybe it's still best to not backport this to 1.12/2.2.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 5, 2016

Member

I'm fine with that.

Member

gibson042 commented Apr 5, 2016

I'm fine with that.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 5, 2016

Member

I see your point, let's call this a 3.0 feature/fix.

Member

dmethvin commented Apr 5, 2016

I see your point, let's call this a 3.0 feature/fix.

@gibson042 gibson042 added this to the 3.0.0 milestone Apr 5, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.