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

Fix behaviour of parameter objects with an 'id' key #301

Merged
merged 4 commits into from Jun 26, 2020

Conversation

bakerkretzmar
Copy link
Collaborator

In #143, which was released somewhere between Ziggy versions 0.6.3 and 0.6.7, a new feature was added to detect when a 'model' object was passed to Ziggy as a route parameter, and use its id property to fill named params if possible.

This allowed us to do the following:

// If our route looks like this...
Route::get('events/{event}', EventController::class)->name('events');
// And we have an object like this, passed down from a controller
// or retrieved via AJAX...
let event = {
    id: 4,
    name: 'Lollapalooza',
};

// ...we can pass it straight in and Ziggy will use the `id` key, even
// though the parameter name is actually `event`
route('events', event);

This is super cool, but there was a bug in its implementation that meant that any route parameter object with an id key passed to Ziggy had that key stripped out, or was entirely re-parsed into an array of ordered, unnamed parameters instead. Notably, this made it impossible (or at least very error-prone) to intentionally pass id as a query parameter.

This PR re-implements that feature a little more carefully—rather than only checking for the presence of an id key, we also now check for the absence of a key matching the route parameter name. I also moved this check to a later point in the process of parsing and replacing the parameters, so it now functions more as a fallback if none of the parameters in question are set explicitly (this also avoids issues when there is actually a named route parameter called id).

Looking again at the route above:

// Because the `event` param is set explicitly, `id` is just added to the query string
// Returns '/events/2?id=9
route('events', { event: 2, id: 9 });

// No `event` parameter passed, automatically uses `id` instead
// Returns '/events/9?location=Times Square'
route('events', { id: 9, location: 'Times Square' });

Fixes #192 and fixes #206.

@bakerkretzmar bakerkretzmar added this to the v1.0 milestone Jun 12, 2020
@bakerkretzmar bakerkretzmar changed the base branch from master to develop June 26, 2020 13:34
@bakerkretzmar bakerkretzmar merged commit 86c7f00 into develop Jun 26, 2020
@bakerkretzmar bakerkretzmar deleted the jbk/fix-model-ids branch June 26, 2020 13:57
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use id param for query strings Parameter behavior changed
1 participant