Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Rename kAFOAuthCredentialServiceName to kAFOAuth1CredentialServiceName #81

Merged
merged 1 commit into from
Sep 9, 2013
Merged

Conversation

ejensen
Copy link
Contributor

@ejensen ejensen commented Sep 2, 2013

Fixes a duplicate symbol error when linking to AFOAuth2Client.

Ref https://github.com/AFNetworking/AFOAuth2Client/blob/master/AFOAuth2Client/AFOAuth2Client.m#L33

Fixes a duplicate symbol error when linking to AFOAuth2Client.
mattt added a commit that referenced this pull request Sep 9, 2013
Rename kAFOAuthCredentialServiceName to kAFOAuth1CredentialServiceName
@mattt mattt merged commit e548db2 into AFNetworking:master Sep 9, 2013
@mattt
Copy link
Contributor

mattt commented Sep 9, 2013

Thanks, @ejensen. Pushing a new release now.

@dcaunt
Copy link

dcaunt commented Sep 9, 2013

Both constants still use the same value, AFOAuthCredentialService – wouldn't this cause problems if you use both clients and the same identifier?

Shouldn't both be renamed explicitly, i.e.

kAFOAuth1CredentialServiceName = @"AFOAuth1CredentialService";
kAFOAuth2CredentialServiceName = @"AFOAuth2CredentialService";

@ejensen
Copy link
Contributor Author

ejensen commented Sep 9, 2013

Both constants still use the same value, AFOAuthCredentialService – wouldn't this cause problems if you use both clients and the same identifier?

@dcaunt Yes, keychain entries would be overridden when set with the identical identifier.

Shouldn't both be renamed explicitly, i.e.

kAFOAuth1CredentialServiceName = @"AFOAuth1CredentialService";
kAFOAuth2CredentialServiceName = @"AFOAuth2CredentialService";

I thought about renaming the service names as you suggested. But doing so, would cause all existing credentials stored under the previous service name (AFOAuthCredentialService) to be inaccessible to the client. Upgrade would introduce a fairly significant change for those who haven't overridden the default service name.

@mattt Thoughts?

@mattt
Copy link
Contributor

mattt commented Sep 19, 2013

@ejensen It's an unfortunate conflation of constant values, but in practice, I think that the current setup is actually not a problem. After all, has anyone heard of a web service that provided authentication with OAuth 1 and 2?

Regardless, these values could and probably should be renamed in a future minor or major release.

@ejensen ejensen deleted the patch-1 branch October 10, 2013 05:14
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.

None yet

3 participants