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

RFC: Refactoring / 3.0 - PSR-0, Composer, and more... #34

Closed
EvanDotPro opened this issue Jul 13, 2012 · 43 comments
Closed

RFC: Refactoring / 3.0 - PSR-0, Composer, and more... #34

EvanDotPro opened this issue Jul 13, 2012 · 43 comments

Comments

@EvanDotPro
Copy link

I'd like to propose that a new branch be "officially" started, possibly as a beginning to a 3.0 release.

  • Upgrade to PSR-0 and (possibly) namespaces -- I can understand the argument against making 5.3 a requirement (Edit: To carify, the extra adapters do not conform to PSR-0 and are astonishingly annoying to set up autoloading for properly in the context of other projects due to the hard-coded require_once statemnts in HybridAuth.)
  • Implement some coding standards
  • Remove all the static usage in favor of real OOP practices
  • Add official composer/packagist support
  • Make it overall more friendly for integration with projects and frameworks
  • Unit tests

Yes, I could fork and just start working on it, but I'd prefer to get upstream support from the project and users so that it can hopefully be a more collaborative effort if there's sufficient interest.

Thoughts?

@daviddesberg
Copy link

I agree with, quite literally, everything in the above post.
+1

@PrplHaz4
Copy link
Contributor

PrplHaz4 commented Aug 2, 2012

I agree as well...it would be great to prevent the proliferation of hybridauth-like libraries if possible, and these steps will help keep it relevant.

@rhutchison
Copy link

+1

@cincodenada
Copy link

I'm leaving looking at implementing HybridAuth or a similar system in a couple projects, and was planning on forking and cleaning this up a bit anyway, so I'll keep an eye on this issue and hopefully pitch in as well :)

@evillemez
Copy link

Huge +1 to this.

@Krienas
Copy link

Krienas commented Aug 29, 2012

Agree with this.

@rafamd
Copy link

rafamd commented Sep 2, 2012

way to go!

@bycosta
Copy link

bycosta commented Oct 22, 2012

+1

1 similar comment
@danizord
Copy link

👍

@patcon
Copy link

patcon commented Feb 21, 2013

👍

There seems to be more of a community here, so I'd rather see this project succeed, but for the record, there's another project that seems to be further along... Particularly nice that all the strategies are separated out into their own composer packages: https://github.com/uzyn/opauth-github

@PrplHaz4
Copy link
Contributor

