Skip to content

feat: support other databases #402

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.

Sign up for GitHub

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

Merged
datamweb merged 4 commits into from
Aug 30, 2022
Merged

feat: support other databases #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7498e47
fix: for OCI8 driver
kenjis Aug 25, 2022
e9a2336
fix: set property cast for AccessToken::$id
kenjis Aug 25, 2022
4bd544b
fix: set property cast for UserIdentity::$id
kenjis Aug 25, 2022
75c37d0
fix: add hack to insert error with OCI8
kenjis Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Entities/AccessToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class AccessToken extends Entity
* @var array<string, string>
*/
protected $casts = [
'id' => '?integer',
'last_used_at' => 'datetime',
'extra' => 'array',
'expires' => 'datetime',
Expand Down
1 change: 1 addition & 0 deletions src/Entities/UserIdentity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class UserIdentity extends Entity
* @var array<string, string>
*/
protected $casts = [
'id' => '?integer',
'force_reset' => 'int_bool',
];

Expand Down
6 changes: 6 additions & 0 deletions src/Models/LoginModel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ public function recordLoginAttempt(
?string $userAgent = null,
$userId = null
): void {
$this->disableDBDebug();

if ($this->db->getPlatform() === 'OCI8' && $identifier === '') {
$identifier = ' ';
Copy link
Copy Markdown
Member

@MGatner MGatner Aug 25, 2022

Choose a reason for hiding this comment

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

... and this?

Copy link
Copy Markdown
Member

@MGatner MGatner Aug 25, 2022

Choose a reason for hiding this comment

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

Ah I see the whole commit message now, that explains this one. Seems like this is probably a bug in the framework driver itself, right?

Copy link
Copy Markdown
Member Author

@kenjis kenjis Aug 25, 2022

Choose a reason for hiding this comment

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

I'm not sure.
Oracle sees '' as NULL.

Copy link
Copy Markdown
Collaborator

@datamweb datamweb Aug 25, 2022

Choose a reason for hiding this comment

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

Oracle Database currently treats a character value with a length of zero as null. However, this may not continue to be true in future releases, and Oracle recommends that you do not treat empty strings the same as nulls.

More see.

Copy link
Copy Markdown
Member Author

@kenjis kenjis Aug 25, 2022

Choose a reason for hiding this comment

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

@datamweb I knew it, but I don't know what should we do in this case.
We try to insert '', but Oracle treats it as NULL and says "cannot insert NULL"

Copy link
Copy Markdown
Collaborator

@datamweb datamweb Aug 25, 2022

Choose a reason for hiding this comment

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

@kenjis I'm not sure if it's helpful, but take a look if you have time.
https://stackoverflow.com/questions/2185743/is-it-possible-to-store-empty-string-as-a-non-null-value-in-the-database/73396113#73396113

Copy link
Copy Markdown
Member Author

@kenjis kenjis Aug 26, 2022

Choose a reason for hiding this comment

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

Use an alternate representation, such as a single space character, for an empty string.

It is what I did here.

Copy link
Copy Markdown
Member

@MGatner MGatner Aug 26, 2022

Choose a reason for hiding this comment

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

@ytetsuro have any recommendations? Or @sclubricants?

}

$return = $this->insert([
'ip_address' => $ipAddress,
'user_agent' => $userAgent,
Expand Down
6 changes: 2 additions & 4 deletions src/Models/UserModel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,18 +169,16 @@ public function findByCredentials(array $credentials): ?User
$email = $credentials['email'] ?? null;
unset($credentials['email']);

$prefix = $this->db->DBPrefix;

// any of the credentials used should be case-insensitive
foreach ($credentials as $key => $value) {
$this->where("LOWER({$prefix}users.{$key})", strtolower($value));
$this->where('LOWER(' . $this->db->protectIdentifiers("users.{$key}") . ')', strtolower($value));
}

if (! empty($email)) {
$data = $this->select('users.*, auth_identities.secret as email, auth_identities.secret2 as password_hash')
->join('auth_identities', 'auth_identities.user_id = users.id')
->where('auth_identities.type', Session::ID_TYPE_EMAIL_PASSWORD)
->where("LOWER({$prefix}auth_identities.secret)", strtolower($email))
->where('LOWER(' . $this->db->protectIdentifiers('auth_identities.secret') . ')', strtolower($email))
->asArray()
->first();

Expand Down