-
-
Notifications
You must be signed in to change notification settings - Fork 424
Add snowflake pdo driver #1218
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
base: master
Are you sure you want to change the base?
Add snowflake pdo driver #1218
Conversation
install-php-extensions
Outdated
| fi | ||
| ;; | ||
| pdo_snowflake) | ||
| if test $PHP_MAJMIN_VERSION -lt 801; then |
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'd avoid this check: we don't do that for all the other extensions
install-php-extensions
Outdated
| printf 'pdo_snowflake requires PHP 8.1+\n' >&2 | ||
| exit 1 | ||
| fi | ||
| if ! command -v cmake >/dev/null 2>&1; then |
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'd avoid this check: we don't do that for all the other extensions, and the cmake is installed thanks to the code in the buildRequiredPackageLists function.
install-php-extensions
Outdated
| installRemoteModule_tmp1="$(cmake --version | head -1 | grep -oE '[0-9]+\.[0-9]+' | head -1)" | ||
| installRemoteModule_tmp2="$(echo "$installRemoteModule_tmp1" | cut -d. -f1)" | ||
| installRemoteModule_tmp3="$(echo "$installRemoteModule_tmp1" | cut -d. -f2)" | ||
| if test "$installRemoteModule_tmp2" -lt 3 || { test "$installRemoteModule_tmp2" -eq 3 && test "$installRemoteModule_tmp3" -lt 17; }; then |
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'd avoid this check: we don't do that for all the other extensions
install-php-extensions
Outdated
| printf 'cmake >= 3.17 required (detected: %s)\n' "$installRemoteModule_tmp1" >&2 | ||
| exit 1 | ||
| fi | ||
| if ! command -v gcc >/dev/null 2>&1; then |
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'd avoid this check: we don't do that for all the other extensions, and gcc is installed thanks to the code in the buildRequiredPackageLists function.
install-php-extensions
Outdated
| exit 1 | ||
| fi | ||
| fi | ||
| installRemoteModule_tmp="$(getPackageSource "https://github.com/snowflakedb/pdo_snowflake/archive/refs/tags/v${installRemoteModule_version}.tar.gz")" |
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.
Do we really need this check?
install-php-extensions
Outdated
| exit 1 | ||
| fi | ||
| cd "$installRemoteModule_tmp" | ||
| export PHP_HOME="$(php-config --prefix)" |
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.
Is the PHP_HOME variable required by build_pdo_snowflake.sh? If so, I'd simply write
PHP_HOME="$(php-config --prefix)" ./scripts/build_pdo_snowflake.sh
install-php-extensions
Outdated
| rm -rf "$installRemoteModule_tmp" | ||
| exit 1 | ||
| fi | ||
| if ! test -f modules/pdo_snowflake.so; then |
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'd avoid this check: the following cp will fail (and the script will abort) if that .so file doesn't exist
ef2fe5f to
33e4ca6
Compare
|
Could you check the errors reported by CI? PS: no need to force-push changes (it makes review harder): when I'll merge this PR I'll squash the commits. |
Summary
This PR adds support for the Snowflake PDO driver (
pdo_snowflake) extension, enabling PHP applications to connect to Snowflake databases using PDO.Limitations
Alpine Linux Not Supported
Snowflake's PDO driver currently does not support Alpine Linux (musl libc). This is a known upstream limitation tracked in snowflakedb/pdo_snowflake#319.
Users requiring Alpine compatibility should use Debian-based images instead:
php:8.3-alpinephp:8.3-cliorphp:8.3-fpmPHP 8.5 Not Supported
PHP 8.5 introduces PDO API changes that pdo_snowflake has not yet adapted to. Support will be added when upstream releases a compatible version.