I have been following both efforts for a while - the opauth library probably had better timing (to take advantage of some more progressive practices), but is lacking in documentation (and community as you've stated). Either way, t's a good opportunity to get the collective effort heading in the right direction.

@patcon
Copy link

patcon commented Feb 21, 2013

Just started up a new meta thread, and @-mentioned a bunch of opauth contributors into the loop: #97

Thinking maaaaybe, if it's not being too optimistic to suggest, the members of both communities could hash some things out :)

@hybridauth
Copy link
Collaborator

Hi all,

First, I deeply apologize for the tremendous delay.

While I agree with most, if not all of what @EvanDotPro said, I also would like to know, if possible, the major consensus of the people here about these two questions:

  • should Hybridauth 3.x do more of the same thing, but attempt to do it better; or,
  • should Hybridauth "evolve" to a new model and/or provide new features.

Believing that Hybridauth 2.x core really needs some serious rework and that a consensus based process would be the best approach to solve this. Your comments, contributions and criticism are more than welcome and greatly appreciated.

A draft Roadmap could be found at Hybridauth wiki.
Please feel free to edit the document for any improvements or suggestions.
https://github.com/hybridauth/hybridauth/wiki/HybridAuth-3.x-Roadmap

A proposal of Hybridauth 3.0:
https://github.com/hybridauth/hybridauth/tree/3.0.0

Thanks!

@jeffreytgilbert
Copy link
Contributor

Is MySpace no longer a relevant login worth maintaining? Seemed like integrating with their system was easy enough. The features I would like to see HybridAuth 3 adopt (which haven't been mentioned) would be:

Better encapsulation using OOP for profiles where the data given back arent in public variables, but behind public methods that can be extended and overwritten.

Better code completion support for smart IDEs like Eclipse and PHPStorm where the Objects returned from calls either contain cast methods or the calls return objects of specific types. This also means avoiding the temptation of adopting bad practices currently becoming popular such as use of anonymous/callable/closure functions which as specifically abandon structure.

Better error handling by throwing exceptions that map to specific errors, there by allowing programmers to map to known exception types creating less trial and error (mostly error).

Not adopting and staying clear of passing around hashed arrays in place of parameters to methods, which has been popularized by Cake and other frameworks, but makes it impossible to get any autocomplete support or know what arguments to pass to a method by jumping to that method.

The ability to return either serialized or unserialized structured login objects that can be stored and retrieved and re-set in hybrid auth, meaning you can turn on or off a social hook at any point. This was something I had to write my own method to do and knew that it would break if the internal sessions HA2 uses ever changed format. It was also a requirement for my project as HA2 was not the only authentication type allowed on the site and sessions would need to be restored upon login if the user logged out.

All of these things I mention because during my integration with HA2 I had issues, at some point, that these features would have solved and helped with during integration.

This one might be a total long shot, but support for multiple sign ons from the same provider would be huge. There are business models that would benefit from the ability to manage multiple facebook or linked in accounts, and I don't know of an authentication library that adequately addresses this ability. I know I considered this one of the shortcomings of HA2 during my adoption, but decided it was an acceptable loss at the time.

On Mar 1, 2013, at 1:31 PM, hybridauth notifications@github.com wrote:

Hi all,

First, I deeply apologize for the tremendous delay.

While I agree with most, if not all of what @EvanDotPro said, I also would like to know, if possible, the major consensus of the people here about these two questions:

should Hybridauth 3.x do more of the same thing, but attempt to do it better; or,
should Hybridauth "evolve" and provide new features.
Believing that Hybridauth 2.x core really needs some serious rework and that a consensus outcome would be the best approach to solve this. Your comments, contributions and criticism are more than welcome and greatly appreciated.

A draft Roadmap could be found at Hybridauth wiki.
Please feel free to edit the document for any improvements or suggestions.
https://github.com/hybridauth/hybridauth/wiki/HybridAuth-3.x-Roadmap

A proposal of Hybridauth 3.0:
https://github.com/hybridauth/hybridauth/tree/3.0.0

Thanks!


Reply to this email directly or view it on GitHub.

@danizord
Copy link

danizord commented Mar 1, 2013

@hybridauth what about unit tests?

@patcon
Copy link

patcon commented Mar 1, 2013

Like most of what you said, but one comment:

Not adopting and staying clear of passing around hashed arrays in place of parameters to methods

My impression was that this was pretty common practice, and not something dreamt up by any one framework PHP framework -- just makes things more pluggable, right? I'm most familiar with that hash being "config", because it's hard to know all the "config" settings that any one plugin will need.

Ex: https://github.com/mitchellh/vagrant/blob/master/lib/vagrant/plugin/v2/provisioner.rb

I'm not a pro coder btw, so maybe I'm wrong :)

@jeffreytgilbert
Copy link
Contributor

It's fine for config stuff that you wouldnt want to pass in through some structured call, especially for scenarios when you're pulling things in through INIs. More often than not, i see it being used like its a convenience to developers (see: Cake) but what it actually does is take away "required" parameters, code hinting, and default values and really hinders authoring work flows.

On Mar 1, 2013, at 2:29 PM, Patrick Connolly notifications@github.com wrote:

Like most of what you said, but one comment:

Not adopting and staying clear of passing around hashed arrays in place of parameters to methods

My impression was that this was pretty common practice, and not something dreamt up by any one framework PHP framework -- just makes things more pluggable, right? I'm most familiar with that hash being "config", because it's hard to know all the "config" settings that any one plugin will need.

Ex: https://github.com/mitchellh/vagrant/blob/master/lib/vagrant/plugin/v2/provisioner.rb

I'm not a pro coder btw, so maybe I'm wrong :)


Reply to this email directly or view it on GitHub.

@hybridauth
Copy link
Collaborator

@jeffreytgilbert

Is MySpace no longer a relevant login worth maintaining?

actually MySpace still worth maintaining. I just think this could be done separately.

the way projects are maintained have dramatically changed in the PHP community due to the growing popularity of both Git and Composer. and my idea is to join the best of the both worlds by,

a) only maintain the most used providers within the upstream branch (https://www.google.com/search?q=social+login+preferences), so any changes on these providers APIs could be made on a timely fashion, while

b) giving the opportunity for other to create and maintain their own services adapters.

