PHP Notice during login [#2326615]

Comment	File	Size	Author
#13	simple_fb_connect-fix_for_php_notice-2326615-13.patch	664 bytes	GiorgosK

#4	simple_fb_connect-fix_for_php_notice-2326615-4.patch	652 bytes	kutergin.viktor

#1	simple_fb_connect-fix_for_php_notice-2326615-1.patch	770 bytes	masipila

Comment #1

masipila CreditAttribution: masipila commented 23 August 2014 at 16:04

Status:

Active

» Needs review

File	Size
simple_fb_connect-fix_for_php_notice-2326615-1.patch	770 bytes

Patch attached, please review and test.

Log in or register to post comments

Comment #2

masipila CreditAttribution: masipila commented 24 August 2014 at 09:48

Assigned:	masipila	» Unassigned
Status:	Needs review	» Needs work

Hmm... It seems that we are still getting the notices so the patch does not fix that... Any ideas what causes the notice?

Log in or register to post comments

Comment #3

masipila CreditAttribution: masipila commented 24 August 2014 at 09:53

Additional information: the session_destroy() and drupal_session_start() in this patch is causing subsequent graph api calls to fail. We definitely should not commit this change.

Log in or register to post comments

Comment #4

kutergin.viktor CreditAttribution: kutergin.viktor commented 8 October 2014 at 07:08

File	Size
simple_fb_connect-fix_for_php_notice-2326615-4.patch	652 bytes

1 file was hidden/shown/deleted

File	Size
simple_fb_connect-fix_for_php_notice-2326615-1.patch	770 bytes

You can try this patch.

Log in or register to post comments

Comment #5

masipila CreditAttribution: masipila commented 12 October 2014 at 08:52

Status:

Needs work

» Reviewed & tested by the community

Succesfully tested patch #4. Thank you!

Log in or register to post comments

Comment #8

saitanay CreditAttribution: saitanay commented 10 November 2014 at 03:10

Status:

Reviewed & tested by the community

» Needs work

The patch causes the login to go into a redirection loop.

with the patch on

$facebook = facebook_client();
  $fb_user = $facebook->getUser();

returns a 0, causing the module to think FB auth hasnt happened and redirects the user to FB again and hence causing the loop.

Log in or register to post comments

Comment #10

masipila CreditAttribution: masipila commented 29 November 2014 at 16:42

I'm not able to reproduce the login redirect loop with patch #4.

Here are my latest experiments from today:

I installed the latest 7.x-2.x from Git
Performed login, got the session already startedPHP notice as expected
Applied patch #4
Performed login, did not get the PHP notice anymore
Tested that $facebook->getUser() returns my FB user id succesfully with the code below

Here's how I tested that $facebook->getUser() returns expected value. I have a block. The following part is returning block content.

  $facebook = facebook_client();
  $fb_user = $facebook->getUser();

  if (!$fb_user) {
    return "something is wrong";
  }
  else {
    return $fb_user;
  }

When I viewed the block, the code above returned my FB user id.

Conclusion, patch #4 seems to work perfectly for me.

@saitanay, it looks like you have committed this to 7.x-1.x. It might well cause a redirect loop over there because the patch is written against 7.x.-2.x which is very much different from 7.x.-1.x. Please test patch #4 against the lates 7.x.-2.x-dev once more and if you get the same results as I did, please commit to 7.x-2.x-dev

Cheers,
Markus

Log in or register to post comments

Comment #11

masipila CreditAttribution: masipila commented 29 November 2014 at 16:42

Status:

Needs work

» Reviewed & tested by the community

Log in or register to post comments

Comment #12

masipila CreditAttribution: masipila commented 29 November 2014 at 16:52

And just to add: I'm not planning to participate in debugging the 7.x-1.x branch since I find the login function of 7.x-1.x impossible to read. That's why I did refactored the login function to the 7.x.-2.x. in the first place :)

What I suggest as next steps is that
1) we get this PHP notice issue nailed --> please test this once more
2) we get #2326621: Add configuration option: use username or real name tested --> this has been waiting for testing by community for a while now
3) then release a 7.x-2.0-beta1 for wider community testing
4) if no new bugs are found from the beta, then we release 7.x-2.0 and phase out 7.x-1.x branch

D7 development can then continue from the 7.x-2.x

Cheers,
Markus

Log in or register to post comments

Comment #13

GiorgosK

Chios, Greece 🇬🇷

CreditAttribution: GiorgosK commented 10 January 2015 at 18:19

File	Size
simple_fb_connect-fix_for_php_notice-2326615-13.patch	664 bytes

Still active on 1.x branch
I don't know if the maintainer has plans to move to 2.x branch but 1.15 has to be patched
here is a patch basically patch same as #4 but line numbers have changed

the patch is actually trivial and since its been tested from other users it is still review and tested and ready to commit

Log in or register to post comments

Comment #14

GiorgosK

Chios, Greece 🇬🇷

CreditAttribution: GiorgosK commented 10 January 2015 at 18:20

Version:

7.x-2.x-dev

» 7.x-1.15

Log in or register to post comments

Comment #15

3 July 2015 at 15:34

saitanay committed 9a2c107 on 8.x-2.x

Issue #2326615 by kutergin.viktor, masipila: Fixed PHP Notice during...

saitanay committed 9e50962 on 8.x-2.x

Issue #2326615 by kutergin.viktor, masipila: Fixed PHP Notice during...

Log in or register to post comments

Comment #16

masipila CreditAttribution: masipila as a volunteer commented 1 January 2016 at 20:09

Status:

Reviewed & tested by the community

» Closed (fixed)

Cleaning up the issue queue from the old 7.x-1.x issues. I'm not sure if this issue was eventually fixed in 7.x-1.x but this issue is not present in 7.x-2.x.