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

Update to allow custom User classpath and also correctly implement Passport 'auth:api' guard #4

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

b8ne
Copy link

@b8ne b8ne commented Jul 2, 2020

As the title suggests and as mentioned in #3
Note that although this is working for my case, given no tests exist it may fail on edge cases so the main NOTE in README.md has been left.

@b8ne
Copy link
Author

b8ne commented Mar 22, 2021

@LukeTowers I've been using this in production since this PR with no problems. Let me know if it needs anything extra in terms of testing or migration. I think it definitely adds to the usability of the platform and is a great point of difference for Winter to be developer-friendly.

@LukeTowers
Copy link
Member

Have you tested it with v1.1 (ie Laravel 6)?

span: auto
required: 1
type: text
comment: "i.e. Backend\\Models\\User. Default: LukeTowers\\Passport\\Models\\BackendUser"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@b8ne if you're still interested in getting this merged in as a Winter plugin, then it would probably be better to handle this configuration in a config file rather than a DB based settings item. If not, just let me know and I'll make the necessary tweaks and transfer it over to the winter namespace.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually is in a config file, see line 77 of config/config.php. I just use the config, but thought other people may find the GUI way easier. If we scrap the settings and just use the config, then the entire models/Settings.php implementation can also be removd.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested it with v1.1 (ie Laravel 6)?

Just updated dependencies and tested on Laravel 6.

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