Better encapsulation using OOP for profiles where the data given back arent in public variables

beside that I hate accessors and mutators, User Profile Model is merely a structured container and in most cases it only contain data as retrieved from providers APIs.

Better error handling by throwing exceptions that map to specific errors

I took this into account, however, I opted for using error codes rather then creating multiples exceptions.
https://github.com/hybridauth/hybridauth/blob/3.0.0/src/Hybridauth/Exception.php

support for multiple sign ons from the same provider would be huge.

there actually a workaround for this.. but this request is worth a thought for sure
https://groups.google.com/d/msg/hybridauth/z48yHZ-KKxE/kcUrTrTJkREJ

@jeffreytgilbert, @patcon

Not adopting and staying clear of passing around hashed arrays in place of parameters to methods

I believe this is due to the lack of proper class method overloading with PHP, and arrays are mainly used as a workaround..
I'm ok with using method args when possible, but ultimately, arrays will be used to make life a bit easier.

@danizord

Sure. Thanks for pointing this as well.

once again, these are just my thoughts ..

@patcon
Copy link

patcon commented Mar 1, 2013

Thanks for humouring me with the helpful answer @jeffreytgilbert

@jeffreytgilbert
Copy link
Contributor

I appreciate the feedback. Getters and setters are a pain to write initially, but that pain is a one time problem. The code consuming these objects shouldn't need to care if they're set, properly formatted, or accessing data from a different source. I stand by my original notion that these should have getters, minimally, and that the objects values shouldn't be directly accessible public variables. At worst it's a mild inconvenience to author them initially. At best, it's sound strategy and future proofing for core objects that can confidently be extended by developers. For internal data objects, i couldn't care less how they're authored and maintained. :)

On Mar 1, 2013, at 4:07 PM, hybridauth notifications@github.com wrote:

@jeffreytgilbert

Is MySpace no longer a relevant login worth maintaining?

actually MySpace still worth maintaining. I just think this could be done separately.

the way projects are maintained have dramatically changed in the PHP community due to the growing popularity of both Git and Composer. and my idea is to join the best of the both worlds by,

a) only maintain the most used providers within the upstream branch (https://www.google.com/search?q=social+login+preferences), so any changes on these providers APIs could be made on a timely fashion, while

b) giving the opportunity for other to create and maintain their own services adapters.

Better encapsulation using OOP for profiles where the data given back arent in public variables

beside that I hate accessors and mutators, User Profile Model is merely a structured container and in most cases it only contain data as retrieved from providers APIs.

Better error handling by throwing exceptions that map to specific errors

I took this into account, however, I opted for using error codes rather then creating multiples exceptions.
https://github.com/hybridauth/hybridauth/blob/3.0.0/src/Hybridauth/Exception.php

support for multiple sign ons from the same provider would be huge.

there actually a workaround for this.. but this request is worth a thought for sure
https://groups.google.com/d/msg/hybridauth/z48yHZ-KKxE/kcUrTrTJkREJ

@jeffreytgilbert, @patcon

Not adopting and staying clear of passing around hashed arrays in place of parameters to methods

I believe this is due to the lack of proper class method overloading with PHP, and arrays are mainly used as a workaround..
I'm ok with using method args when possible, but ultimately, arrays will be used to make life a bit easier.

@danizord

Sure. Thanks for pointing this as well.

once again, these are just my thoughts ..


Reply to this email directly or view it on GitHub.

@hybridauth
Copy link
Collaborator

I usually avoid writing getters and setters not out of laziness, as a matter of fact I do not use IDEs w/ code completion nor I do like them :)

there is a long long list of good reasons for why getters and setters should be used in which I agree as well, but my point is: there is no need for then when using raw data like the users profile.

nonetheless, if other people disagree, then it's fair enough and getters shall be used.

@dorongutman
Copy link
Contributor

a hard +1 on more strictness and structure (getters and setters, ,multiple exception types, no hashed arrays - yes arguments/parameters)

@bycosta
Copy link

bycosta commented Mar 2, 2013

Another hard +1 !

On Friday, 1 de March de 2013 at 20:52, Doron Gutman wrote:

a hard +1 on more strictness and structure (getters and setters, ,multiple exception types, no hashed arrays - yes arguments/parameters)


