oauth2-client

oauth2-client copied to clipboard

The official provider packages are pretty inconsistent on code style and naming conventions.

For example, in the GitHub adapter package, the implementation of ResourceOwnerInterface is referred to as GithubResourceOwner, while in the FaceBook and Google implementations, these are FaceBookUser and GoogleUser.

Providers (in documentation and doc-blocks) are sometimes referred to as "providers", other times as "gateways".

Constructors are inconsistently type-hinted, sometimes as array $response = array(), other times as array $response.

And accessors use completely different code styles - for example, in the GitHub implementation, get-methods use e.g. return $this->response['email'] ?: null which will trigger an E_NOTICE for undefined values, the Google implementation wraps return-statements in if (!empty($this->response['emails'])) (but has inconsistent return points) and the FaceBook implementation uses an internal getField() method which uses isset.

The FaceBook implementation internally stores the array as $data, while the in the Google and GitHub implementations it's $response.

Perhaps these implementations should derive from a common ancestor, so that at least constructors and the internal field would be consistent.

Either way, perhaps you ought to include a basic style guide, or a CodeSniffer or MessDetector profile, or perhaps better, identify a single reference implementation as the "correct" one to follow, e.g. in implementing.md, so that third-party implementations wouldn't automatically end up with all different styles depending on which implementation you used as a template?

I'm not trying to be nit-picky here, it's just that reading documentation and looking at different providers can be very confusing when the terminology (in documentation and source-code) and code style is very inconsistent - and it takes much longer than necessary to absorb this otherwise very complete and functional OAuth framework.

I don't think it's my place to start forking and fixing things, since I can't know which of the various styles or terminology I'm supposed to align with, but do let me know if there's anything I can do to help?

I agree that we should be more consistent with structure and nomenclature. The reason for the disconnect between these libraries is historical more than anything. Most began development before this library reached version 1.0, and the oauth2-client base package didn't reach stable nomenclature/structure until then.

In general, I would argue that the base package is canonical and all other officially-supported provider packages should be updated for consistency. However, I want to make sure the other package developers agree with this direction first.

Additionally, we also have to be concerned with BC. We can't just change FaceBookUser to FacebookResourceOwner, since that might break all applications using the Facebook provider. We'll need to be careful about when and if we roll out these kinds of changes.

Tagging maintainers of official League provider packages for thoughts... @SammyK, @stevenmaguire, @shadowhand

I'm +1 for consistency but only to a degree.

The Facebook provider tries to use a ubiquitous language between the provider and the Facebook platform. For example, the resource owner is called FacebookUser because it represents a Facebook User on the Graph API. And there's a method called getField() because Facebook calls attributes on a node "fields".

I'm OK with changing the provider to match a common set of rules so long as they don't interfere with the ubiquitous language from platform to platform. :)

We chose "ResourceOwner" as the language for the base package since not all descendant packages will be for platforms that deal in "users." Additionally, OAuth 2 uses the language "resource owner" over "user," but again, this is because the resource owner might not be a user. It could be a machine, or a group, etc.

So, that's why the base package goes with a more generic name for the owner of whatever resource you're attempting to access.

I agree with @SammyK that descendent packages should use nomenclature based on the platforms they're interfacing with. OAuth 2 is a framework, not a protocol. So, every platform will be a little bit different in how they function and the way they name things.

If there are places we can be consistent without overriding the way providers name their things or the functionality they provide, then let's work on those areas.

I understand and appreciate the concerns with the inconsistencies, especially with how they fill the space in my head when I want everything to line up and be organized :)

I've encountered this concern before, especially in an OSS project of my own, and in the end I have to wonder what is gained by agreeing to and documenting an explicit rule which can be "violated" so easily? I think it is unrealistic to expect that every provider that is built follows the exact same convention, to wit the use case provided by @SammyK, and if 100% are not in compliance then it seems like an arbitrary requirement; you will still need to write custom code per provider.

I think a more reasonable strategy is to identify the specific packages that you feel are "out of line", refactor them, create a pull request, and state your case. I have a feeling, especially when it comes to the dozen or so packages that I maintain (which were largely composed of historical packages @ramsey mentioned), that your PR would be accepted and you will make the improvement you are seeking through action rather than bureaucracy.

I don't support a blanket set of requirements which dictate the API of down stream providers. I do support cleaning things up where and when it makes sense. It makes sense for some of the packages I maintain and I will gladly consider contributions.

The Facebook provider tries to use a ubiquitous language between the provider and the Facebook platform

Good point.

Who calls it a "resource owner" anyway? That's extremely abstract - in almost any case, in domain terms, the resource owner is going to be a "User", so maybe the guideline should be for other packages to adopt that.

we also have to be concerned with BC. We can't just change FaceBookUser to FacebookResourceOwner, since that might break all applications using the Facebook provider

Of course, but a name change is a trivial fix - a major version increase and a short UPGRADING.md file in the root that describes the change.

(per above, I would recommend adopting "User" over "Resource Owner" as the type name though, so in the case of the FaceBook adapter, no change.)

I think a more reasonable strategy is to identify the specific packages that you feel are "out of line", refactor them, create a pull request, and state your case

Yes, but as established, there is no defined baseline reference you can follow at the moment.

Once there is, I'd be happy to help out with PRs.

(per above, I would recommend adopting "User" over "Resource Owner" as the type name though, so in the case of the FaceBook adapter, no change.)

We won't be changing the name of this in the base package, since the resource owner is not always a user.

We won't be changing the name of this in the base package

Not what I'm suggesting :-)

I'm not sure what you're suggesting. Each provider is going to be different. Many might use the word "user," but others won't. Sometimes the resource owner is a machine/app/server (client credentials grant). Sometimes its something else. In these cases, it doesn't make sense to use the word "user."

I'm not sure what you're suggesting. Each provider is going to be different. Many might use the word "user," but others won't.

I'm not suggesting that, I'm suggesting everyone choose a concrete domain term from whatever domain that resource owner belongs to. In most cases that's going to be a "User", but if it's an "Organization" or perhaps even "Server" (can't think of a case, but) of course it should be named accordingly.

A "resource owner" is nothing concrete, it's something abstract - I'm saying an implementation of the resource owner interface is something concrete, so it should have a concrete name.

In other words, I'm agreeing with most developers, who seem to name their implementations "User".

I'm saying an implementation of the resource owner interface is something concrete, so it should have a concrete name.

Gotcha. I agree. :-)

@mindplay-dk for clarification: the term "resource owner" comes directly from OAuth spec language and is intentionally vague because it needs to apply to many contexts.

I don't feel strongly about making documented rules about things that cannot be enforced at the code level. Regardless of what the classes are called, the functionality still works the same. What would we really gain by making rules about how 3rd party providers should name things? How would we enforce that?

@shadowhand I don't expect you to enforce it - what I suggested was:

include a basic style guide, or a CodeSniffer or MessDetector profile, or perhaps better, identify a single reference implementation as the "correct" one to follow, e.g. in implementing.md, so that third-party implementations wouldn't automatically end up with all different styles depending on which implementation you used as a template

And I do think you could mandate things like packages shouldn't throw notices or warnings.

I think it's fair to say that that this issue should be renamed to "Create a third party test suite" that can be applied to downstream providers to verify general compatibility. There are certainly situations that cannot be completely covered, such as using ArrayAccessor when appropriate, but something is better than nothing.