-
Notifications
You must be signed in to change notification settings - Fork 254
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
Conversation
@Livijn can you make new composer package with this fix or @DanielCoulbourne can you accept it? |
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. |
I found it as fix for mentioned issue #206 , hope it works well cause atm I can't use param named as |
It works fine for me in 2 different projects and I did add some tests as well in the PR. |
Can you share what version of laravel and ziggy you use? |
I mean with this PR it works fine. You can require it in your composer.json: |
Hmn, I removed
I regenerated ziggy routes, cleared views cache and it works same as previously = crashed |
That is the correct branch. What does your route look like and how are you referencing it with Ziggy? |
In |
What I meant was how does your route look like and how are you using that route? |
I don't see any optional id param. |
@mattstauffer I have tested this and it resolves the issue for routes with optional {id?} params. Thanks. 😄 |
* 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
Fixes a bug when using an optional id param, e.g.:
report/{type}/{id?}
Fixes #206