Reply to this email directly or view it on GitHub (#34 (comment)).

@hybridauth
Copy link
Collaborator

Welp, Okey.

Following that approach and recommendations, here's three possible examples of using Hybridauth:

ex1: ( using a config array and defaults )

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$config = array(
    "base_url" => "http://domain.com/path/to/hybridauth/src/",
    "providers" => array( 
        "Google" => array(
            "enabled" => true,
            "keys" => array ( 'key' => '***.apps.googleusercontent.com', "secret" => '***' )
        )
    ) 
);

$profile = ( new \Hybridauth\Hybridauth( $config ) )->authenticate( "Google" )->getApi()->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();

ex2: (flexible but verbose; no config required)

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$hybridauth = new \Hybridauth\Hybridauth();

$adapter = $hybridauth->getAdapter( "Google" );

$adapter->getAuthService()->setApplicationId( '***.apps.googleusercontent.com' );
$adapter->getAuthService()->setApplicationSecret( '***' );
$adapter->getAuthService()->setApplicationScope( 'https://www.googleapis.com/auth/userinfo.profile' );

$adapter->getAuthService()->setEndpointBaseUri('https://www.googleapis.com/oauth2/v1/');
$adapter->getAuthService()->setEndpointRedirectUri( 'http://domain.com/path/to/hybridauth/src/');

$adapter->getAuthService()->setEndpointAuthorizeUri('https://accounts.google.com/o/oauth2/auth');
$adapter->getAuthService()->setEndpointRequestTokenUri('https://accounts.google.com/o/oauth2/token');
$adapter->getAuthService()->setEndpointTokenInfoUri('https://www.googleapis.com/oauth2/v1/tokeninfo');

$adapter->getAuthService()->setEndpointAuthorizeUriAdditionalParameters(array( 'access_type' => 'offline' ));

$adapter->authenticate();

$profile = $adapter->getApi()->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();

ex3: Using a stored tokens

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$hybridauth = new \Hybridauth\Hybridauth();

$adapter = $hybridauth->getAdapter( "Google" );

$profile = $adapter->getApi( 'ya29.****' )->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();

Thoughts?

@jeffreytgilbert
Copy link
Contributor

I can't give this the time it deserves at the moment, but tomorrow or Monday I should have time to think it out and reply with something meaningful. Are you implementing this now, or just testing the waters? If it's being worked on in the 3 branch right now, I'll fork it and send over any pull requests i think might help.

On Mar 2, 2013, at 3:08 PM, hybridauth notifications@github.com wrote:

Welp, Okey.

Following that approach and recommendations, here's three possible examples of using Hybridauth:

ex1: ( using a config array and defaults )

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$config = array(
"base_url" => "http://domain.com/path/to/hybridauth/src/",
"providers" => array(
"Google" => array(
"enabled" => true,
"keys" => array ( 'key' => '_.apps.googleusercontent.com', "secret" => '_' )
)
)
);

$profile = ( new \Hybridauth\Hybridauth( $config ) )->authenticate( "Google" )->getApi()->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();
ex2: (flexible but verbose; no config required)

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$hybridauth = new \Hybridauth\Hybridauth();

$adapter = $hybridauth->getAdapter( "Google" );

$adapter->getAuthService()->setApplicationId( '_.apps.googleusercontent.com' );
$adapter->getAuthService()->setApplicationSecret( '
_' );
$adapter->getAuthService()->setApplicationScope( 'https://www.googleapis.com/auth/userinfo.profile' );

$adapter->getAuthService()->setEndpointBaseUri('https://www.googleapis.com/oauth2/v1/');
$adapter->getAuthService()->setEndpointRedirectUri( 'http://domain.com/path/to/hybridauth/src/');

$adapter->getAuthService()->setEndpointAuthorizeUri('https://accounts.google.com/o/oauth2/auth');
$adapter->getAuthService()->setEndpointRequestTokenUri('https://accounts.google.com/o/oauth2/token');
$adapter->getAuthService()->setEndpointTokenInfoUri('https://www.googleapis.com/oauth2/v1/tokeninfo');

$adapter->getAuthService()->setEndpointAuthorizeUriAdditionalParameters(array( 'access_type' => 'offline' ));

$adapter->authenticate();

$profile = $adapter->getApi()->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();
ex3: Using a stored tokens

include "src/Hybridauth/Hybridauth.php" ;

\Hybridauth\Hybridauth::registerAutoloader();

$hybridauth = new \Hybridauth\Hybridauth();

$adapter = $hybridauth->getAdapter( "Google" );

$profile = $adapter->getApi( 'ya29.****' )->getUserProfile();

echo 'Hi ' . $profile->getDisplayName();
Thoughts?


Reply to this email directly or view it on GitHub.

@hybridauth
Copy link
Collaborator

sorry I wasn't clear. this is actually a proposal for Hybridauth 3.x public API.

I just want to make sure that most people here would agree on that before implementing it this way..

@jacksonp
Copy link
Contributor

Thanks first of all for HybridAuth. I'd like to see incremental changes over a large refactoring, I don't see how one is warranted. For what it's worth though, ex1 above looks like the least painful of the 3!

@hybridauth
Copy link
Collaborator

@jacksonp

Ha 3 is more about refactoring the library internally to make it more extensible/flexible (ex2) and for the most part will be kept compatible with 2.x (ex 1)

@Krienas
Copy link

Krienas commented Mar 18, 2013

One thing which I really want to see is abstraction of configuration as much as it is possible. Application keys and secrets are good example what could be abstracted. They are part of almost every configuration, but configuration sent differ for different service providers. That could be abstracted within driver. Not sure about possibilities to abstract scope control, but that could be useful too.

@StorytellerCZ StorytellerCZ added this to the 3.0.x milestone Feb 17, 2014
@jklein
Copy link

jklein commented Mar 10, 2014

I'm considering using hybridauth in a new project, but to be honest seeing this issue with a ton of reasonable suggestions and no movement for a year is concerning. Is there a plan to ever finish the 3.0 release and incorporate this feedback?

@dashohoxha
Copy link
Contributor

Why don't you give a try to the branch 3.0.0: https://github.com/hybridauth/hybridauth/tree/3.0.0
Maybe it works correctly. If not, maybe you can fix and improve it. I think that @hybridauth would be willing to give commit access to anyone that is willing to work on that branch.

I have just implemented an OAuth2 client for PHP: https://github.com/dashohoxha/oauth2-client-php
Maybe you can integrate this one as well on the branch 3.0.0

@nazar-pc
Copy link
Contributor

@jklein, I recommend you to use latest version of code from master branch for now. There was a long period, when there were no development in this library, but now several guys working on it, merged lot of fixes for different issues, and most actual is master git branch. When they finish - here, in repository, next release version should appear.
We all are a little bit confused with versions, but that should be fixed in near future with new stable release. About 3.x - original maintainer started it, but didn't finished - so it will be continued after we get new stable release of 1.x.

@localheinz
Copy link
Contributor

Any news on this one?

Seems long overdue!

@StorytellerCZ
Copy link
Contributor

v3 dev is currently PSR-4 compliant

@localheinz
Copy link
Contributor

@AdwinTrave

That would be 3.0.0-Remake?

@StorytellerCZ
Copy link
Contributor

Yes

@lorezyra
Copy link

lorezyra commented May 2, 2015

Any plans to implement namespaces?

@nazar-pc
Copy link
Contributor

nazar-pc commented May 2, 2015

As written before in this thread - already implemented in experimental 3.0-Remake branch

@lorezyra
Copy link

lorezyra commented May 2, 2015

Cool. 👍 I look forward to seeing this stable enough for production.

@soullivaneuh
Copy link
Contributor

Follow PSR-1/2 practices could be also a good start.

See #459 as example.

@soullivaneuh
Copy link
Contributor

@EvanDotPro please see #461.

Feel free to comment it. 👍

@wheelsandcogs
Copy link

Is 3.0 currently in a usable state? i.e. is it working with a few missing providers / small bugs or is it still under heavy dev / fairly broken?

I'm trying to make a decision on whether to go with 3.0 or 2.4 on a new project, so a rough idea of stability would really help, cheers.

@miled
Copy link
Member

miled commented Jun 9, 2015

@wheelsandcogs 3.0 is still under heavy development and not suited for production. afaik, there's few braves people who already using it, but the decision is yours.

For anyone interested to know, the exact status of project can be found at https://github.com/hybridauth/hybridauth/blob/3.0.0-Remake/TODO.md

@miled miled closed this as completed Jun 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests