Skip to content

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

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

Jump to bottom
Closed
sowcow opened this issue Mar 29, 2016 · 11 comments
Closed

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

sowcow opened this issue Mar 29, 2016 · 11 comments
Assignees
@dmethvin
Labels
Blocker Has Pull Request Serialize
Milestone
3.0.0

Comments

@sowcow
Copy link

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
Copy link
Member

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

@sowcow
Copy link
Author

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
serialize: .param - don't use ajaxSettings.traditional as a default v…
0449df5 
…alue.

Closes jquerygh-3023
@sowcow
Copy link
Author

sowcow commented Mar 31, 2016

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

sowcow pushed a commit to sowcow/jquery that referenced this issue Apr 1, 2016
Serialize: .param - don't use ajaxSettings.traditional
c5c3860 
Closes jquerygh-3023
@sowcow
Copy link
Author

sowcow commented Apr 3, 2016

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

@sowcow sowcow mentioned this issue Apr 4, 2016
Closed
@dmethvin
Copy link
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.

@timmywil
Copy link
Member

@dmethvin 👍

@dmethvin
Copy link
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.

@sowcow
Copy link
Author

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
Copy link
Member

mgol commented Apr 25, 2016

For reference, PR is in #3030.

@dmethvin
Copy link
Member

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

@sowcow
Copy link
Author

sowcow commented Apr 26, 2016

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

dmethvin pushed a commit to dmethvin/jquery that referenced this issue Apr 26, 2016
@dmethvin
Serialize: .param - don't use ajaxSettings.traditional
61d0d1a 
Ref jquerygh-3023
Closes jquerygh-3030
dmethvin added a commit to dmethvin/jquery that referenced this issue Apr 26, 2016
@dmethvin
Ajax: Ensure ajaxSettings.traditional is still honored
2299592 
Fixes jquerygh-3023

Since .param() no longer looks at this setting we need unit tests
to ensure it is still honored by $.ajax().
dmethvin pushed a commit that referenced this issue Apr 27, 2016
@dmethvin
Serialize: .param - don't use ajaxSettings.traditional
4f27042 
Ref gh-3023
Closes gh-3030
@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.
Assignees

@dmethvin dmethvin

Labels
Blocker Has Pull Request Serialize
Milestone
3.0.0
Development

No branches or pull requests
5 participants
@dmethvin @timmywil @sowcow @gibson042 @mgol