Update Facebook Provider - Fixes #665 #688

Merged
merged 6 commits into from Oct 29, 2016

Projects

None yet

7 participants

@annuh
Contributor
annuh commented Oct 14, 2016

This PR updates the Facebook provider for HybridAuth.

An updated Facebook SDK that used the Facebook Graph API v2.8, so the login API calls still work after October 30th.

The Facebook SDK OAuth client uses parse_str() function which causes problems with the HybridAuth urls, since the hauth.start and hauth.done query parameters are converted to hauth_start and hauth_done (see http://stackoverflow.com/questions/68651/get-php-to-stop-replacing-characters-in-get-or-post-arrays). This raised an 'uri_redirect mismatch' error in the OAuth client.

TL;DR: You need to change to OAuth callback url in Facebook from
http://www.yoursite.com/HybridAuth?hauth.done=Facebook
to http://www.yoursite.com/HybridAuth?hauth_done=Facebook

It is working for me, please let me know if you find any problems

annuh added some commits Oct 14, 2016
@annuh annuh Update Facebook Provider - Fixes #665
* Add Facebook SDK to composer.json
* Change Facebook adapter and added workaround for broken Oauth links
* Clean-up code
b1c89a2
@annuh annuh Clean-up code
Fixed typo
3c93c76
@StorytellerCZ StorytellerCZ modified the milestone: 3.0.x, 2.x Oct 14, 2016
@StorytellerCZ
Contributor

Thank you! Amazing work!

This is definitely a breaking change, but given how far we have fallen behind, it was bound to happen.
I would like to hear what others think as well. If I don't forget I will merge this and release new version on the 21st. Hopefully this will give enough of a time for people to make the changes.

Could you also please update the docs on the gh-pages branch? Thanks!

@egyptianbman

You sir, are a Godsend. Thank you for doing this!

@StorytellerCZ, maybe add a minor version commit before merging this in that trigger_errors an E_USER_WARNING to let people know that they need to upgrade before October 30th?

@StorytellerCZ StorytellerCZ merged commit c7fa331 into hybridauth:master Oct 29, 2016
@Shade-
Contributor
Shade- commented Oct 29, 2016

I do not support this change and I think this should be reverted as soon as possible. First of all, while most projects use Composer there are still some who do not, so including the SDK manually should be at least documented somewhere for those projects in which Composer is not used and cannot be used.

Secondly, the latest Facebook SDK does not support PHP 5.2. This breaks HybridAuth minimum requirements and shifts it to PHP 5.4. There was a reason why the SDK was edited back in the days when Facebook updated to v4 and v5; the "old" 3.2.3 is still working fine with the patches applied on HybridAuth 2.7.0.

@egyptianbman

I have to say, I don't agree with you, @Shade-.

There are two major issues with continuing to support old versions such as PHP 5.2 and 5.3:

  1. In a way, supporting such old versions is "promoting" using the old versions. Many a developer will not upgrade until they're forced to. While I understand some may not be able to due to other dependencies, that's why there is versioning in place. Those people can choose to stay on version 2.7 of hybridauth until they can properly upgrade their version of PHP.
  2. Software evolves. Changes are introduced constantly and not utilizing composer or continuing to support very old versions of PHP just complicates development and maintenance of this project.

If someone isn't using composer, they have the ability to check out various versions of this library. They also have the ability to cherry-pick what commits they want included. I don't see staying up-to-date as damaging in any way since there are more than enough options for those who choose to remain on an outdated version of PHP and/or not use composer.

@Shade-
Contributor
Shade- commented Oct 29, 2016

It's quite useless requiring a newer PHP version for just one provider IMHO. If it was me,I would support PHP 7+; but since HybridAuth has been compatible with PHP 5.2, there's no need to upgrade the requirements for one provider only. Also, other patches have been introduced which could be applied to existing projects using older versions of PHP. A minor version should not change the requirements, I would have waited 3.0.0 instead to pull this change.

Besides, I don't use Composer because my host does not give support for it as it's a shared one. Many of them do not allow Composer to be ran. In addition to this, I have been integrating HybridAuth with MyBB which does not need Composer to be ran. This is a major issue for me (and for everyone integrating HybridAuth with existing frameworks which do not need a dependency manager). I'm not against it, but I do believe the SDK should be added to the package, or detailed instructions on how to download and add it to HybridAuth should be provided in the docs.

@StorytellerCZ
Contributor

We don't really have much choice in upgrading the Facebook provided as the v2.7 will no longer work after tomorrow. Though I agree with @Shade- that we should do v2.8.1 with the latest Facebook SDK included.

On the ongoing debate of minimal PHP requirements. According to stats, PHP 5.2 no longer shows on the charts. So we should be able to slowly upgrade to higher versions, but that is something I was hoping for major version (as this should have been as well).

Issues like this expose the biggest problem with the project: @miled has been missing, so no progress on v3. I don't have time to do anything except occasionally review PR and draft new versions. Since my startup is JS land I'm unable to incorporate it into my daily work so there isn't any improvement from my side. Yet last time I checked there was no free alternative to HybridAuth, so many still relay on it.

So to sum it up. Unless new people step forward to help maintain and develop this project we will have this debate over and over again.

As to how to go forward on this particular issue. Submit PRs.

@HealingRooms

I'm having a particular problem with the most recent version (as of Nov 1, 2016) and Facebook, with the only code modified since updating being those of the hybridauth/config.php file.

Here is one error, retrieved from the site's own error_log file

PHP Fatal error: Class 'Facebook\Facebook' not found in /home/.../hybridauth-git/hybridauth/Hybrid/Providers/Facebook.php on line 56

That is the line starting with this:

    $this->api = new FacebookSDK([

And here is part of another debugging line, retrieved from the custom debugging file, defined in the config.php file:

    Hybrid_Auth::setup( Facebook ), no params given.

It, too, seems to be pointing to the Facebook function.

@Shade-
Contributor
Shade- commented Nov 1, 2016

You're most probably missing the SDK, which is included only if you have installed HybridAuth with Composer.

@HealingRooms

Thank you, @Shade-

This was installed via FTP, from the most recent downloadable zip file.

Do you have any pointers for what would make this work?

@Shade-
Contributor
Shade- commented Nov 2, 2016

Well the SDK is available here: https://github.com/facebook/php-graph-sdk

But I have no clues where to include the folder. Maybe @StorytellerCZ knows where Composer compiles the SDK to.

@akf
akf commented Nov 2, 2016

If you're not using composer, follow the link from the php-graph-sdk repo to https://developers.facebook.com/docs/php/gettingstarted -- and look at the section headed "Manually installing (if you really have to)".

It looks like the download button on that page gives you release 5.0.0, whereas you can get a much more recent version off github.

Probably VERY IMPORTANT: https://github.com/facebook/php-graph-sdk has a README noting that it's not the stable branch, and to go to https://github.com/facebook/php-graph-sdk/tree/5.4 for stable.

@Shade-
Contributor
Shade- commented Nov 2, 2016

The problem is – you don't know where to put all those files.

@annuh
Contributor
annuh commented Nov 2, 2016

@Shade- It doesn't really matter. A common way is to create a folder 'Vendor' and put all external libraries in this folder.

Whenever you include the HybrydAuth library, you also need to require the Facebook library, using ie.
`require_once DIR . 'Vendor/facebook-sdk-v5/autoload.php';``

@Shade-
Contributor
Shade- commented Nov 2, 2016

@annuh I know you can require that on your own. I am a developer too.

However, since this project aims to provide a centralized, all-in-one authentication environment, requiring external libraries/SDKs must be handled by the project, not by the final user.

#698 and @HealingRooms are the first evidences of why this PR was a (partial) mistake.

@HealingRooms

Here is some progress, since my problem 9 days ago with trying a simple FTP approach to installing the HybridAuth. We upgraded the WHM to v60, upgraded to EasyApache4 (with some challenges), activated Composer, and we have installed the Facebook Graph SDK.

We got a developers key for Twitter, and the Twitter script seems to be working fine.

Unfortunately, the Facebook script included in the most recent download (9 days ago) seems to be missing something.

PHP Fatal error: Class 'Facebook\Facebook' not found in /home/########/public_html/hybridauth-git/hybridauth/Hybrid/Providers/Facebook.php on line 57

The original first couple lines of Facebook.php are as follows:

use Facebook\Exceptions\FacebookSDKException;
use Facebook as FacebookSDK;

Then lines 57 and 64 define the following:

$this->api = new Facebook\Facebook([
            'app_id' => $this->config["keys"]["id"],
            'app_secret' => $this->config["keys"]["secret"],
            'default_graph_version' => 'v2.8',
            'trustForwarded' => $trustForwarded,
]);

$this->api = new FacebookSDK([
            'app_id' => $this->config["keys"]["id"],
            'app_secret' => $this->config["keys"]["secret"],
            'default_graph_version' => 'v2.8',
            'trustForwarded' => $trustForwarded,
]);

We've tried a work-around, by changing this line:

use Facebook as FacebookSDK;

to this:

use Facebook\Facebook as FacebookSDK;

...but it only seems to shift the problem elsewhere on that Facebook.php file.

Does anyone thing that I might be missing something else that I didn't see in the documentation for HybridAuth?

@HealingRooms
HealingRooms commented Nov 10, 2016 edited

Here is the MISSING INGREDIENT for the Facebook problem with the most recent version of HybridAuth (as of 2016-11-10).

Put this line...

require __DIR__ . '/../vendor/autoload.php';

...at the top of hybridauth/index.php.

This was discovered by Keven Hughes at LiquidWeb.com.

@Shade-
Contributor
Shade- commented Nov 10, 2016

That is good if you have Facebook SDK in the respective folder. You require the autoloader which handles everything for you. However if you do not have the SDK in place this won't work.

I may end up adding a PR for 2.8.1 as soon as possible.

@tobeorla
Contributor
tobeorla commented Jan 4, 2017

@Shade- Since when staying on a shared host is a problem for using composer? Can't you just execute "php composer.phar install" via php exec?

@Shade-
Contributor
Shade- commented Jan 4, 2017

Since most of shared hostings do not allow SSH access to perform command line activities.

@annuh
Contributor
annuh commented Jan 4, 2017

@Shade- You can also run composer install on your local machine and transfer all files together via ie. FTP

@Shade-
Contributor
Shade- commented Jan 4, 2017

This is not possible because my shared hosting does not meet the necessary requirements. Many hostings are still stuck to older PHP versions unfortunately.

@tobeorla
Contributor
tobeorla commented Jan 5, 2017 edited

@Shade- php exec does not require ssh access : S (http://php.net/manual/en/function.exec.php)
And "Many hostings are still stuck to older PHP versions unfortunately." that's wrong. Check the latest stats:
https://seld.be/notes/php-versions-stats-2016-2-edition
PHP 5.5.9 11.87% PHP 5.6 39.67%
PHP 7.0.6 10.39% PHP 5.5 29.56%
PHP 5.6.20 8.41% PHP 7.0 20.24%
PHP 5.6.21 7.69% PHP 5.4 7.64%
PHP 5.6.19 4.71% PHP 5.3 2.43%

PHP requirements in Packages present on packagist
5.2 2.35% (-0.16)
5.3 41.25% (-4.01)
5.4 30.12% (-1.57)
5.5 16.98% (+1.5)
5.6 6.22% (+2.7)
7.0 3.08% (+1.54)

Composer requirement: PHP 5.3 ...
And it's quite easy, just switch to a better host. ovh.com , online.net, digitcalocean.com, etc..
Composer is the present, not the future.

@Shade-
Contributor
Shade- commented Jan 5, 2017

I could show you other stats in which it's clearly stated that the average PHP version installed on shared hostings is far from being up to date, but that won't contribute positively to the discussion. Also, my hosting works well for my needs BUT does not allow me to install Composer in any possible way, apart from the outdated PHP version. I don't need to change host for this.

My point here is simple: this PR updated the project with a module (so something that IS NOT central to the project itself: it's just one of many other modules) that 1) shifts up the environment requirements of the whole package and 2) needs specific and external projects such as Composer to be installed looks like a complete fail to me. A more reasonable approach would have been updating the whole library to use modern tools offered by more up to date PHP versions and eventually integrate the use of Composer for the entire package. And that's what has already been in development with the 3.0 remake, so I would have accepted this only for the next major leap forward, not a minor update which worked well even without this addition. If it works, why would you touch it?

I'm not against using the latest features and softwares, I'm quite happy to learn new techniques and upgrade my skills year by year. But I always look for the balance between pros and cons and this PR looks bad to me for the aforementioned reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment