Issue #62 fix query params

[istaveren]

Fix for issue #62, #63 & #64

Also added some tests.

[istaveren]

I checked with php-cs-fixer fix tests/ --rules=@PSR2 Runnig it with .php_cs failed with PHP Fatal error: Uncaught Error: Class 'Symfony\CS\Config\Config' not found in /home/iwan/projects/prive/pdo-via-oci8/.php_cs:73

[yajra]

Can you please fix the conflicts and update the PR from 1.0 branch? I already fixed the docker used by the package for it to work with TravisCI. Thanks!

[istaveren]

Conflicts fixed.

Nice that you added the docker image. Consider to also run the test in docker. Or if you like I can give that a try.

[yajra]

Just tested this on my project and it seems like the patch for bindParam is failing the app.

  Exception trace:

  1   ErrorException::("oci_bind_by_name(): ORA-01036: illegal variable name/number")
      /www/yajra/oci8/pdo-via-oci8/src/Pdo/Oci8/Statement.php:492

  2   oci_bind_by_name(":p-1", "platform")
      /www/yajra/oci8/pdo-via-oci8/src/Pdo/Oci8/Statement.php:492

[istaveren]

Can you write a test or add an example on how I can reproduce it?

Please check ConnectionTest->testBindParam or suggestions on how I should test?

[yajra]

I am using this package along with my other one on Laravel apps. I think you can recreate the issue by creating a fresh laravel app then just run the migration.

[istaveren]

Yes, I can probably do that. But would it not be easy to just write a failing unit test? So that I can fix it?

The only change I did is I added the -1

      if (is_numeric($parameter)) {
            $parameter = ':p' . ($parameter-1);
        }

If you send oci_bind_by_name(":p-1", "platform") I guess somehow you are sending a $parameter that is 0 or is somehow indicated as numeric?

So my guess is that the check is_numeric need to be improved. But for that, I need to know what you are sending to it.

Will this fix it?

        if (preg_match('/^\d+$/', $parameter)) {
            $parameter = ":p".(intval($parameter)-1);
        }

[yajra]

Oh yeah, writing a failing test is a great idea. Will try to create one. Btw, you need some seeder for the laravel side for some basic inserts. Thanks!

Get Outlook for iOShttps://aka.ms/o0ukef

---

From: Iwan van Staveren notifications@github.com Sent: Friday, April 26, 2019 9:06 PM To: yajra/pdo-via-oci8 Cc: Arjay Angeles; Comment Subject: Re: [yajra/pdo-via-oci8] Issue #62 fix query params (#63)

Yes, I can probably do that. But would it not be easy to just write a failing unit test? So that I can fix it?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/yajra/pdo-via-oci8/pull/63#issuecomment-487049879, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAUQH7OWTZSWPSICZJTL6WLPSL46FANCNFSM4HG5LN7Q.

[istaveren]

I would love to fix this, can we chat? Please ping me on hangout istaveren or some other channel.

[yajra]

Can't reproduce a failing tests but it seems like there are some modifications on my other package that is needed to work with this. Creating a PR for it and we'll see how it works.

[yajra]

Or actually, this is how it fails:

    public function testBindParamName()
    {
        $stmt = $this->con->prepare('select * from all_tables where upper(owner) = upper(?) and upper(table_name) = upper(?)');
        $owner = 'owner';
        $table = 'table';
        $this->assertTrue($stmt->bindParam(0, $owner, PDO::PARAM_STR));
        $this->assertTrue($stmt->bindParam(1, $table, PDO::PARAM_STR));
    }

But as you've stated, the param should start at 1?

[yajra]

PR https://github.com/yajra/laravel-oci8/pull/505 sent but it's failing on RefCursor tests. Will try to check again further when I got the chance.

[istaveren]

Correct see issue #63 a bindParam should start with 1.

https://www.php.net/manual/en/pdostatement.bindparam.php

[yajra]

Or I might be missing something that we can adjust to support bindParam using int?

[istaveren]

So how can this be fixed?

Both param 0 and Param 1 should point to the first param? I think in that case your code will still not work, because your code assumes 1 to be the second one where it is the first one. By the PDO definition.

Or actually, this is how it fails:

    public function testBindParamName()
    {
        $stmt = $this->con->prepare('select * from all_tables where upper(owner) = upper(?) and upper(table_name) = upper(?)');
        $owner = 'owner';
        $table = 'table';
        $this->assertTrue($stmt->bindParam(0, $owner, PDO::PARAM_STR));
        $this->assertTrue($stmt->bindParam(1, $table, PDO::PARAM_STR));
    }

But as you've stated, the param should start at 1?

[yajra]

Sorry I've provided a bad example. Basically, this kind of syntax is not yet supported and thus should be removed in this PR.

// this example is bad, not supported.
$stmt = $this->con->prepare(
  'select * from all_tables where upper(owner) = upper(?) and upper(table_name) = upper(?)'
);
$this->assertTrue($stmt->bindParam(0, $owner, PDO::PARAM_STR));
$this->assertTrue($stmt->bindParam(1, $table, PDO::PARAM_STR));

// this is how it would be processed now.
$stmt = $this->con->prepare(
  'select * from all_tables where upper(owner) = upper(?) and upper(table_name) = upper(?)'
);

// after prepare, sql becomes
'select * from all_tables where upper(owner) = upper(:p0) and upper(table_name) = upper(:p1)'

$this->assertTrue($stmt->bindParam(":p0", $owner, PDO::PARAM_STR));
$this->assertTrue($stmt->bindParam(":p1", $table, PDO::PARAM_STR));

[istaveren]

Ok reverted it, will create a pull request on #64

[yajra]

Released on v1.4.0, thanks a lot! 🍻