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

feat: add SQLite3 config synchronous #9202

Open
wants to merge 2 commits into
base: 4.6
Choose a base branch
from

Conversation

michalsn
Copy link
Member

Description
This PR adds support for the synchronous setting in SQLite3. It's not persistent across connections, which means it resets to the default (usually 2) with every new connection. We must set synchronous 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:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added enhancement PRs that improve existing functionalities 4.6 labels Sep 25, 2024
@@ -70,6 +79,10 @@ public function initialize()
if (is_int($this->busyTimeout)) {
$this->connID->busyTimeout($this->busyTimeout);
}

if (is_int($this->synchronous)) {
Copy link
Contributor

@maniaba maniaba Oct 6, 2024

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.

Copy link
Member Author

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).

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants