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

`jQuery.param`’s behaviour differs with the one described in the docs #3023

Closed
sowcow opened this Issue Mar 29, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@sowcow
Contributor

sowcow commented Mar 29, 2016

docs

http://api.jquery.com/jQuery.param/

As of jQuery 1.8, the $.param() method no longer uses jQuery.ajaxSettings.traditional as its default setting and will default to false. For best compatibility across versions, call $.param() with an explicit value for the second argument and do not use defaults.

actual

https://github.com/jquery/jquery/blob/master/src/serialize.js#L65

    // Set traditional to true for jQuery <= 1.3.2 behavior.
    if ( traditional === undefined ) {
        traditional = jQuery.ajaxSettings && jQuery.ajaxSettings.traditional;
    }
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 29, 2016

Member

Nice catch, @sowcow! Would you like to submit a PR?

Member

gibson042 commented Mar 29, 2016

Nice catch, @sowcow! Would you like to submit a PR?

@sowcow

This comment has been minimized.

Show comment
Hide comment
@sowcow

sowcow Mar 30, 2016

Contributor

Yes, I'm looking forward to it.

Contributor

sowcow commented Mar 30, 2016

Yes, I'm looking forward to it.

sowcow pushed a commit to sowcow/jquery that referenced this issue Mar 31, 2016

@sowcow

This comment has been minimized.

Show comment
Hide comment
@sowcow

sowcow Mar 31, 2016

Contributor

Probably I should also modify ajax.js, i will check that tomorrow.

Contributor

sowcow commented Mar 31, 2016

Probably I should also modify ajax.js, i will check that tomorrow.

sowcow added a commit to sowcow/jquery that referenced this issue Apr 1, 2016

@sowcow

This comment has been minimized.

Show comment
Hide comment
@sowcow

sowcow Apr 3, 2016

Contributor

I think i'm done, comment if any fixes are needed.

Contributor

sowcow commented Apr 3, 2016

I think i'm done, comment if any fixes are needed.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 14, 2016

Member

Everyone agrees this is ready to land? If so I'll land it for master/3.0, update the 3.0 upgrade guide, and submit a docs ticket to change the version to 3.0. Ugh, I guess we'll also need a Migrate ticket.

Member

dmethvin commented Apr 14, 2016

Everyone agrees this is ready to land? If so I'll land it for master/3.0, update the 3.0 upgrade guide, and submit a docs ticket to change the version to 3.0. Ugh, I guess we'll also need a Migrate ticket.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil
Member

timmywil commented Apr 14, 2016

@dmethvin 👍

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 14, 2016

Member

One thing I just realized, the tests here were covering the case for jQuery.ajax() where it uses traditional. We should add a test or two in test/unit/ajax.js that asserts setting traditional will encode the parameters properly.

@sowcow would you be able to do that? You can use this test as a template because it does something very similar. Just pass in traditional: true as one of the ajax parameters and ensure it encodes correctly, you can swipe the encoding cases from the existing tests here.

Member

dmethvin commented Apr 14, 2016

One thing I just realized, the tests here were covering the case for jQuery.ajax() where it uses traditional. We should add a test or two in test/unit/ajax.js that asserts setting traditional will encode the parameters properly.

@sowcow would you be able to do that? You can use this test as a template because it does something very similar. Just pass in traditional: true as one of the ajax parameters and ensure it encodes correctly, you can swipe the encoding cases from the existing tests here.

@sowcow

This comment has been minimized.

Show comment
Hide comment
@sowcow

sowcow Apr 18, 2016

Contributor

I'll do that tomorrow.

Contributor

sowcow commented Apr 18, 2016

I'll do that tomorrow.

@mgol mgol added the Serialize label Apr 25, 2016

@mgol mgol added this to the 3.0.0 milestone Apr 25, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 25, 2016

Member

For reference, PR is in #3030.

Member

mgol commented Apr 25, 2016

For reference, PR is in #3030.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 25, 2016

Member

sorry @sowcow should have pinged you here since the action item was here.

Member

dmethvin commented Apr 25, 2016

sorry @sowcow should have pinged you here since the action item was here.

@sowcow

This comment has been minimized.

Show comment
Hide comment
@sowcow

sowcow Apr 26, 2016

Contributor

sorry, I'm a bit irrational about tests, so i can't do that ⛄️

Contributor

sowcow commented Apr 26, 2016

sorry, I'm a bit irrational about tests, so i can't do that ⛄️

dmethvin added a commit to dmethvin/jquery that referenced this issue Apr 26, 2016

dmethvin added a commit to dmethvin/jquery that referenced this issue Apr 26, 2016

Ajax: Ensure ajaxSettings.traditional is still honored
Fixes gh-3023

Since .param() no longer looks at this setting we need unit tests
to ensure it is still honored by $.ajax().

@dmethvin dmethvin closed this in df2051c Apr 27, 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.