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

Disable sqlite3 and pdo_sqlite by default #3610

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Oct 16, 2018

Since libsqlite has been unbundled[1], it makes sense to change the
defaults of building ext/sqlite3 and ext/pdo_sqlite, i.e. to disable
both extensions by default – this is more in line with other bundled
extension which rely on external dependencies.

Nothing has to be done for Windows, since both extensions are disabled
by default already.

[1] http://git.php.net/?p=php-src.git;a=commit;h=6083a387a81dbbd66d6316a3a12a63f06d5f7109

Since libsqlite has been unbundled[1], it makes sense to change the
defaults of building ext/sqlite3 and ext/pdo_sqlite, i.e. to disable
both extensions by default – this is more in line with other bundled
extension which rely on external dependencies.

Nothing has to be done for Windows, since both extensions are disabled
by default already.

[1] <http://git.php.net/?p=php-src.git;a=commit;h=6083a387a81dbbd66d6316a3a12a63f06d5f7109>
@cmb69
Copy link
Contributor Author

cmb69 commented Oct 16, 2018

This issue has been pointed out by @sgolemon. Thanks!

I'm not sure whether the Travis configuration would have to be adjusted to still build the extensions and run the respective tests.

@Majkl578
Copy link
Contributor

This sort of breaks the assumptions that SQLite is always available by default. A lot of projects use SQLite as a fallback/default database backend.

Also according to docs:

The SQLite3 extension is enabled by default as of PHP 5.3.0. It's possible to disable it by using --without-sqlite3 at compile time. [1]

The PDO_SQLITE PDO driver is enabled by default. [2]

[1] http://php.net/manual/en/sqlite3.installation.php
[2] http://php.net/pdo_sqlite

@cmb69
Copy link
Contributor Author

cmb69 commented Oct 16, 2018

@Majkl578 I'm not totally happy about this change, but what if there's no libsqlite3 available on the system?

Copy link
Contributor

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

This doesn't look like a good idea: yes, SQLite may not exist on the target system, in which case the build will fail, and that's OK.

@Majkl578
Copy link
Contributor

but what if there's no libsqlite3 available on the system

That's probably something that should've been considered before unbundling SQLite, but not as a reason to disable it by default.

SQLite may not exist on the target system, in which case the build will fail, and that's OK.

Yes, and in this case they can still use --without-sqlite3/ --without-pdo-sqlite. 👍

@nikic
Copy link
Member

nikic commented Oct 16, 2018

IIRC we also require libxml2 in a default build, so there's precedent here.

On the other hand, IMHO you should not draw any conclusions of availability based on the "default" build. Unless it's part of --disable-all builds, the extension is optional and you can't rely on it being available. Distros are going to split the extension off anyway (php7.2-sqlite3), so de facto it's non-default anyway.

@cmb69
Copy link
Contributor Author

cmb69 commented Oct 16, 2018

IIRC we also require libxml2 in a default build, so there's precedent here.

Indeed. Thanks! So I tend to withdraw this PR, but I'll keep it open for while – maybe there are still good reasons to change the defaults.

@cmb69
Copy link
Contributor Author

cmb69 commented Oct 21, 2018

Okay, no further input – closing.

@cmb69 cmb69 closed this Oct 21, 2018
@cmb69 cmb69 deleted the disable-sqlite branch October 21, 2018 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants