adding host (ommitted) parameter

[bleuscyther]

The host parameter ( args[3]) is locked to 127.0.0.1, because it is not used here. When the database host is not on 127.0.0.1 or the IP is not authorized the database connection fails.

[bleuscyther]

Need some help, why is this failing?

[abiusx]

Why is what failing? Care to elaborate?

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Apr 29, 2014, at 2:56 PM, jeffrey n. carre notifications@github.com wrote:

Need some help, why is this failing?

— Reply to this email directly or view it on GitHub.

[SvenRtbg]

Looking into it...

[SvenRtbg]

Ok, while I did not find a reason for the test fail, I do have an opinion of how to improve the code.

The class Database_pdo_mysql does a bad job validating its constructor parameters.

It is supposed to work like this:

• call it with zero parameters should not work. It returns "false", but constructors never return anything, so you end up with an object that gets created, but is unusable, because there is no database instance inside. This should fail more clearly.
• call it with one parameter assumes this parameter is a "PDO" class (failing to allow using classes that inherit from PDO). If this is not the case, again we end up with an unusable object that lacks the internal database instance, but this will at least fail with a warning because there is an unconditional call to "setAttribute" of this PDO instance that will fail if $this->dbh is no object with such method. Again, this failure is badly communicated to the programmer.
• call it with more than one parameter assumes that it is called with THREE parameters (which is not checked) in the current code (or FOUR if we look at the patch in the pull request). These parameters are not validated in any way, but simply passed into the DatabaseConfig object that acts as a value object to store the parameters.

The patch raises a valid issue, though: There is currently no way to influence the remote address of the Mysql database by passing the IP. But on the other hand this might be the reason for the Travis test fail: The test does not attempt to pass in 127.0.0.1, but relies on the default value in the config object. With the patch applied, there is a fifth parameter in the constructor call which carries a value. Which value? Since the test does not pass the IP, the absence of that parameter will first of all trigger a notice for accessing an array index that is not there, but then pass NULL as the parameters value. NULL is not the default value, and this overwrites the default 127.0.0.1 as the database host.

I can think of many reasons why I don't like the approach of overloading a function call with dynamic evaluation of parameters, but if this approach is chosen, at least the parameter validation has to be rock solid and helpful in error cases.

So after reading my rant, would you dare to add some validation to this constructor? I'd very much appreciate it. And by the way: You might also be able to add some testing for this as well, but I fear this would be asking too much. Thank you nonetheless for your contribution so far.

If you decide to update your pull request, simply add commits to your existing branch and push to your repo.

[mebjas]

The test cases are failing because there are certain test cases where host is not passed as parameter and thus there exists no $args[3] in such case.

[abiusx]

Just a note, localhost usually behaves differently from 127.0.0.1 in that it uses local sockets instead of network interface. Sometimes because this is a different media, the connection fails.

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Apr 29, 2014, at 3:46 PM, minhaz notifications@github.com wrote:

phpsec\Database_pdo_mysql_Test::testQueryExecutionWithNamedParameters phpsec\Database_pdo_mysql_Test::testQueryExecutionWithArrayParameters phpsec\Database_pdo_mysql_Test::testQueryExecutionWithExpandedListParameters phpsec\Database_pdo_mysql_Test::testDatabaseConnection These are the failing test-cases

— Reply to this email directly or view it on GitHub.

[bleuscyther]

@SvenRtbg Sorry for the duplicate push. I will definitely find some time to propose something more complete.

@abiusx , +1