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

[1.x] Use named routes on Inertia with Ziggy #314

Merged
merged 1 commit into from
Oct 6, 2020
Merged

[1.x] Use named routes on Inertia with Ziggy #314

merged 1 commit into from
Oct 6, 2020

Conversation

m1guelpf
Copy link
Contributor

@m1guelpf m1guelpf commented Oct 6, 2020

Description

This PR replaces all URLs on the Inertia skeleton with named routes, making use of the Ziggy package from Tighten.

Related Issues

Closes #157

Future Improvements

I wasn't able to replace the URLs on the two-factor authentication component since their routes aren't named. If requested, I could submit a PR to fortify that adds names for these routes for completion's sake.

@driesvints driesvints changed the title Use named routes on Inertia with Ziggy [1.x] Use named routes on Inertia with Ziggy Oct 6, 2020
@driesvints
Copy link
Member

Think we'll need to target 2.x here since ziggy is now basically required and existing installs won't have the dependency.

@taylorotwell
Copy link
Member

@m1guelpf I don't think it matters since this only really affects the installation process and stubs. Existing apps won't have it but new apps will.

@taylorotwell taylorotwell merged commit c99de21 into laravel:1.x Oct 6, 2020
@driesvints
Copy link
Member

@taylorotwell correct, didn't saw that only the stubs were updated 👍

@taylorotwell
Copy link
Member

taylorotwell commented Oct 6, 2020

@m1guelpf after merging this and testing locally I get 419 CSRF problems on every POST.

Yeah, confirmed twice. I use most recently tagged laravel//jetstream + Inertia and everything is fine. I use Jetstream edge after this PR is merged and I get CSRF problems on POST (for example password confirmation or even logout).

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Oct 6, 2020

I don't get how this should affect anything, the result should be exactly the same

@m1guelpf m1guelpf deleted the route-names branch October 6, 2020 15:21
@taylorotwell
Copy link
Member

@m1guelpf you don't have this problem?

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Oct 6, 2020

I didn't, but I also didn't fully test all endpoints (just made sure no pages throwed errors and tried a few), so it's possible I didn't trigger any POST ones. The interesting thing is, if you dd on the tokensMatch function of the VerifyCsrfToken middleware before the check, both tokens match, but if you let the check happen, they fail.

@taylorotwell
Copy link
Member

So you are able to reproduce what I'm talking about now?

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Oct 6, 2020

Found the issue, submitting another PR

Screenshot 2020-10-06 at 4 47 17 PM

@m1guelpf m1guelpf mentioned this pull request Oct 6, 2020
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.

Use routes on Inertia views
3 participants