Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Fix PDO parameter names #178

Closed
wants to merge 5 commits into from
Closed

Fix PDO parameter names #178

wants to merge 5 commits into from

Conversation

alshayed
Copy link
Contributor

It appears that PDO only allows alphanumeric and underscore characters ([a-zA-Z0-9_]) for placeholder parameter names. However, the zend-db pdo driver just passes on anything without regard to those limitations. This patch addresses that by replacing any non-allowable characters with an underscore.

This is needed because various databases you might access through the PDO driver support more characters than that unquoted. For example MySQL also supports a dollar sign ($) and DB2 on the IBM i (I'm using this via PDO ODBC) supports the dollar sign and the hash/pound sign (#). (Side note - it works fine using the IbmDb2 driver which does positional parameters, but that is only available if you have that built in to PHP and are running it directly on the DB2 server or have purchased DB2 Connect)

I believe this addresses issue #35 though obviously not by changing to positional parameters as suggested there.

@alshayed
Copy link
Contributor Author

I'm not really sure how to add the tests to exercise this through the database. If somebody would be willing to turn this into the proper test code I would really appreciate it.

Here's how you can prove it manually with PDO/MySQL:

CREATE TABLE test001 (
id integer not null auto_increment,
text_ char(10),
text$ char(10),

primary key (id)
);

Then example PHP in the root directory:

<?php

require __DIR__ . '/vendor/autoload.php';

$db_options = [
    'driver' => 'pdo',
    'dsn' => 'mysql:host=YOUR_DB_HOST;dbname=YOUR_DB_NAME;charset=utf8',
    'user' => 'YOUR_DB_USER',
    'pass' => 'YOUR_DB_PASS',
];

$db = new \Zend\Db\Adapter\Adapter($db_options);

$test001 = new \Zend\Db\TableGateway\TableGateway('test001', $db);
$test001->insert([
    'text_' => 'hi',
    'text$' => 'dude',
]);

$rowset = $test001->select();

$count = 0;
foreach ($rowset as $row) {
    $count++;
    echo json_encode($row) . PHP_EOL;
}

echo PHP_EOL . "found " . $count . " row(s)" . PHP_EOL;

So if you run it against the current master branch it fails with an exception, but my changes make it work as expected.

Copy link
Contributor

@ezimuel ezimuel left a comment

Choose a reason for hiding this comment

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

I don't think the approach of this PR is correct, in order to fix #35 we need to work on Zend\Db\Adapter\Driver\Pdo\Pdo::formatParameterName() and Zend\Db\Adapter\Driver\Pdo\Statement::bindParametersFromContainer(). I'm working on a new PR to address this.

@@ -298,7 +298,7 @@ public function getPrepareType()
public function formatParameterName($name, $type = null)
{
if ($type === null && !is_numeric($name) || $type == self::PARAMETERIZATION_NAMED) {
return ':' . $name;
return ':' . preg_replace('/[^a-zA-Z0-9_]/', '_', $name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This replace can produce conflicts in bindParam name. For instance, if you have 2 fields text- and text$ they will be replaced with text_ and text_ both.

@ezimuel
Copy link
Contributor

ezimuel commented Mar 3, 2017

Closing this in favor of #224

@ezimuel ezimuel closed this Mar 3, 2017
@alshayed alshayed deleted the alshayed-fixPdoParameterName branch May 23, 2017 16:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants