-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: add SQLite3 config synchronous #9202
base: 4.6
Are you sure you want to change the base?
Conversation
a800f5c
to
3af4da8
Compare
@@ -70,6 +79,10 @@ public function initialize() | |||
if (is_int($this->busyTimeout)) { | |||
$this->connID->busyTimeout($this->busyTimeout); | |||
} | |||
|
|||
if (is_int($this->synchronous)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a good idea to add validation or throw a database exception in this section. Since we are directly passing $this->synchronous
into the PRAGMA statement, without validating whether the value is one of the expected integers (0, 1, 2, 3)
, we risk unexpected behavior or incorrect PRAGMA settings if an invalid value is provided. Adding validation would ensure we are handling only valid inputs and would prevent potential silent failures.
Additionally, it might be worth considering the use of an enum to define the valid values for synchronous. This would provide a clearer and more structured way to enforce the allowed options and make the code easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added synchronous value validation and test.
I think using an enum here would mess with simplicity a bit - but if more people say we should go this way I can add it. Although I don't think we have a single enum in the project at the moment (I might have missed something).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using an enum is necessary in this case. The reason enums weren't used in the framework was due to older PHP versions, but I believe we should use enums where possible.
In general, I'm a fan of enums as they improve code readability, but I don't see a strong need for them here.
Description
This PR adds support for the
synchronous
setting in SQLite3. It's not persistent across connections, which means it resets to the default (usually2
) with every new connection. We must setsynchronous
on each connection if we want it to differ from the default.More info about this setting: https://www.sqlite.org/pragma.html#pragma_synchronous
Checklist: