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

[5.7] Support custom user provider names in generator commands #25681

Merged
merged 1 commit into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@markwalet
Contributor

markwalet commented Sep 18, 2018

The generator commands only worked when the auth.providers.user configuration was used. When you change this to something else, the user model wouldn't be replaced correctly.

I refactored the retrieval of the authentication model to a seperate method called userProviderModel(). Then I searched and replaced all occurrences of config('auth.providers.users.model').

I don't know if this is the way you'd like to implement this method. But it fixes the bug. I'll keep the PR open for feedback and edits.

@jmarcher

This comment has been minimized.

Show comment
Hide comment
@jmarcher

jmarcher Sep 18, 2018

Contributor

This could use some tests.

Contributor

jmarcher commented Sep 18, 2018

This could use some tests.

@markwalet

This comment has been minimized.

Show comment
Hide comment
@markwalet

markwalet Sep 18, 2018

Contributor

I know. I searched for some example tests for the commands but I couldn't find any. So I didn't know how you want these tests to look.

Contributor

markwalet commented Sep 18, 2018

I know. I searched for some example tests for the commands but I couldn't find any. So I didn't know how you want these tests to look.

@markwalet

This comment has been minimized.

Show comment
Hide comment
@markwalet

markwalet Sep 19, 2018

Contributor

I can't even run all tests. I get a memory exception. Is there a way to run all tests in chunks or something? Without increasing my memory limit.

Contributor

markwalet commented Sep 19, 2018

I can't even run all tests. I get a memory exception. Is there a way to run all tests in chunks or something? Without increasing my memory limit.

@jmarcher

This comment has been minimized.

Show comment
Hide comment
@jmarcher

jmarcher Sep 19, 2018

Contributor

You can run test per Class, Method, Group, etc. but you still need to run all the tests at least once to make sure your changes do not break other parts of the system.

Contributor

jmarcher commented Sep 19, 2018

You can run test per Class, Method, Group, etc. but you still need to run all the tests at least once to make sure your changes do not break other parts of the system.

@markwalet

This comment has been minimized.

Show comment
Hide comment
@markwalet

markwalet Sep 19, 2018

Contributor

Yeah I don't feel like running through every testclass manually. There should be a way to do this automatically.

Besides that, I know that this change will not break anything. Because all tests get executed with CI.

I do want to write some tests for this change, but I need someone who can tell me where to start. Especially because I cannot find any tests for the artisan:make commands.

Contributor

markwalet commented Sep 19, 2018

Yeah I don't feel like running through every testclass manually. There should be a way to do this automatically.

Besides that, I know that this change will not break anything. Because all tests get executed with CI.

I do want to write some tests for this change, but I need someone who can tell me where to start. Especially because I cannot find any tests for the artisan:make commands.

@jmarcher

This comment has been minimized.

Show comment
Hide comment
@jmarcher

jmarcher Sep 19, 2018

Contributor

I meant that your tests YOUR tests per class/method or group.

Contributor

jmarcher commented Sep 19, 2018

I meant that your tests YOUR tests per class/method or group.

@markwalet

This comment has been minimized.

Show comment
Hide comment
@markwalet

markwalet Sep 19, 2018

Contributor

Ah yeah, that's possible.

Do you know why there aren't any existing tests for the commands?

Contributor

markwalet commented Sep 19, 2018

Ah yeah, that's possible.

Do you know why there aren't any existing tests for the commands?

@GrahamCampbell GrahamCampbell changed the title from Support custom user provider names in generator commands to [5.7] Support custom user provider names in generator commands Sep 19, 2018

@markwalet

This comment has been minimized.

Show comment
Hide comment
@markwalet

markwalet Oct 3, 2018

Contributor

Anyone who can help me?

Contributor

markwalet commented Oct 3, 2018

Anyone who can help me?

@taylorotwell taylorotwell merged commit 3bb7059 into laravel:5.7 Oct 4, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment