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 integration with Laravel default auth system and User model #22

Closed
wants to merge 46 commits into from
Closed

Add integration with Laravel default auth system and User model #22

wants to merge 46 commits into from

Conversation

josepostiga
Copy link

@josepostiga josepostiga commented Nov 10, 2018

Hey there! An awesome package, you've got here.

This PR has the main goal of integrating Wink with the base Laravel auth system and use the default User model.

In order to do that, I started by refactoring the migration, which I turned into a Stub that's published when the php artisan wink:install command is run and removed the Wink specific database connection config variable and any reference of it throughout the Wink controllers.

Also, as consequence, I removed almost any controller related to authentication and registration, in favour of using the default ones that Laravel provides with the command php artisan make:auth. I did keep, however, the frontend assets and I'll leave the refactor of those for another PR, for now.

I did remove the WinkAuthor model and extracted the necessary relations methods to an IsAuthor trait. This trait needs to be included on a Model that represents the Users, on a given system. I did, however, add two config vars that let developers decide which model to use (defaulting to the User model Laravel supplies) and the corresponding table name on the database (defaulting, too, to the Laravel's users table).

The rest of the work declared was about updating DockBlocks return statements and updating logic comparison operators to be more strict.

Hope this helps about #13 and many other questions that I've seen throughout Twitter.

josepostiga and others added 19 commits November 10, 2018 02:56
Removes Wink specific authentication controllers;
Add isAuthor trait to relate User model with Wink resources;
Extract migration class to a stub file in favor of usage of vendor publish commands.

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…rators comparison and remove specific wink db connection configuration

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Removes Wink specific authentication controllers;
Add isAuthor trait to relate User model with Wink resources;
Extract migration class to a stub file in favor of usage of vendor publish commands.

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…rators comparison and remove specific wink db connection configuration

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…el-default-auth-system' into improvement/integrate-with-laravel-default-auth-system
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…wink

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
@Lloople
Copy link
Contributor

Lloople commented Nov 10, 2018

👏 What a huge Pull Request!

We were discussing, also, the fact that having the wink related fields in a different table with a 1 to 1 relationship. For example wink_author containing user_id, slug, bio and avatar.

What do you think about that?

There's a chance that a project integrating Wink has already added a lot of fields to users table and they don't want to mix them up.

As I always say, wait until someone else leave comments before doing anything based on what I've said 😅

@josepostiga
Copy link
Author

josepostiga commented Nov 10, 2018

Thank you @Lloople. I've been following the discussion, that's why I referenced it on this PR.

I don't see a real need for adding the complexity of a 1-1 relation between a users table and the additional author fields (slug, bio and avatar). The likelihood of adding those fields directly to the users' table and having a high impact on an application is low, in my opinion.

However, if it needs to be done, for whatever edgy case there might be, you can do some extending of the base User model class, apply the Trait on that new model and updating the corresponding config values to reference the model and the table to use. I'm talking from the top of my head, as I haven't tested this specifically, but I don't see that'd be too hard to adjust.

@Lloople
Copy link
Contributor

Lloople commented Nov 10, 2018

Yeah I get your point, 3 attributes are nothing really. But I think the conversation was more focused on the fact that maybe the current application is already huge, or maybe Wink will require more fields in the future than just 3.

I think it was about keeping things organized, an app can already have a field slug on users table, so you'll need to rename this one to wink_slug for example, that will make the installation process more complicated to the final user of the package.

It's fine for me though, I'll use this package in a clean Laravel app :) But if we wanted to use it in the project I'm working at my job we will need to modify the column names 😅

@josepostiga
Copy link
Author

Yeah, I'm always interested in testing different scenarios but, and I'll use the slug example you gave me here, If it already exists then you can change the migration to not handle that field.

From what I understand, Wink will continue to work as expected since the column exists and you'll be able to edit it from the profile form.

@Lloople
Copy link
Contributor

Lloople commented Nov 10, 2018

Maybe that field is used for some internal things, I don't know it can be anything. Keeping wink fields in a separate table ensures 100% compatibility with the whole system if this package is installed in an existing app.

@DivineOmega
Copy link
Contributor

This is great. I was going to work on something similar to this, based on my comment here, but it looks like you've covered it.

I also definitely agree with the publishing of the migration and removal of the wink:migrate command.

@themsaid
Copy link
Owner

Thank you :) But the problem with this approach is that the user is forced to use the default database connection. There's no way you can host Wink on a different database.

Also you might not want all your users to have access to Wink.

The main goal here is to give the most flexibility, that's why I decided to separate the entities in the first place.

themsaid and others added 22 commits November 11, 2018 21:21
Removes Wink specific authentication controllers;
Add isAuthor trait to relate User model with Wink resources;
Extract migration class to a stub file in favor of usage of vendor publish commands.

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…rators comparison and remove specific wink db connection configuration

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Removes Wink specific authentication controllers;
Add isAuthor trait to relate User model with Wink resources;
Extract migration class to a stub file in favor of usage of vendor publish commands.

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…rators comparison and remove specific wink db connection configuration

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
…wink

Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
Signed-off-by: Jose Postiga <josepostiga@icloud.com>
@themsaid
Copy link
Owner

Going to close this PR for now, really appreciate your efforts but I want it be done in a more flexible way :)

Let's continue the discussion in #13, maybe we come up with a more flexible approach.

@themsaid themsaid closed this Nov 12, 2018
@josepostiga
Copy link
Author

Ok, sure. Thanks to all for taking the time to analyse this PR. 👍

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.

6 participants