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

Version 2.0 #35

Closed
wants to merge 0 commits into from
Closed

Version 2.0 #35

wants to merge 0 commits into from

Conversation

bencorlett
Copy link
Member

@bencorlett bencorlett commented Aug 22, 2015

Version 2

@bencorlett
Copy link
Member Author

This PR replaces #34.

@ajibarra
Copy link

Hi @bencorlett. Are you planning to merge this changes soon to develop or master? I am looking for the standard between both oauth1 and oauth2 client. Great work btw.

@bencorlett
Copy link
Member Author

@ajibarra soon(ish), still working through the rewrite for v2 in my spare time. No real ETA as yet unfortunately. v1 is stable, just has an API that doesn't match the OAuth 2 client (it used to, but OAuth 2 has since moved forward and I'm catching up here).

@stevenmaguire
Copy link
Member

@bencorlett you're making things happen! I'm happy to help out here and I don't want to impede your existing flow. If you can provide a deeper layer of detail about your current plan/thought process for the refactor, I can take care of some of the work.

@stevenmaguire
Copy link
Member

#40 encapsulates a bit of refactoring that I've been wanting to do. I've moved a handful of methods that are factories or static in nature to appropriate classes and created new classes where appropriate. My goal was to clean up the AbstractServer so it only had business critical logic.

@stevenmaguire
Copy link
Member

Send arbitrary requests

This will be possible with the getAuthenticatedRequest method.

// The server provides a way to get an authenticated API request for
// the service, using the access token; it returns an object conforming
// to Psr\Http\Message\RequestInterface.
$request = $server->getAuthenticatedRequest(
    'GET',
    'http://your.service/endpoint',
    $tokenCredentials
);

$response = $this->getHttpClient()->send($request);

@stevenmaguire
Copy link
Member

@bencorlett I see you are circling this project again. What are your thoughts on moving along this refactor? What is left to accomplish?

@bencorlett
Copy link
Member Author

Good question!

Let me pull the code down today and have a look, I really need to stop being such a slacker haha.

I’ll do that today :)

On 23 Mar 2016, at 9:28 AM, Steven Maguire notifications@github.com wrote:

@bencorlett https://github.com/bencorlett I see you are circling this project again. What are your thoughts on moving along this refactor? What is left to accomplish?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #35 (comment)

@shehi
Copy link

shehi commented Mar 31, 2016

Any ETA on v2 folks?

@GrahamCampbell
Copy link
Member

Any movement here?

@mikemand
Copy link

mikemand commented Jul 1, 2016

Any idea when this will be finished? Guzzle 4 is also EOL now, and v1 is still requiring Guzzle 3.

@bencorlett
Copy link
Member Author

I'm back actively spending time on this project and I'm hoping to have the final refactoring done from the feature/switch-to-phpspec branch merged in within the week to release a beta of v2.

Following that, we just need to add implementations for more OAuth2 Servers and then we are good tor release.

@elazar
Copy link

elazar commented Jul 18, 2016

@bencorlett Not to put too much more pressure on you, but a patch for a security vulnerability was included in Guzzle 6.2.1. I'm assuming the vulnerability exists in all prior versions and that those versions won't receive patches. So, the sooner the switch to Guzzle 6 can happen, the better. 😄

@bencorlett
Copy link
Member Author

Ooo, interesting. Could you please provide me with a link? I might see if I can also patch 1.x to run on a newer Guzzle as well without needing a major bump. Pretty sure we won't be modifying any public methods so it could be a patch version.

Sent from my iPhone

Please excuse my brevity

On 23 Mar. 2016, at 9:55 am, Ben Corlett bencorlett@me.com wrote:

Good question!

Let me pull the code down today and have a look, I really need to stop being such a slacker haha.

I’ll do that today :)

On 23 Mar 2016, at 9:28 AM, Steven Maguire notifications@github.com wrote:

@bencorlett I see you are circling this project again. What are your thoughts on moving along this refactor? What is left to accomplish?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@elazar
Copy link

elazar commented Jul 18, 2016

@bencorlett Here's the site for the vulnerability: https://httpoxy.org/. The Guzzle release notes I linked in my earlier comment include a related item.

@bencorlett
Copy link
Member Author

Oh, shit. Sorry I didn't see your link prior.

Okay, cool. I have a project for this morning ;)

Sent from my iPhone

Please excuse my brevity

On 19 Jul. 2016, at 8:45 am, Matthew Turland notifications@github.com wrote:

@bencorlett Here's the site for the vulnerability: httpoxy.org. The Guzzle release notes I linked in my earlier comment include a related item.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

composer.json Outdated
"@phpcs",
"@phpcpd",
"@phploc",
"@phpspec",
Copy link
Member

