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.

Sign up for GitHub

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

Jump to bottom

Add new PDO mysql connection attr to control multi statements option #896

Closed
wants to merge 7 commits into from
Closed

Add new PDO mysql connection attr to control multi statements option #896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2cdbf7
Start adding new attribute to control multi statements
pwolanin Nov 7, 2014
d6949b0
add new constant to test
pwolanin Nov 10, 2014
bf776eb
add test case for new connection attribute
pwolanin Nov 10, 2014
61d53a3
Take out php6 style expect strings and add more checks to the test
pwolanin Nov 25, 2014
294bfb1
add additional test verifying multiple result proc still works when l…
pwolanin Nov 28, 2014
3580271
Add a check for a single statement with trailing delimiter
pwolanin Dec 1, 2014
e800bdb
Add output at the top of the foreach loop to make debugging easier
pwolanin Dec 1, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions ext/pdo_mysql/mysql_driver.c
View file
Open in desktop
Expand Up @@ -556,10 +556,14 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options TSRMLS_
#ifdef CLIENT_MULTI_RESULTS
|CLIENT_MULTI_RESULTS
#endif
;
#ifdef CLIENT_MULTI_STATEMENTS
|CLIENT_MULTI_STATEMENTS
if (!driver_options) {
connect_opts |= CLIENT_MULTI_STATEMENTS;
} else if (pdo_attr_lval(driver_options, PDO_MYSQL_ATTR_MULTI_STATEMENTS, 1 TSRMLS_CC)) {
connect_opts |= CLIENT_MULTI_STATEMENTS;
}
#endif
;

#if defined(PDO_USE_MYSQLND)
int dbname_len = 0;
Expand Down
2 changes: 1 addition & 1 deletion ext/pdo_mysql/pdo_mysql.c
View file
Open in desktop
Expand Up @@ -126,7 +126,7 @@ static PHP_MINIT_FUNCTION(pdo_mysql)
#if MYSQL_VERSION_ID > 50605 || defined(PDO_USE_MYSQLND)
REGISTER_PDO_CLASS_CONST_LONG("MYSQL_ATTR_SERVER_PUBLIC_KEY", (zend_long)PDO_MYSQL_ATTR_SERVER_PUBLIC_KEY);
#endif

REGISTER_PDO_CLASS_CONST_LONG("MYSQL_ATTR_MULTI_STATEMENTS", (zend_long)PDO_MYSQL_ATTR_MULTI_STATEMENTS);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be guarded like the previous constant — we still support MySQL 4.1 (not entirely clear on why, admittedly), and it doesn't support CLIENT_MULTI_STATEMENTS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the connection code is guarded with:

#ifdef CLIENT_MULTI_STATEMENTS

not by MySQL version. In fact, it looks like the 4.1 API supports it:
http://dev.mysql.com/doc/refman/4.1/en/c-api-multiple-queries.html

So, I think guarding the constant with the same check would make sense, though I'm not sure at this point when it would be undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I got tricked by a sentence in the MySQL 5.0 manual implying it was new in that version. Sorry about that! Disregard what I said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so does it make sense to guard the class constant using

#ifdef CLIENT_MULTI_STATEMENTS

or since it would be ignored if somehow that wan't defined, it's better to always define it to avoid PHP code having to check if it exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're sure it's always defined in MySQL >= 4.1, then let's not bother with the #ifdef. No point cluttering the code unnecessarily. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's actually defined by default by mysqlnd:

mysqlnd/mysqlnd_enum_n_def.h
96:#define CLIENT_MULTI_STATEMENTS      (1UL << 16) /* Enable/disable multi-stmt support */

It's also available in the C API since January 2003
http://bazaar.launchpad.net/~mysql/libmysql/1.0/view/1412/include/mysql_com.h#L109

http://bazaar.launchpad.net/~mysql/libmysql/1.0/revision/1403.62.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, my conclusion is we really don't need the #ifdef anywhere any more, but I guess leaving it in where it is might be fine for now.


