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

Include "host" in @sessionToken output #194

Merged

Conversation

michaellehmkuhl
Copy link
Contributor

Since the URL::tokenRoute() helper now includes the host parameter, it seems useful to include the same in the @sessionToken blade directive output for forms.

@Kyon147
Copy link
Owner

Kyon147 commented Sep 8, 2023

@michaellehmkuhl could you just take a look at this for PHP 8.0 failing?

@michaellehmkuhl
Copy link
Contributor Author

@Kyon147 it looks like it's the phpstan static code analysis step that's failing. That check seems to be disabled in the other package tests. It's only failing in this case because phpstan isn't able to resolve the Request class through Laravel magic. If you're OK with just adjusting your PHP 8.0 / Laravel 9.39.0 test to match the other tests (by disabling the phpstan check), then I think you'll see it passing.

@michaellehmkuhl
Copy link
Contributor Author

michaellehmkuhl commented Sep 8, 2023

@Kyon147 it crossed my mind that we could also probably get over the phpstan hurdle by making the class path explicit with a use Illuminate\Support\Facades\Request; so I went ahead and updated the code to reflect that approach.

Unfortunately, it doesn't look like that approach made phpstan happy, either.

@michaellehmkuhl
Copy link
Contributor Author

@Kyon147 OK. Switched to the request() helper instead, and phpstan seems ok with that approach. All checks have passed! ✅

@Kyon147 Kyon147 merged commit a3ba3fb into Kyon147:master Sep 11, 2023
4 checks passed
@Kyon147
Copy link
Owner

Kyon147 commented Sep 11, 2023

@michaellehmkuhl thanks for taking the time to look at this and get the CI/CD happy. I've merged this in now, so will look at pushing out a patch release this week as it should be helpful to people using Blade.

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.

2 participants