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

[5.x] Add getToken() method to User contract #648

Closed

Conversation

josepostiga
Copy link

This PR adds a new getToken() method to the abstract User class that returns the $token property value that both \Laravel\Socialite\One\User and \Laravel\Socialite\Two\User share. I also moved the declaration of the property to the abstract class because it's the same in both subclasses.

This doesn't break backwards compatibility so it should be pretty harmless to integrate in a minor version.

@taylorotwell
Copy link
Member

Hey Jose,

Is this fixing a bug?

@taylorotwell taylorotwell marked this pull request as draft July 28, 2023 13:58
@josepostiga
Copy link
Author

Hi Taylor!

As far as I know, it's not fixing a bug. I'm working on a project where I'm using Laravel Socialite to create user accounts, and as far as the project cares, it doesn't really matter which concrete type of User is used, as both of them will have the property, but it would be nice to not have to depend on a concrete subclass of User to not have my IDE screaming at me because I'm "accessing an undefined property".

😅

@driesvints driesvints marked this pull request as ready for review July 28, 2023 14:19
@taylorotwell
Copy link
Member

I don't think we can technically add the method to the interface on a patch release as it would be a breaking change.

@josepostiga josepostiga deleted the feat/update_user_contract branch July 28, 2023 16:30
@josepostiga
Copy link
Author

Oh, right. Forgot the small detail of anyone that could have other implementations of this contract in their own projects.

But I still think we could benefit from this. Would you be open to have a major version? Maybe taking the opportunity to also do some refactoring by adding types to methods? 😊

If you see any value in that, I could try and help out in a fork and send it over for review? Maybe send over some notes over tweaks we could do besides this one, before anything. If the team likes it, we all win. If not, no harm no fall…

But I’d totally understand if that’s not something you guys would be open to invest on, at this time.

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