Choose a reason for hiding this comment

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

This test fails on the HHVM environment in Travis. Do we want to ignore HHVM? Do we want to find a new test solution?

Copy link

Choose a reason for hiding this comment

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

I'd recommend ignoring HHVM at this age, since PHP 7 is as fast. PHP ecosystem has changed a lot since HHVM's inception.

@stevenmaguire
Copy link
Member

@bencorlett I am still interested in helping to get this project moved up and out to 2.0. It seems like the current test suite configuration is causing some issues with CI. I've added some comments and would like some input and/or direction from you on how we should proceed.

I am happy to get some more work done here :)

@stevenmaguire
Copy link
Member

Yoo-hoo @bencorlett

@prisis
Copy link
Contributor

prisis commented Apr 5, 2018

Do you need some help or is this project dead?

@bencorlett
Copy link
Member Author

bencorlett commented Apr 6, 2018 via email

@philsturgeon
Copy link
Member

We had this conversation over a year ago @bencorlett so yeah I'd say the swap should happen! 😅

I'm not involved in the League anymore, so pester @frankdejonge or somebody to get this a new home.

@stevenmaguire
Copy link
Member

I'm happy to take on more responsibility with this project. I'll need to get back up to speed a bit. I've been very involved in this last round of changes but it's been quite some time since my head has been in this space :)

@shehi
Copy link

shehi commented Aug 20, 2020

Any update here? :) We have Guzzle 7 now with PHP 7.2+ support.

@shehi
Copy link

shehi commented Aug 20, 2020

Another question of mine is: who uses OAUTH-1 nowadays?! Haven't whole world moved to OAUTH-2 already?

@GrahamCampbell
Copy link
Member

Twitter still uses oauth 1 only.

@shehi
Copy link

shehi commented Aug 20, 2020

@shehi
Copy link

shehi commented Aug 20, 2020

@GrahamCampbell , sorry my bad. That link is not OAuth-2-on-behalf-of-user. It's just JWT-Bearer authentication (client-credential flow), on behalf of App.

@mfn
Copy link

mfn commented Aug 20, 2020

Indeed, all the "real stuff" is still OAuth 1.0a => https://developer.twitter.com/en/docs/authentication/oauth-1-0a
(basically the most important stuff for serious Twitter interaction)

@shehi
Copy link

shehi commented Aug 20, 2020

Yea. I know for sure almost noone will bother updating this package. Tech is too old to be of any value to anyone.

Maybe for the sake of moving forward, at Socialite side there should be new major version release with Oauth-1 and Twitter left behind. Otherwise everything will be stuck in past.

@shehi
Copy link

shehi commented Aug 20, 2020

Can someone add me as a Member to this repo?

CC: @bencorlett

@shehi
Copy link

shehi commented Aug 20, 2020

I need that to be able to stop Travis-CI runs which I know will fail. Waiting for them takes too long.

@shehi
Copy link

shehi commented Aug 21, 2020

After spending 7-8 hours to refactor this codebase, I can say the following:

  • This codebase has very critical typing issues. Many methods can return totally unrelated, multiple types (sometimes int|string, sometimes object|string, even void|string as well).
  • For some reason it attempts to re-implement PHP's http_build_query() function and with errors at that.
  • Even after I completely refactored it, adding declare(strict_types=1) in all files is completely impossible. One of the main methods, sign() method attempts to use non-strings (booleans, nulls and similar) as strings, url-decoding and re-encoding them back and forth, which creates completely unpredictable outcomes. I attempted to add fixes there, but it was too much hassle to go through, so no PHP declarations for the moment.

Overall, even though there is Version-2 branch half-done, I'd strongly suggest actually redoing that work on top of my refactoring, also adding declare() s to files.

@philsturgeon
Copy link
Member

@shehi this codebase started life as codeigniter-oauth1, which was built to work with PHP 4 as well as PHP 5. As such, some functions which existed in PHP 5 had to be reimplimented for the same of PHP 4 support. It's possible you're noticing some of this legacy, as of course you might notice with the types. Please boyscout respectfully and help chip away at any type mismatches, as again this was not built when types existed.

@philsturgeon
Copy link
Member

@shehi also maybe you could start your own PR, as this was started by somebody who is no longer active on the project.

@shehi
Copy link

shehi commented Aug 21, 2020

@philsturgeon really? CodeIgnitor and PHP4? Didn't know that. Older than I imagined!

Already started & finished my PR. I was just giving input regarding this PR, because a lot of work was done here without addressing the points I mentioned above. IMHO these shortcomings and problems should be addressed before building new features, or at least in parallel. My observation was: they weren't.

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.

None yet

10 participants