#ifdef PDO_USE_MYSQLND
mysqlnd_reverse_api_register_api(&pdo_mysql_reverse_api TSRMLS_CC);
Expand Down
3 changes: 2 additions & 1 deletion ext/pdo_mysql/php_pdo_mysql_int.h
View file
Open in desktop
Expand Up @@ -172,8 +172,9 @@ enum {
PDO_MYSQL_ATTR_SSL_CAPATH,
PDO_MYSQL_ATTR_SSL_CIPHER,
#if MYSQL_VERSION_ID > 50605 || defined(PDO_USE_MYSQLND)
PDO_MYSQL_ATTR_SERVER_PUBLIC_KEY
PDO_MYSQL_ATTR_SERVER_PUBLIC_KEY,
#endif
PDO_MYSQL_ATTR_MULTI_STATEMENTS,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this needs to be behind a MYSQL_VERSION_ID check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above - this should not be needed - supported by Mysql 4.1

};

#endif
95 changes: 95 additions & 0 deletions ext/pdo_mysql/tests/pdo_mysql_attr_multi_statements.phpt
View file
Open in desktop
@@ -0,0 +1,95 @@
--TEST--
PDO::MYSQL_ATTR_MULTI_STATEMENTS
--SKIPIF--
<?php
require_once(dirname(__FILE__) . DIRECTORY_SEPARATOR . 'skipif.inc');
require_once(dirname(__FILE__) . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');
MySQLPDOTest::skip();
$db = MySQLPDOTest::factory();
?>
--INI--
error_reporting=E_ALL
--FILE--
<?php
require_once(dirname(__FILE__) . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');

$dsn = MySQLPDOTest::getDSN();
$user = PDO_MYSQL_TEST_USER;
$pass = PDO_MYSQL_TEST_PASS;

$table = sprintf("test_%s", md5(mt_rand(0, PHP_INT_MAX)));
$db = new PDO($dsn, $user, $pass);
$db->exec(sprintf('DROP TABLE IF EXISTS %s', $table));
$create = sprintf('CREATE TABLE %s(id INT)', $table);
$db->exec($create);
$db->exec(sprintf('INSERT INTO %s(id) VALUES (1)', $table));
$stmt = $db->query(sprintf('SELECT * FROM %s; INSERT INTO %s(id) VALUES (2)', $table, $table));
$stmt->closeCursor();
$info = $db->errorInfo();
var_dump($info[0]);
$stmt = $db->query(sprintf('SELECT id FROM %s', $table));
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
// A single query with a trailing delimiter.
$stmt = $db->query('SELECT 1 AS value;');
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));

// New connection, does not allow multiple statements.
$db = new PDO($dsn, $user, $pass, array(PDO::MYSQL_ATTR_MULTI_STATEMENTS => false));
$stmt = $db->query(sprintf('SELECT * FROM %s; INSERT INTO %s(id) VALUES (3)', $table, $table));
var_dump($stmt);
$info = $db->errorInfo();
var_dump($info[0]);

$stmt = $db->query(sprintf('SELECT id FROM %s', $table));
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
// A single query with a trailing delimiter.
$stmt = $db->query('SELECT 1 AS value;');
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));

$db->exec(sprintf('DROP TABLE IF EXISTS %s', $table));
print "done!";
?>
--EXPECTF--
string(5) "00000"
array(2) {
[0]=>
array(1) {
["id"]=>
string(1) "1"
}
[1]=>
array(1) {
["id"]=>
string(1) "2"
}
}
array(1) {
[0]=>
array(1) {
["value"]=>
string(1) "1"
}
}
bool(false)
string(5) "42000"
array(2) {
[0]=>
array(1) {
["id"]=>
string(1) "1"
}
[1]=>
array(1) {
["id"]=>
string(1) "2"
}
}
array(1) {
[0]=>
array(1) {
["value"]=>
string(1) "1"
}
}
done!

1 change: 1 addition & 0 deletions ext/pdo_mysql/tests/pdo_mysql_class_constants.phpt
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ if (!extension_loaded('mysqli') && !extension_loaded('mysqlnd')) {
"MYSQL_ATTR_SSL_CAPATH" => true,
"MYSQL_ATTR_SSL_CIPHER" => true,
"MYSQL_ATTR_COMPRESS" => true,
"MYSQL_ATTR_MULTI_STATEMENTS" => true,
);

if (!MySQLPDOTest::isPDOMySQLnd()) {
Expand Down