Implement #72653: SQLite should allow opening with empty filename#2030
Implement #72653: SQLite should allow opening with empty filename#2030cmb69 wants to merge 4 commits intophp:masterfrom
Conversation
From the [sqlite3_open](https://www.sqlite.org/c3ref/open.html) docs: | If the filename is an empty string, then a private, temporary on-disk | database will be created. This private database will be automatically | deleted as soon as the database connection is closed. We make that facility available to userland.
|
The failing checks are unrelated to this PR. |
ext/sqlite3/sqlite3.c
Outdated
| } | ||
| if (filename_len != sizeof(":memory:")-1 || | ||
| if (filename_len == 0) { | ||
| fullpath = emalloc(1); |
There was a problem hiding this comment.
Why not fullpath = NULL here ?
Would work, and prevent a one byte allocation.
There was a problem hiding this comment.
I agree that the emalloc(1) is silly, but I'm not sure that fullpath = NULL would work (it does now, but that might be an implementation detail). The SQLite3 docs say "empty filename".
What about using a stack allocated empty string instead?
There was a problem hiding this comment.
Why not, but you'll have to track that to be sure not to free it.
Otherwise : change the whole code to use a stack allocated buffer instead of a heap allocated one ;-)
|
Yep, OK to merge that in 5.6 and 7.0 once comments are addressed |
|
The issue with emalloc(1) should be solved now. I can "merge" later to PHP 5.6+. |
|
Great 👍 |
|
Imho empty_string is an unnecessary microoptimization, it just makes the code dirty. |
|
If we're going down that route lets make it generic and cover :memory: as well, by setting fullpath = filename and checking for that. |
Okay, so we either want only the first or all three commits. |
ext/sqlite3/sqlite3.c
Outdated
| @@ -117,8 +117,8 @@ PHP_METHOD(sqlite3, open) | |||
| if (strlen(filename) != filename_len) { | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
Can't this if be removed as were ZPP'ing for p (not s) anyway?
There was a problem hiding this comment.
I would say yes, it can be removed.
Also, why not use a "P" , to be given a zend_string ? Just a small refactor, zend_string should really be prefered over C strings as much as possible.
The comparaisons would then use zend_string_equals_literal().
I can patch it if you want
There was a problem hiding this comment.
Also, why not use a "P" , to be given a zend_string ?
AIUI, that would be PHP 7 only.
I can patch it if you want.
Yes, go for it. :-)
There was a problem hiding this comment.
Separate PR then, as it will obviously be PHP 7 only ;-)
There was a problem hiding this comment.
Okay, I'll commit this one first to PHP 5.6+.
| @@ -114,11 +114,8 @@ PHP_METHOD(sqlite3, open) | |||
| zend_throw_exception(zend_ce_exception, "Already initialised DB Object", 0); | |||
There was a problem hiding this comment.
It's unrelated to this PR, but there's a return missing here.
There was a problem hiding this comment.
Good catch. I'll fix it as well.
|
Comment on behalf of cmb at php.net: "Merged" with cc125f2. |
5 participants
From the sqlite3_open docs:
| If the filename is an empty string, then a private, temporary on-disk
| database will be created. This private database will be automatically
| deleted as soon as the database connection is closed.
We make that facility available to userland.