Don't expose the SQLite3::readOnly method with SQLite < 3.7.4 #3614
Conversation
…d of returning false, as the underlying function in SQLite3 library is not available
|
Comment on behalf of petk at php.net: Labelling... Thank you for the pull request @bohwaz |
|
Thanks! In my opinion, a reasonable improvement. Of course, there is a potential BC break, but since SQLite3 3.7.4 has been released on 2010-12-08, it may not really affect anybody. |
|
Taking into account that this method is existing since PHP 5.3.7, I think it would be better to stick with the current behavior. The method is documented as
which means that it provides a conservative return value. As such, always returning false is a valid (if imprecise) implementation of this method. Alternatively, we could bump the sqlite version again. sqlite 3.7.4 would be in Ubuntu 12.04 and RHEL 7. |
Would be fine for me, considering that PHP 7.4 is likely to be released roughly nine years after SQLite 3.7.4. |
That would be a good solution too. As for the patch, it was done after a suggestion from @cmb69 on internals, and I find that having false returned when the feature is actually not available is not correct either. |
I agree, especially since the docs are not correct (sqlite3_stmt_readonly() does not regard user defined functions). |
|
So, what's the state here? Do we agree that we should raise the libsqlite3 requirement on master and not make this change? |
|
I do agree with raising the libsqlite3 requirement and ditch my PR. But a similar issue might be raised with other features such as #3669 where I raise an error when the libsqlite3 is too old, but I'm not sure if it's the best practice either. |
|
Raising the requirement to SQLite 3.7.4 as of PHP 7.4.0 still seems good to me. The currently proposed behavior for |
|
I think raising an exception for unavailable functionality is fine generally (not much else you can do...) My concern here was just the BC break, which doesn't exist for new functions :) |
`SQLite3::readOnly()` uses `sqlite3_stmt_readonly()` which is only available as of libsqlite 3.7.4. For older SQLite3 versions we return always `false`, which can be confusing. Instead of sticking with this behavior, or even undefining the method for old SQLite3 versions, we lift the requirements to SQLite 3.7.4 (released on 2010-12-08), according to a respective discussion[1]. Since pdo_sqlite doesn't use `sqlite3_stmt_readonly()`, we stick with the minimum requirement of SQLite 3.5.0. [1] <#3614>
|
I've bumped the ext/sqlite3 requirements now (a757ebb), so this PR is obsolete. |
Instead of returning false when the SQLite3 library is too old, just don't expose the method as it doesn't make any sense as the feature is just not present.