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

Change the visibility of the getter method from private to public #121

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

irieznykov
Copy link
Contributor

Changes

I have changed the visibility of the getter method from private to public. I think this makes a sense because the $auth0 variable is private and its getter is private so there is no ability to get auth0 SDK using Auth0Service. In my case, I need access to the getRefreshToken() method of the SDK because this method was not realized in the service but I need to use the same instance of the SDK which used in methods getUser(), getIdToken().

@joshcanhelp
Copy link
Contributor

@irieznykov - Thank you for the PR and thoughtful description here!

I understand the use case here and we're re-thinking the architecture in this module currently but I wonder if it would be better, at this point, to just add the method that you need instead of changing the visibility of this accessor. If you need the refresh token, you can add this method instead:

class Auth0Service
{
    // ...
    public function getRefreshToken()
    {
        return $this->getSDK()->getRefreshToken();
    }
    // ..
}

@irieznykov
Copy link
Contributor Author

irieznykov commented Mar 1, 2019

Thank you for the replying @joshcanhelp
Agree with you. But, anyway, on my mind, there is no sense to have the private variable and the private getter for it. So, maybe, you can change the visibility to protected at least? This will allow someones to have the ability to interact with the same SDK instance in the child class from the Auth0Service. Please let me know if I can do something with the changes and create the PR for you otherwise I'm waiting for the changes from you.

@joshcanhelp
Copy link
Contributor

@irieznykov - In my mind, the idea here is not to expose the entire SDK but just parts of it you might need, hence the specific accessor methods. I think making the whole thing public does more harm than good as the rest of the functionality there is not needed in this context.

If you feel strongly, I'll spend some time reviewing the implications here. Otherwise, if you switch to what I'm suggesting above, I can get this merged in.

@irieznykov
Copy link
Contributor Author

Hey @joshcanhelp I've changed back the visibility for the getter and have added the new method getRefreshToken() inside service, could you please take a look?

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Clean and simple, works for me!

@joshcanhelp joshcanhelp merged commit dd8c2d2 into auth0:master Mar 7, 2019
@joshcanhelp joshcanhelp added this to the v5-Next milestone Mar 7, 2019
@joshcanhelp joshcanhelp changed the title change the visibility of the getter method from private to public Change the visibility of the getter method from private to public Jun 27, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants