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

Test on PHP 8.2 #607

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Test on PHP 8.2 #607

merged 3 commits into from
Nov 8, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Oct 28, 2022

I've had to implement an attribute map on the abstract user entity to make sure dynamic property handling still worked as before.

@driesvints driesvints marked this pull request as draft October 28, 2022 13:13
@driesvints driesvints marked this pull request as ready for review November 8, 2022 13:07
@taylorotwell taylorotwell merged commit 1cd1682 into 5.x Nov 8, 2022
@taylorotwell taylorotwell deleted the driesvints-patch-1 branch November 8, 2022 15:07
@sebastiaanluca
Copy link

sebastiaanluca commented Nov 10, 2022

Sorry I'm always the bringer of bad news 😄

Coming from v5.5.5, caught a bug in our implementation of Socialite where token_secret would now be null if you get it via the object_get() helper:

-  'token_secret' => 'abc',
+  'token_secret' => NULL,

My findings:

// abc
$socialiteUser->tokenSecret,

// abc
data_get($socialiteUser, 'tokenSecret'),

// null
object_get($socialiteUser, 'tokenSecret'),

// ZOt24fxKTmkxYRBnUOgui8BYdg6TzsUn7deYgByZMJmAqMN9qTswRRiSVeCk
$socialiteUser->token,

// ZOt24fxKTmkxYRBnUOgui8BYdg6TzsUn7deYgByZMJmAqMN9qTswRRiSVeCk
data_get($socialiteUser, 'token'),

// ZOt24fxKTmkxYRBnUOgui8BYdg6TzsUn7deYgByZMJmAqMN9qTswRRiSVeCk
object_get($socialiteUser, 'token'),

The weird thing here is it only fails for tokenSecret 🤨 Both are only defined in \Laravel\Socialite\One\User (not the parent AbstractUser) and are set in the same way. Have no idea at the moment what the difference is and why one returns null.


I always find the issue 5 minutes after posting it 😅

Seems to be primarily a test issue. Was creating an OAuth2 user manually, but also passing a tokenSecret which that class doesn't have. Since the change in this PR adds dynamically getting the property and object_get() does an isset($object->{$segment}) check, it returns the default of null.

So the takeaway here would be isset() + __get() doesn't work —as isset() checks the actual defined property on the class? https://www.php.net/manual/en/function.isset.php#51113

$attributes = [
    'id' => '8a1e57jQiC',
    'email' => 'jesse@smarthealth.works',
    'name' => 'Jesse Green',
    'avatar' => 'https://www.example.com/image.png',

    'token' => 'ZOt24fxKTmkxYRBnUOgui8BYdg6TzsUn7deYgByZMJmAqMN9qTswRRiSVeCk',
    'refreshToken' => 'JAQEqccZqfljlrQ1kRkl1GfoSYH3B0hayXCMYCBcFNjQ3ZGdEOouluI2geTH',
    'tokenSecret' => 'abc',
    'expiresIn' => 6547897,
];

return (new \Laravel\Socialite\Two\User)
    ->setRaw($attributes)
    ->map($attributes);

@driesvints
Copy link
Member Author

@sebastiaanluca heya, yes I suppose that's correct. Sorry you got bit here (again). But seems you're good and we don't need to rollback?

@sebastiaanluca
Copy link

@driesvints I think you're good. The OAuth1 user has the tokenSecret property, so it should be accessible via object_get(). It's a real edge case 😄

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

3 participants