[RFC VOTE] Unbundle ext/interbase

Evening Internals

Apologies for the one day delay, but the voting for the RFC "Unbundle
ext/interbase" is open[1]. There is only one voting choice, which
decides if the extension should be moved to PECL.

Since this started a day later than anticipated, then the voting will
naturally run for one extra day and therefore officially close on
tuesday, 23th of April, 2019 around noon EET.

[1] https://wiki.php.net/start#voting

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Den tir. 9. apr. 2019 kl. 20.02 skrev Kalle Sommer Nielsen kalle@php.net:

Evening Internals

Apologies for the one day delay, but the voting for the RFC "Unbundle
ext/interbase" is open[1]. There is only one voting choice, which
decides if the extension should be moved to PECL.

Since this started a day later than anticipated, then the voting will
naturally run for one extra day and therefore officially close on
tuesday, 23th of April, 2019 around noon EET.

Apologies for the wrong link, the correct one is:
https://wiki.php.net/rfc/deprecate-and-remove-ext-interbase#voting

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi Kalle,

You forgot to move to voting on the RFC index.

Cheers
Joe

Den tir. 9. apr. 2019 kl. 20.02 skrev Kalle Sommer Nielsen <kalle@php.net

:

Evening Internals

Apologies for the one day delay, but the voting for the RFC "Unbundle
ext/interbase" is open[1]. There is only one voting choice, which
decides if the extension should be moved to PECL.

Since this started a day later than anticipated, then the voting will
naturally run for one extra day and therefore officially close on
tuesday, 23th of April, 2019 around noon EET.

Apologies for the wrong link, the correct one is:
https://wiki.php.net/rfc/deprecate-and-remove-ext-interbase#voting

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi

Den tir. 9. apr. 2019 kl. 20.02 skrev Kalle Sommer Nielsen kalle@php.net:

Evening Internals

Apologies for the one day delay, but the voting for the RFC "Unbundle
ext/interbase" is open[1]. There is only one voting choice, which
decides if the extension should be moved to PECL.

Since this started a day later than anticipated, then the voting will
naturally run for one extra day and therefore officially close on
tuesday, 23th of April, 2019 around noon EET.

[1] https://wiki.php.net/start#voting

The two weeks voting period for this RFC has passed and the RFC has
been passed with 46-0 in favor. I will prepare relevant practical
tasks for this RFC throughout the week.

Thanks to everyone who voted.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi!

Den tir. 9. apr. 2019 kl. 20.02 skrev Kalle Sommer Nielsen kalle@php.net:

The two weeks voting period for this RFC has passed and the RFC has
been passed with 46-0 in favor. I will prepare relevant practical
tasks for this RFC throughout the week.

A problem is that the ext/pdo_firebird tests rely on ext/interbase to
create a test database[1], which would require everybody who intends to
run these tests to install PECL/interbase package now. It would be
great if somebody would rewrite this, so interbase would no longer be a
dependency of the pdo_firebird tests.

[1] <https://github.com/php/php-src/blob/PHP-7.4/ext/pdo_firebird/tests
/testdb.inc>

--
Christoph M. Becker

Hi Christoph

Den tor. 2. maj 2019 kl. 14.56 skrev Christoph M. Becker cmbecker69@gmx.de:

A problem is that the ext/pdo_firebird tests rely on ext/interbase to
create a test database[1], which would require everybody who intends to
run these tests to install PECL/interbase package now. It would be
great if somebody would rewrite this, so interbase would no longer be a
dependency of the pdo_firebird tests.

[1] <https://github.com/php/php-src/blob/PHP-7.4/ext/pdo_firebird/tests
/testdb.inc>

Yeah that is one thing that is kind of unfortunate and a bad practice.
However since interbase was disabled for AppVeyor, these tests had no
chance to run in a while. Looking at the actual code in testdb.inc, it
seems like it creates the database for testing purposes, I would
suppose this is due to PDO_Firebird perhaps requiring a database to
continue its connection flow in PDO itself.

I'm not sure how we can go about this, maybe a .sql file for testers
who run the test suite for schemas and then see if we can integrate
some auto importer in our CI build scripts for this special case? (In
the case we cannot go around PDO). Either way the test files requires
a manual touch to configure credentials to run.

