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

Add path option to publishable config file #1714

Closed
wants to merge 1 commit into from

Conversation

selfsimilar
Copy link

All config('passport.*') values should be represented in the template config file. path was the only one missing.

In context, I have spent a good deal of time today trying to determine how to modify Passport's route prefix, and most web articles are before July 2021 when the PR to refactor routes to dedicated file was accepted. It's not clear to me when the static routes() function was removed, but most online instructions reference using that in the boot() function of AuthServiceProvider.php to modify the route prefix (e.g. https://pownall.dev/post/prefix-laravel-passport-routes).

I am also planning to open a PR to update the official Laravel Passport docs, to clarify this there as well. But having this represented in the publishable config file is, imo, a necessary regardless so all calls to config('passport.*') in Passport's code are visible in the template file.

All `config('passport.*')` values should be represented in the template
config file. `path` was the only one missing.
@driesvints driesvints changed the title Add path option to publishable config file. Add path option to publishable config file Feb 6, 2024
@driesvints
Copy link
Member

I personally don't feel we should add this to the config. /oauth at the root of the path is a good default and general goto when using oauth authentication. Customizing this might indeed be needed in certain situations but those are edge cases at best.

@selfsimilar
Copy link
Author

I agree 100% that /oauth is the correct default and 99% of the time doesn't need to be changed. However, I would note the following:

  • This PR is pretty benign in that it simply exposes what was hidden, but doesn't change any behavior. Perhaps adding to the docblock above the value to say something like "Usually does not need to be adjusted" would be helpful?
  • There is currently nothing in the documentation to indicate that changing the prefix is possible. I've opened a PR to fix that, but it took me some time wading through out-of-date articles before I started combing through the Passport code to determine the current way.
  • Most people will not publish the config file unless they need to customize something, at which point you should trust that they will not simply change values on a whim. To hide controls after publishing the config file feels mistrustful of other developers, or malicious.
  • Finally, a fairly popular package by Joel Butcher called Socialtream (>390 stars) conflicts with the /oauth route. I have updated the Socialstream docs to reflect the new way of updating Passport's prefix route, but suffice it to say that while changing the route is an edge case, it is certainly still encountered.

I hope you will still consider adding this to help save even a handful of developers time while working with this wonderful project.

Cheers

@driesvints
Copy link
Member

Thanks for that @selfsimilar. I guess what we can maybe do is add the config option but not document it. Let's see what @taylorotwell says.

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.

3 participants