Refactor internal configuration and public properties #294
Conversation
Add `getState` for external access to authorization state.
| */ | ||
| protected function getScopeSeparator() | ||
| { | ||
| return ','; |
There was a problem hiding this comment.
I thought about making this a class constant.
Public interface is not modified, but internally headers are now generated by using `getDefaultHeaders` and `getAuthorizationHeaders` methods, the latter only be called when a token is passed into `getHeaders`. This replaces the `$authorizationHeader` property and makes the internal interface more flexible for custom authentication schemes.
Remove `getScopes` and `setScopes` from provider interface, as we can now overload scopes using authorization options. Remove `$scopes` and `$scopeSeparator` properties in favor of `getDefaultScopes` method and `SCOPE_SEPARATOR` constant that can be overloaded by providers.
| */ | ||
| public $state; | ||
| const SCOPE_SEPARATOR = ','; |
There was a problem hiding this comment.
Converted the constant value to... constants.
|
This looks good to me. As long as we keep |
|
All looks great to me. Looks like you said you needed to remove/change some things. Is it all good to merge, or should I wait? |
|
Okay to merge. I made my changes immediately after opening the PR.
|
|
What are your thoughts on making provider configuration immutable? |
|
Actually nevermind I just had a closer look. :) |
Refactor internal configuration and public properties
| */ | ||
| public $redirectUri = ''; | ||
| const RESPONSE_TYPE = 'json'; |
There was a problem hiding this comment.
Having the response type as a constant is problematic for the Facebook provider. In Graph v2.2 and below the response type was "string" and starting in v2.3 the response type was "json". See how the Facebook provider was dealing with this issue. Any way we can revert this change for the response type?
Also - although I like constants, there's no guarantee that any of these constants will be consistent across all versions of a provider's API (as in the response type example). Perhaps we should revert all of them to be on the safe side? :)
|
Make it method and shove it in the interface then? |
So have protected methods return the default, and be overridden where necessary? That's how I've always done things as it has all the benefits of the property not being accessible outside of the class (and its descendants), having a default value present, and being flexible for extending classes. You can then return a response type based on the value of something else, eg. Graph API version. No need to have these in the interface as extending classes would only override if required to do so. |
AbstractProvidermade protected