I'm personally fine with the simple solution, thoughts?

(ps. Thanks for the fix for the AppVeyor part I missed)

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi Kalle,

Den tor. 2. maj 2019 kl. 14.56 skrev Christoph M. Becker cmbecker69@gmx.de:

A problem is that the ext/pdo_firebird tests rely on ext/interbase to
create a test database[1], which would require everybody who intends to
run these tests to install PECL/interbase package now. It would be
great if somebody would rewrite this, so interbase would no longer be a
dependency of the pdo_firebird tests.

[1] <https://github.com/php/php-src/blob/PHP-7.4/ext/pdo_firebird/tests
/testdb.inc>

Yeah that is one thing that is kind of unfortunate and a bad practice.
However since interbase was disabled for AppVeyor, these tests had no
chance to run in a while. Looking at the actual code in testdb.inc, it
seems like it creates the database for testing purposes, I would
suppose this is due to PDO_Firebird perhaps requiring a database to
continue its connection flow in PDO itself.

Indeed, it seems that pdo_firebird requires to connect to an existing
database, contrary to e.g. pdo_mysql which supports DSN like
mysql:host=localhost, to my knowledge. But even if pdo_firebird has
been connected to an existing database, it appears that it's not
possible to create a new database via PDO (I got something like "prepare
does not support CREATE DATABASE", even with emulated prepares turned on).

I'm not sure how we can go about this, maybe a .sql file for testers
who run the test suite for schemas and then see if we can integrate
some auto importer in our CI build scripts for this special case? (In
the case we cannot go around PDO). Either way the test files requires
a manual touch to configure credentials to run.

I'm personally fine with the simple solution, thoughts?

ACK. Given that these test did not run on our CI (pdo_firebird is not
even built on Travis) anyway, I think a simple solution which allows
someone who wants to run the tests after some manual setup is sufficient
as a first step. We always could improve on that later.

(ps. Thanks for the fix for the AppVeyor part I missed)

No problemo! :)

--
Christoph M. Becker

Hi Christoph

Den tor. 2. maj 2019 kl. 14.56 skrev Christoph M. Becker cmbecker69@gmx.de:

A problem is that the ext/pdo_firebird tests rely on ext/interbase to
create a test database[1], which would require everybody who intends to
run these tests to install PECL/interbase package now. It would be
great if somebody would rewrite this, so interbase would no longer be a
dependency of the pdo_firebird tests.

[1] <https://github.com/php/php-src/blob/PHP-7.4/ext/pdo_firebird/tests
/testdb.inc>

Yeah that is one thing that is kind of unfortunate and a bad practice.
However since interbase was disabled for AppVeyor, these tests had no
chance to run in a while. Looking at the actual code in testdb.inc, it
seems like it creates the database for testing purposes, I would
suppose this is due to PDO_Firebird perhaps requiring a database to
continue its connection flow in PDO itself.

I'm not sure how we can go about this, maybe a .sql file for testers
who run the test suite for schemas and then see if we can integrate
some auto importer in our CI build scripts for this special case? (In
the case we cannot go around PDO). Either way the test files requires
a manual touch to configure credentials to run.

I'm personally fine with the simple solution, thoughts?

Firebird requires the database to exist before PDO can make a connection
and there are a number of ways to handle it, but the simple solution is
to use the example database that comes with Firebird. What does
complicate things though is that distributions often bundle it
separately, and will 'improve security' by not using the default user
login. Hence the credentials problem becomes installation dependent. A
default install using the official Firebird distributions should work
with the test suite using the sample database rather than the temporary
one.

--
Lester Caine - G8HFL

Contact - https://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - https://lsces.co.uk
EnquirySolve - https://enquirysolve.com/
Model Engineers Digital Workshop - https://medw.co.uk
Rainbow Digital Media - https://rainbowdigitalmedia.co.uk

Yeah that is one thing that is kind of unfortunate and a bad practice.
However since interbase was disabled for AppVeyor, these tests had no
chance to run in a while. Looking at the actual code in testdb.inc, it
seems like it creates the database for testing purposes, I would
suppose this is due to PDO_Firebird perhaps requiring a database to
continue its connection flow in PDO itself.

