Project

General

Profile

Bug #7605

Facebook Changed api

Added by Konrad Banasiak about 6 years ago. Updated almost 6 years ago.

Status:
Assigned
Priority:
Normal
Category:
Plugins
Target version:
Start date:
01/04/2015
Due date:
% Done:

50%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

Facebeook changed their api, So the integration with facebook doesn't work on version 1.9.10.x
I has prepared the new version of integration with facebook.
To merge the source code with chamilo please.
Replace all code from attached zip file.
Make a following changes in file ./index.php:
  • add the line: "require_once 'main/auth/external_login/facebook.inc.php';" after lines requires_one...
  • move up the following lines:
    "// Facebook connexion, if activated
    if (api_is_facebook_auth_activated() && !api_get_user_id()) {
    facebook_connect();
    }"
    before line:
    "$controller = new IndexManager($header_title);"
To run the new integration please:
  • uncomment line: " $_configuration['facebook_auth'] = 1;" in the configuration.php file
  • uncoment line:
    "$facebook_config = array( 'appId' => 'APPID',
    'secret' => 'SECRET',
    'return_url' => api_get_path(WEB_PATH).'?action=fbconnect'
    );" in the file auth.conf.php and update APPID and SECRET
  • turn on the plugin: add_facebook_login_button
  • add plugin add_facebook_login_button to region login_top or login_bottom

Regards

Konrad Banasiak


Files

facebook.zip (66.6 KB) facebook.zip Source Code Konrad Banasiak, 01/04/2015 12:57

Associated revisions

Revision c29e8e2d (diff)
Added by Yannick Warnier almost 6 years ago

Add missing file from previous commit - refs #7605

Revision 4c62cb08
Added by Yannick Warnier almost 6 years ago

Fixed code style again because of non-updated PR, then move the facebook.inc.php inclusion to avoid NOTICE alerts - refs #7605

Revision 61cc4aee (diff)
Added by Yannick Warnier almost 6 years ago

Fixed code style again because of non-updated PR, then move the facebook.inc.php inclusion to avoid NOTICE alerts - refs #7605

Revision 12880672 (diff)
Added by Yannick Warnier almost 6 years ago

Fixed code style again because of non-updated PR, then move the facebook.inc.php inclusion to avoid NOTICE alerts - refs #7605

Revision 78453589 (diff)
Added by Yannick Warnier almost 6 years ago

Fix auth config file path in require - refs #7605

History

#1

Updated by Yannick Warnier about 6 years ago

  • Category set to Plugins
  • Target version set to 1.10.0
  • % Done changed from 0 to 20
#2

Updated by Yannick Warnier about 6 years ago

  • Status changed from New to Needs testing
  • Assignee set to Julio Montoya
#3

Updated by Yannick Warnier about 6 years ago

  • Description updated (diff)
#4

Updated by Julio Montoya about 6 years ago

  • Assignee changed from Julio Montoya to Yannick Warnier

I'm not sure if we should keep maintaing this code in 1.10.
We should use an external library as in version 2.0 such as:

https://github.com/hwi/HWIOAuthBundle

#5

Updated by Julio Montoya about 6 years ago

  • Status changed from Needs testing to Needs more info
#6

Updated by Yannick Warnier about 6 years ago

  • Assignee changed from Yannick Warnier to Julio Montoya

Julio, how much time would it take you to transfer it to OAuth? Comment on the side: I don't think Facebook uses OAuth.

To me it is only a question of time. We don't have that much time on our hands for 1.10.0, so I don't want to include something that will take 20h off our schedule.

#7

Updated by Julio Montoya about 6 years ago

  • Assignee changed from Julio Montoya to Yannick Warnier

Yannick Warnier wrote:

Julio, how much time would it take you to transfer it to OAuth? Comment on the side: I don't think Facebook uses OAuth.

To me it is only a question of time. We don't have that much time on our hands for 1.10.0, so I don't want to include something that will take 20h off our schedule.

A lot of time.

But this is a plugin. It was develoved by Hubert. He shouldn't make it available for 1.10 or at least review this fix?

#8

Updated by Yannick Warnier about 6 years ago

  • Assignee changed from Yannick Warnier to Hubert Borderiou

Good point. Let's assign the review of the current fix to Hubert...

#9

Updated by Hubert Borderiou about 6 years ago

Hi Konrad, I've installed it on my 1.9.10.2 has you said and it works great.
I've just removed the 'publish_stream' datas from Facebook to be sent to Chamilo in file
main/auth/external_login/facebook.inc.php

function facebook_get_login_url() {

global $facebook_config;
$helper = new FacebookRedirectLoginHelper($facebook_config['return_url']);
$login_url = $helper->getLoginUrl(array(
// 'scope' => 'email,publish_stream' ));
'scope' => 'email));
return $login_url;
}

#10

Updated by Hubert Borderiou about 6 years ago

I've tested it, and created the pull request
https://github.com/chamilo/chamilo-lms/pull/727

#11

Updated by Hubert Borderiou almost 6 years ago

  • Status changed from Needs more info to Needs testing
#12

Updated by Hubert Borderiou almost 6 years ago

Fait pour la 1.9.10.2

#13

Updated by Yannick Warnier almost 6 years ago

  • Status changed from Needs testing to Bug resolved
  • % Done changed from 20 to 100

OK, and I integrated the same changes into 1.10.x, so I'll close the task. Thanks Konrad for the patch. Thanks Hubert for the integration.

#14

Updated by Konrad Banasiak almost 6 years ago

You welcome.

#15

Updated by Yannick Warnier almost 6 years ago

  • Status changed from Bug resolved to Assigned
Re-opening because there are still several things that do not work (for Hubert's PR):
  • the Facebook check in local.inc.php should be similar to the CAS case: a check on some external login method defined in configuration.php and then a specific case for that. Putting it on top of local.inc.php puts the priority on Facebook, which is not desired. Check lines ~230 where a check is made on CAS (given you're going to work on CAS, this should be easy)
  • when developing in Chamilo, you should always enable the E_NOTICE error level so you're aware when something is wrong. As such, for example, I had to move the inclusion of facebook.inc.php inside the first check for facebook (lines ~124 of local.inc.php), otherwise I received a warning
  • when sending a new PR, please make sure you first pull the changes from 1.9.x branch, otherwise I have to re-apply my code-style patches every time you send a PR
#16

Updated by Yannick Warnier almost 6 years ago

  • % Done changed from 100 to 90

I fixed the NOTICE messages and the code style. The only remaining issue is to move the api_is_facebook_auth_activated() block to a more sensible place in local.inc.php

#17

Updated by Yannick Warnier almost 6 years ago

Will be moved to the next version in 5 days if the observation on api_is_facebook_auth_activated() is not attended.

#18

Updated by Yannick Warnier almost 6 years ago

Additionally, the forced inclusion of the new Facebook API makes Chamilo 1.9 fail with PHP 5.3 because the SDK requires PHP 5.4. So the inclusion has to check for that as well.

#19

Updated by Yannick Warnier almost 6 years ago

  • % Done changed from 90 to 50

I'm moving this update to 2.0

#20

Updated by Yannick Warnier almost 6 years ago

  • Target version changed from 1.10.0 to 2.0

Also available in: Atom PDF