Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cannot use id param for query strings #206

Closed
dbpolito opened this issue Apr 1, 2019 · 5 comments · Fixed by #263 or #301
Closed

Cannot use id param for query strings #206

dbpolito opened this issue Apr 1, 2019 · 5 comments · Fixed by #263 or #301
Assignees
Milestone

Comments

@dbpolito
Copy link

dbpolito commented Apr 1, 2019

I got a route with no param declared, and i'm using like this:

route('something', {type: 'a', id: 1});
  • Current: /something?0=1
  • Expected: something?type=a&id=1

Probably introduced by #143

@jakebathman
Copy link
Collaborator

Can anyone reproduce this? I'll give it a try this Friday if not, just to see if it's still a bug and if it is we'll get a fix in.

@JameL83
Copy link

JameL83 commented Oct 1, 2019

I can reproduce this and i think its this function normalizeParams in routes.js, correct me if i am wrong.

normalizeParams(params) {
if (typeof params === 'undefined')
        return {};

    // If you passed in a string or integer, wrap it in an array
    params = typeof params !== 'object' ? [params] : params;

    // If the tags object contains an ID and there isn't an ID param in the
    // url template, they probably passed in a single model object and we should
    // wrap this in an array. This could be slightly dangerous and I want to find
    // a better solution for this rare case.

    if (params.hasOwnProperty('id') && this.template.indexOf('{id}') == -1) {
        params = [params.id];
    }

    this.numericParamIndices = Array.isArray(params);
    return Object.assign({}, params);
}

@JameL83
Copy link

JameL83 commented Nov 27, 2019

Any progress on this?

Livijn added a commit to Livijn/ziggy that referenced this issue Dec 4, 2019
A model usually has more than 2 properties. So we check that when passing down a single model to the `route()` param parameter. I would prefer removing this check altogether since it introduces more problems than it solves. 

However, this update allows for use cases like this: `route('report', { type: 'post', id: 123 })`. Fixes tighten#206
This was referenced Dec 4, 2019
@KuNman
Copy link

KuNman commented Mar 25, 2020

I confirm this behavior.

Debug result of route(url, {page: 1, id: 2}).
Get params are broken.

@jakebathman jakebathman added in progress We're working on it and removed hacktoberfest help wanted needs more info Waiting for more information labels Apr 24, 2020
@jakebathman jakebathman added this to the v1.0 milestone Apr 24, 2020
@bakerkretzmar bakerkretzmar self-assigned this Apr 24, 2020
@hydrogennz
Copy link

hydrogennz commented May 25, 2020

@KuNman did the route you were building this for have an optional id param by chance?

i.e. Route::get("/check/{id?}")->name("check")

The issue I think is the check in normalizeParams needs to also check for optional ID params.

e.g.:

        if (
            params.hasOwnProperty('id') && this.template.indexOf('{id}') == -1
        ) {
            params = [params.id];
        }

needs to be:

        if (
            params.hasOwnProperty('id') &&
            (this.template.indexOf('{id}') == -1 && this.template.indexOf('{id?}') == -1)
        ) {
            params = [params.id];
        }

Edit:

Just realised that #263 actually solves the problem as above. This definitely seems to be the issue only thing now is to merge it in and mark this as resolved.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
6 participants