I'm not sure how we can go about this, maybe a .sql file for testers
who run the test suite for schemas and then see if we can integrate
some auto importer in our CI build scripts for this special case? (In
the case we cannot go around PDO). Either way the test files requires
a manual touch to configure credentials to run.

I'm personally fine with the simple solution, thoughts?

Firebird requires the database to exist before PDO can make a connection
and there are a number of ways to handle it, but the simple solution is
to use the example database that comes with Firebird. What does
complicate things though is that distributions often bundle it
separately, and will 'improve security' by not using the default user
login. Hence the credentials problem becomes installation dependent. A
default install using the official Firebird distributions should work
with the test suite using the sample database rather than the temporary
one.

A patch would be welcome!

--
Christoph M. Becker

A patch would be welcome!
Looks like every file needs modifying ...
I'll see what I can do, but my local php-src is broken and will not sync
currently :(

--
Lester Caine - G8HFL

Contact - https://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - https://lsces.co.uk
EnquirySolve - https://enquirysolve.com/
Model Engineers Digital Workshop - https://medw.co.uk
Rainbow Digital Media - https://rainbowdigitalmedia.co.uk

Indeed, it seems that pdo_firebird requires to connect to an existing
database, contrary to e.g. pdo_mysql which supports DSN like
mysql:host=localhost, to my knowledge.

Postgres works the same way - a connection is always to a single database,
so you cannot have a DSN which doesn't specify one. Default installations
now ship with an empty database called "postgres" to run administration
commands like "CREATE DATABASE" on.

Glancing that the ext/pgsql and ext/pdo_pgsql tests, though, it looks like
we just require the user to have run "createdb test" before running the
test suite.

Regards,

Rowan Collins
[IMSoP]

Indeed, it seems that pdo_firebird requires to connect to an existing
database, contrary to e.g. pdo_mysql which supports DSN like
mysql:host=localhost, to my knowledge.

Postgres works the same way - a connection is always to a single database,
so you cannot have a DSN which doesn't specify one. Default installations
now ship with an empty database called "postgres" to run administration
commands like "CREATE DATABASE" on.

Glancing that the ext/pgsql and ext/pdo_pgsql tests, though, it looks like
we just require the user to have run "createdb test" before running the
test suite.

Indeed, see
https://github.com/php/php-src/blob/php-7.3.5/ext/pgsql/tests/README.

--
Christoph M. Becker

Glancing that the ext/pgsql and ext/pdo_pgsql tests, though, it looks like
we just require the user to have run "createdb test" before running the
test suite.
Indeed, see
https://github.com/php/php-src/blob/php-7.3.5/ext/pgsql/tests/README.

https://github.com/bitweaver/install/blob/master/create_firebird_database.php

Generates a firebird database from PHP via the command line which is the
other approach.
I've started again with a clean sheet and have a working php-src so I'll
try and sort a suitable patch ...

--
Lester Caine - G8HFL

Contact - https://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - https://lsces.co.uk
EnquirySolve - https://enquirysolve.com/
Model Engineers Digital Workshop - https://medw.co.uk
Rainbow Digital Media - https://rainbowdigitalmedia.co.uk

--
Lester Caine - G8HFL

Contact - https://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - https://lsces.co.uk
EnquirySolve - https://enquirysolve.com/
Model Engineers Digital Workshop - https://medw.co.uk
Rainbow Digital Media - https://rainbowdigitalmedia.co.uk

Hi Christoph

Den tor. 2. maj 2019 kl. 16.57 skrev Christoph M. Becker cmbecker69@gmx.de:

Hi Kalle,

Indeed, it seems that pdo_firebird requires to connect to an existing
database, contrary to e.g. pdo_mysql which supports DSN like
mysql:host=localhost, to my knowledge. But even if pdo_firebird has
been connected to an existing database, it appears that it's not
possible to create a new database via PDO (I got something like "prepare
does not support CREATE DATABASE", even with emulated prepares turned on).

Please check this commit:
http://git.php.net/?p=php-src.git;a=commitdiff;h=c9599c1c726e3bce8ce5a80c1ac8ee2c81bffb61

It also includes a 'php.fdb', which I'm not certain whether or not we
should keep as its quite large, but at least it makes the tests run
for me with 15/15 pass.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

It also includes a 'php.fdb', which I'm not certain whether or not we
should keep as its quite large, but at least it makes the tests run
for me with 15/15 pass.

The problem with including a database is it's dependent on the version
of FB running. This will only run with FB3 while FB2.x is still in
common use hence it being preferable to use something supplied with the
FB installation.

https://firebirdsql.org/manual/qsg10-connecting.html

--
Lester Caine - G8HFL

Contact - https://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - https://lsces.co.uk
EnquirySolve - https://enquirysolve.com/
Model Engineers Digital Workshop - https://medw.co.uk
Rainbow Digital Media - https://rainbowdigitalmedia.co.uk

Hi

Den tor. 2. maj 2019 kl. 22.55 skrev Lester lester@lsces.co.uk:

The problem with including a database is it's dependent on the version
of FB running. This will only run with FB3 while FB2.x is still in
common use hence it being preferable to use something supplied with the
FB installation.

https://firebirdsql.org/manual/qsg10-connecting.html

For Windows at least, we only support Firebird3 with the binary-sdk, I
don't know about Unix.

However I updated my patch, see this for more info:
http://git.php.net/?p=php-src.git;a=commitdiff;h=1c893b89bdb2b8bd5608c14cd0930727db6ba409

If a brave soul can test on Unix it would be great, but this is about
as good as it gets.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Den tor. 2. maj 2019 kl. 22.55 skrev Lester lester@lsces.co.uk:

The problem with including a database is it's dependent on the version
of FB running. This will only run with FB3 while FB2.x is still in
common use hence it being preferable to use something supplied with the
FB installation.

https://firebirdsql.org/manual/qsg10-connecting.html

For Windows at least, we only support Firebird3 with the binary-sdk, I
don't know about Unix.

However I updated my patch, see this for more info:
http://git.php.net/?p=php-src.git;a=commitdiff;h=1c893b89bdb2b8bd5608c14cd0930727db6ba409

If a brave soul can test on Unix it would be great, but this is about
as good as it gets.

ACK.

While running the tests with a PHP debug version, I've noticed that
bug_aaa.phpt is causing a memory leak. Maybe someone proficient with
Firebird/Interbase will want to have a look at it.

--
Christoph M. Becker

Hi Christoph

Den fre. 3. maj 2019 kl. 12.02 skrev Christoph M. Becker cmbecker69@gmx.de:

While running the tests with a PHP debug version, I've noticed that
bug_aaa.phpt is causing a memory leak. Maybe someone proficient with
Firebird/Interbase will want to have a look at it.

Thank you for testing, good to know that they at least run there too!

As for the memory leak, perhaps we should mark the relevant test with
an --XFAIL-- or something? The name was suspicious in the first place.

Guess this is as good as it gets for PDO_Firebird at least. I can see
the PR regarding adding boolean types to PDO_Firebird was updated too,
to reflect these changes so it should be good to go into 7.4 I suppose
if its rebased.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi Kalle!

Den fre. 3. maj 2019 kl. 12.02 skrev Christoph M. Becker cmbecker69@gmx.de:

While running the tests with a PHP debug version, I've noticed that
bug_aaa.phpt is causing a memory leak. Maybe someone proficient with
Firebird/Interbase will want to have a look at it.

Thank you for testing, good to know that they at least run there too!

ACK. It would be even nicer to have these tests run on a CI, though.

As for the memory leak, perhaps we should mark the relevant test with
an --XFAIL-- or something? The name was suspicious in the first place.

The respective commit message[1] explains the test name; it is about a
fix for an unfiled bug, so instead of a concrete bug number the suffix
"aaa" had been chosen.

Anyhow, I have submitted PR #4111 which would fix the memory leak. I'm
not 100% sure that this couldn't cause a UAF scenario, though, so I have
targeted PHP-7.4 for now.

Guess this is as good as it gets for PDO_Firebird at least.

I have submitted PR #4112 to bring back the common PDO tests, which are
the majority of existing tests for pdo_firebird. Three tests are
failing (which should be investigated), but these tests already failed
in PHP-7.3 and maybe before.

[1]
http://git.php.net/?p=php-src.git;a=commit;h=cf46ac1179376f58895feb6ed914b03bea19e295
[2] https://github.com/php/php-src/pull/4111
[3] https://github.com/php/php-src/pull/4112

--
Christoph M. Becker

I have submitted PR #4112 to bring back the common PDO tests, which are
the majority of existing tests for pdo_firebird. Three tests are
failing (which should be investigated), but these tests already failed
in PHP-7.3 and maybe before.

I had a closer look at the tests failing with Firebird 3.0.4.33054 (x64):

bug_69356.phpt fails because FB doesn't support SELECT expr; queries;
apparently, a FROM clause would be required.

bug_71447.phpt fails because there are issues regarding comments with a
single quote.

bug_73234.phpt fails because FB doesn't support defining explicit
nullable columns, i.e.

CREATE TABLE (id INT NULL)

is unsupported. Removing the NULL let's the test proceed, but binding a
zero as PDO::PARAM_NULL apparently inserts string(1) "0" which looks
like a bug in the driver.

--
Christoph M. Becker

Hi Christoph

Den lør. 4. maj 2019 kl. 14.19 skrev Christoph M. Becker cmbecker69@gmx.de:

I have submitted PR #4112 to bring back the common PDO tests, which are
the majority of existing tests for pdo_firebird. Three tests are
failing (which should be investigated), but these tests already failed
in PHP-7.3 and maybe before.

I had a look at your PRs, both look good! Like I mentioned on the
common.phpt one, I wasn't aware of this PDO magic, so its good that
you fixed it quickly.

+1 on both PRs.

I had a closer look at the tests failing with Firebird 3.0.4.33054 (x64):

bug_69356.phpt fails because FB doesn't support SELECT expr; queries;
apparently, a FROM clause would be required.

If that is the case, we should add the FROM clause. When I tested it,
it was passing but I'm uncertain how exactly. I was using a 3.0.2 x64
build on Windows for these, so there seems to be something odd going
on here.

bug_71447.phpt fails because there are issues regarding comments with a
single quote.

Could this be an SQL dialect setting missing given its pass for me?

bug_73234.phpt fails because FB doesn't support defining explicit
nullable columns, i.e.

CREATE TABLE (id INT NULL)

is unsupported. Removing the NULL let's the test proceed, but binding a
zero as PDO::PARAM_NULL apparently inserts string(1) "0" which looks
like a bug in the driver.

Again odd it did not fail for me, but we should log a bug about this
issue if there isn't one already and hope that someone will pick it up
unless you want to invest the time into it.

Thanks for looking into these! Much appreciated.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Hi Kalle!

Den lør. 4. maj 2019 kl. 14.19 skrev Christoph M. Becker cmbecker69@gmx.de:

I have submitted PR #4112 to bring back the common PDO tests, which are
the majority of existing tests for pdo_firebird. Three tests are
failing (which should be investigated), but these tests already failed
in PHP-7.3 and maybe before.

I had a look at your PRs, both look good! Like I mentioned on the
common.phpt one, I wasn't aware of this PDO magic, so its good that
you fixed it quickly.

+1 on both PRs.

Great. I've merged both.

I had a closer look at the tests failing with Firebird 3.0.4.33054 (x64):

bug_69356.phpt fails because FB doesn't support SELECT expr; queries;
apparently, a FROM clause would be required.

If that is the case, we should add the FROM clause. When I tested it,
it was passing but I'm uncertain how exactly. I was using a 3.0.2 x64
build on Windows for these, so there seems to be something odd going
on here.

bug_71447.phpt fails because there are issues regarding comments with a
single quote.

Could this be an SQL dialect setting missing given its pass for me?

That's quite possible.

bug_73234.phpt fails because FB doesn't support defining explicit
nullable columns, i.e.

CREATE TABLE (id INT NULL)

is unsupported. Removing the NULL let's the test proceed, but binding a
zero as PDO::PARAM_NULL apparently inserts string(1) "0" which looks
like a bug in the driver.

Again odd it did not fail for me, but we should log a bug about this
issue if there isn't one already and hope that someone will pick it up
unless you want to invest the time into it.

I'd like to pass this to someone more knowledgeable with PDO and
especially Interbase/Firebird.

Thanks for looking into these! Much appreciated.

Thanks for the basic port of the test suite to pure PDO, and for your
efforts to move ext/interbase to PECL!

Thanks,
Christoph