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

add postgres support #32

Closed
wants to merge 1 commit into from
Closed

Conversation

Mic92
Copy link
Contributor

@Mic92 Mic92 commented Sep 28, 2023

No description provided.

Copy link
Owner

@mohe2015 mohe2015 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has been a long time since I last looked at this so only clarification questions in case you know the answer.

amoa_provider VARCHAR(255) NOT NULL,

-- the local user id
amoa_local_user INTEGER NOT NULL,
Copy link
Owner

@mohe2015 mohe2015 Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could only identify this to have changed from INTEGER UNSIGNED to INTEGER. Do you by chance know why it was INTEGER UNSIGNED for mysql and why we could not simply make it INTEGER for both? I would like to keep this as simple as possible.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I fixed this with

amoa_local_user BIGINT NOT NULL,

@@ -240,7 +240,7 @@ public function continuePrimaryAuthentication( array $reqs ) {
$result = $dbr->select(
'authmanageroauth_linked_accounts',
[ 'amoa_provider', 'amoa_remote_user', 'amoa_local_user' ],
[ 'amoa_provider' => $req->amoa_provider, 'amoa_remote_user' => $resp->linkRequest->amoa_remote_user ],
[ 'amoa_provider' => $req->amoa_provider, 'amoa_remote_user' => strval($resp->linkRequest->amoa_remote_user) ],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For which cases does this make a difference? I currently don't have the big picture in my head.

Copy link
Owner

@mohe2015 mohe2015 Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested myself, seems like amoa_remote_user can be an integer if the identity provider provides it in that format.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I fixed this with

@mohe2015
Copy link
Owner

Thanks for providing pointers at what needed fixing. As explained in my comments I think I fixed this in my yearly cleanup 4dfbb9d (with a few different changes that hopefully lead to the same result).

Also sorry for taking so long, I missed the notification. Feel free to ping or request review in that case next time.

While my changes in the end are quite different to yours I should've probably still mentioned you somewhere in the fix, sorry.

@mohe2015 mohe2015 closed this Oct 24, 2023
@Mic92 Mic92 deleted the postgres-support branch January 15, 2024 14:14
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.

2 participants