Skip to content
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

Allow optional id param #263

Merged
merged 7 commits into from Jun 12, 2020
Merged

Allow optional id param #263

merged 7 commits into from Jun 12, 2020

Conversation

Livijn
Copy link
Contributor

@Livijn Livijn commented Dec 4, 2019

Fixes a bug when using an optional id param, e.g.: report/{type}/{id?}

Fixes #206

@KuNman
Copy link

KuNman commented Mar 29, 2020

@Livijn can you make new composer package with this fix or @DanielCoulbourne can you accept it?

@mattstauffer
Copy link
Member

I can merge this issue if folks say it works well for them. Can folks comment or leave emoji responses if this helps or hurts them?

Daniel is a bit behind on Ziggy because he’s got a lot going on!! but I’ll make sure the Tighten team picks it up going forward.

@KuNman
Copy link

KuNman commented Mar 29, 2020

I found it as fix for mentioned issue #206 , hope it works well cause atm I can't use param named as id which is quite frustrating.

@Livijn
Copy link
Contributor Author

Livijn commented Mar 29, 2020

It works fine for me in 2 different projects and I did add some tests as well in the PR.

@KuNman
Copy link

KuNman commented Mar 29, 2020

Can you share what version of laravel and ziggy you use?
I'm trying to execute route(url, {id: 1}) and it returns Router.urlParams: {0: 1}. When I change key id to p.ex. _id it works - Router.urlParams: {_id: 1}.

@Livijn
Copy link
Contributor Author

Livijn commented Mar 30, 2020

Can you share what version of laravel and ziggy you use?
I'm trying to execute route(url, {id: 1}) and it returns Router.urlParams: {0: 1}. When I change key id to p.ex. _id it works - Router.urlParams: {_id: 1}.

I mean with this PR it works fine. You can require it in your composer.json: "tightenco/ziggy": "dev-master#97f219bf1a0616bd4e6150b557b25617fef7893e"

@KuNman
Copy link

KuNman commented Mar 30, 2020

Hmn, I removed ^0.9.0 and installed dev-master#97f219bf1a0616bd4e6150b557b25617fef7893e, composer.lock says

"name": "tightenco/ziggy",
"version": "dev-master",
"source": {
    "type": "git",
    "url": "https://github.com/tightenco/ziggy.git",
    "reference": "97f219bf1a0616bd4e6150b557b25617fef7893e"
},
"dist": {
    "type": "zip",
    "url": "https://api.github.com/repos/tightenco/ziggy/zipball/97f219bf1a0616bd4e6150b557b25617fef7893e",
    "reference": "97f219bf1a0616bd4e6150b557b25617fef7893e",
    "shasum": ""
},

I regenerated ziggy routes, cleared views cache and it works same as previously = crashed urlParams.
Should I do something else yet?

@Livijn
Copy link
Contributor Author

Livijn commented Apr 1, 2020

That is the correct branch. What does your route look like and how are you referencing it with Ziggy?

@KuNman
Copy link

KuNman commented Apr 7, 2020

In index.blade.php I call @routes in head section then in js I invoke it like that const route = window.route; and use like that window.axios.get(route(url, body));.

@Livijn
Copy link
Contributor Author

Livijn commented Apr 7, 2020

In index.blade.php I call @routes in head section then in js I invoke it like that const route = window.route; and use like that window.axios.get(route(url, body));.

What I meant was how does your route look like and how are you using that route?

@KuNman
Copy link

KuNman commented Apr 7, 2020

image

Routes/api.php
Screenshot 2020-04-07 at 20 34 42

We are talking about routes generated by Laravel's method for API resources.

@Livijn
Copy link
Contributor Author

Livijn commented Apr 10, 2020

I don't see any optional id param.

@bakerkretzmar bakerkretzmar added this to the v1.0 milestone May 8, 2020
@hydrogennz
Copy link

@mattstauffer I have tested this and it resolves the issue for routes with optional {id?} params. Thanks. 😄

@bakerkretzmar bakerkretzmar merged commit 5b75174 into tighten:master Jun 12, 2020
@bakerkretzmar
Copy link
Collaborator

@Livijn thanks for your patience and for this fix. Just want to note here that this actually doesn't fix #206—that issue is a bug in trying to pass a query param called id.

bakerkretzmar added a commit that referenced this pull request Jun 13, 2020
bakerkretzmar added a commit that referenced this pull request Jul 31, 2020
* Formatting

* Clarify and shorten test names

* Fix assertion parameter order – `assert.equal` wants actual first, then expected

* Run tests with AVA instead of Mocha

* Move tests related to checking the current route into their own file

* Add test for regex bux

* Update port test

* Remove unnecessary setup

* Rename test.route.js to route.spec.js

* Extract common setup steps

* Move axios tests into their own file

* Move check tests into their own file

* Wip

* Consolidate setup

* Update PHP testing configuration

* Remove unused methods and imports

* Require phpunit explicitly

* Add test from #263

* Naming and formatting, and splitting more tests out into their own files

* Load test routes from fixture file

* Make assertions stricter

* Make assertions stricter and clean up formatting

* Define routes in setUp again

* Formatting

* Formatting

* Formatting

* Wip

* Finish converting JS tests

* Add tests from #302 for subfolder routing

* Add tests from #301 for parsing the id key out of an object

* Remove arrow functions for compatibility with PHP < 7.4

* Update dependencies

* Clean up repetitive assertions

* Formatting

* Stub out tests for open PRs

* Stub out test for matching current route *with parameters*

* Remove babel Object.assign transform

* Simplify Ava config

* Update deps and format babel config

* Clean up setup a bit

* Clean up PHP test formatting

* Simplify filtering

* Correctly set latest tag during `npm publish` even if it isn't annotated

* Move all tests back into one file

* Remove axios tests

* Use power-assert

* Rename back to route.test.js

* Use Jest

* Update Contributing and test workflow

* Formatting

* Remove build:watch

* Formatting

* Formatting

* Fix globals

* Fix middleware test on Laravel <7

* Update changelog

* PHPUnit and PHP version

* Remove dumps

* Don't test PHP 7.2

* Wip
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.

Cannot use id param for query strings
5 participants