support 1 dialect

[sim1984]

This change adds support for databases created in 1 dialect. It also fixes bugs https://bugs.php.net/bug.php?id=71652 and https://bugs.php.net/bug.php?id=65690.
Added dialect parameter to DSN. By default, it is 3.

try {
	$dsn = 'firebird:dbname=localhost/3053:test;charset=utf-8;dialect=1';
	$user = 'SYSDBA';
	$password = 'masterkey';
	$att = new \PDO($dsn, $user, $password, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
	$sql = 
	'SELECT 
	  1 as n, 
	  2.0 as f, 
	  cast(0.76 as numeric(15, 2)) as k,
      cast(\'2019-06-12\' as date) as dt	  
	FROM RDB$DATABASE';
	$query = $att->prepare($sql);
	$query->execute();
	
	$rows = $query->fetchAll(\PDO::FETCH_OBJ);
	$json = json_encode($rows, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR);
	echo "<pre>$json</pre>";
}
catch(\Throwable $e) {
	echo $e->getMessage() . '<br>';
	echo '<pre>' . $e->getTraceAsString() . '</pre>';
}

[cmb69]

cc @MartinKoeditz

[MartinKoeditz]

Looks good. Thank you.

[sim1984]

I would really like this improvement to be included in PHP 7.4, especially since the ibase_ * extension has been moved to PECL. The changes made do not break backward compatibility for 3rd dialect databases. For 1-dialect databases, queries are prepared according to their rules; a scalable DOUBLE also works correctly. However, I cannot write tests to verify the 1-dialect. Since all your tests are initially based on a pre-created database of 3 dialects. If I need to make any other changes, then I will do it.

[KalleZ]

cc @derickr

[cmb69]

Since all your tests are initially based on a pre-created database of 3 dialects.

Well, the environment variables PDO_FIREBIRD_TEST_DATABASE and PDO_FIREBIRD_TEST_DSN can be easiliy adjusted, and if I understand correctly, creating a dialect 1 database is as simple as:

SQL> set sql dialect 1;
SQL> create DATABASE '…' user '…' password '…';

However, running the tests against such database seem to have the same results as before. So it seems to be reasonable to add tests which fail for dialect 1, and to mark these with a SKIPIF directive to not run them against a dialect 3 database.

[cmb69]

Nit: there should be spaces after if and around the == operator.

[sim1984]

done

[cmb69]

Would it make sense to make this member a bitfield (not sure about the width), and to take away the respective space from _reserved?

[sim1984]

I thought about it. In principle, 2 bits are now enough to store information about the SQL dialect. However, doubts overcame me here:

		H->sql_dialect = PDO_FB_DIALECT;
		if (vars[3].optval) {
			H->sql_dialect = atoi(vars[3].optval);
		}

What happens if you pass a number greater than 3? Now such things are simply ignored (the dialect is considered 3). If we will use bit fields, then we need to add an additional check against overflow.

[cmb69]

As it is now, there is already the possiblity for overflow. sql_dialect is declared as unsigned, but passed to isc_dsql_prepare() which expects an unsigned short. And atoi() even returns an int, so there might be implementation dependent conversion of negative integer to unsigned. Doesn't look like a real problem, though, since the developer is not supposed to pass some arbitrary value as dialect. Anyhow, I don't really mind if the dialect is stored in a bitfield, or an unsigned or unsigned short.

[sim1984]

Well, the environment variables PDO_FIREBIRD_TEST_DATABASE and PDO_FIREBIRD_TEST_DSN can be easiliy adjusted, and if I understand correctly, creating a dialect 1 database is as simple as:

SQL> set sql dialect 1;
SQL> create DATABASE '…' user '…' password '…';

Yes you are right.

However, running the tests against such database seem to have the same results as before. So it seems to be reasonable to add tests which fail for dialect 1, and to mark these with a SKIPIF directive to not run them against a dialect 3 database.

Hm. Should we run all tests on databases in two dialects? Most tests will give the same result on both bases. There are a small number of tests that are tied to the dialect of the database. Above, I have an example that will not be executed if the database is created in one dialect, and the dns contains another. If you help me set up the environment for testing, I will write the above example as a test.

[cmb69]

Should we run all tests on databases in two dialects?

Well, I think we should be able to run the tests on databases in two dialects. As it is now, we don't run pdo_firebird tests on any CI (on Travis and AzurePipelines the extension is not even built, on AppVeyor it is built, but excluded from the tests). It's up those maintaining the extension to run the tests.

If you help me set up the environment for testing, I will write the above example as a test.

Great! I think it is sufficient to create the (empty) database(s), and to set following environment variables:

PDO_FIREBIRD_TEST_DATABASE=C:\test.fdb
PDO_FIREBIRD_TEST_DSN=firebird:dbname=C:\test.fdb
PDO_FIREBIRD_TEST_PASS=12345
PDO_FIREBIRD_TEST_PASSWORD=12345
PDO_FIREBIRD_TEST_USER=sysdba

The values of the variables have to be adapted to the setup, of course.

[KalleZ]

@cmb69 How come it is not run/built on CIs? I thought it was fixed when you and I patched up the ext/pdo_firebird test suite to not depend on ext/interbase, or did I miss something here?

[cmb69]

@KalleZ, I guess nobody cared to set it up actually. AppVeyor doesn't support Firebird out of the box, so we'd need to install the database server manually (which likely slows down CI, if even possible).

[KalleZ]

@cmb69 Ah I see! Thanks for the info, and in that case I think the exception of running the tests makes perfect sense unless we want a dedicated matrix for databases like this

[sim1984]

I added testing 1 dialect. If you set the line containing "dialect=1" to the environment variable PDO_FIREBIRD_TEST_DSN, it successfully fulfills the condition that the database is created in 1 dialect. If "dialect=1" is not available in the DSN, then the test is skipped. Will it go like that?

set PDO_FIREBIRD_TEST_DSN=firebird:dbname=localhost:test;dialect=1

[cmb69]

Besides the minor fixes to the test case, this looks good to me now. I also think that this is good for PHP 7.4, as it is a small self-contained feature, and support for dialect1 database may still be required in some cases. I think @derickr and/or @petk should have the final say whether this should go into 7.4 or master only.

[cmb69]

Suggested change

	string(3) "0.76"
	string(4) "0.76"

[cmb69]

Suggested change

	string(3) "0.76"
	string(4) "0.76"

[KalleZ]

I agree with @cmb69, potentially could help migration from ext/interbase, but I do not see it being an issue coming in 7.4.1 either as its small and self contained

[petk]

Hello, then it's good to go to PHP 7.4 also from my side. Unless @derickr has more thoughts here and sees something we've missed...

Thank you @sim1984 for the improvement.

[cmb69]

Thanks for the PR! Since there are apparently no objections, I've applied as 3fb42a3 to PHP-7.4.