PHP MySQL Query Where x = $variable from function

Question

Why is this not working:

function listOrderComments ($factnr){
global $connection;

//$factnr = 123; //or $factnr = "123"; (Both work) 

$query = "SELECT * FROM orderstatus WHERE factuurnummer = '$factnr'"; 
$result = mysqli_query($connection, $query);

When I echo $factnr I get "123" back.

When I uncommented //$factnr = 123; my function is working.

Looked everywhere for a solution. check the type $factnr is (string).

Have you tried echoing your factnr being passed through your function parameter to check it is infact a valid number? — MinistryOfChaps, Jul 05 '17 at 21:20
[Little Bobby](http://bobby-tables.com/) says ***[your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php)*** Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php). Even [escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! — Jay Blanchard, Jul 05 '17 at 21:27

Tom · Answer 1 · 2019-07-15T11:29:41.483

4

Well if you're using a variable in your query you're opening yourself up to an injection attack for one.

If you're going to be using that variable I would recommend you use bind_param for your query

Read the PHP manual link below and you will be able to figure out the issue

http://php.net/manual/en/mysqli-stmt.bind-param.php

If you're passing in a variable to your function it should already be set so I don't understand why you're setting it to 123 anyway. So execute the sql statement and bind the parameter following the first example on the PHP docs page.

public function listOrderComments ($factnr)
{
   global $connection; 
   $query = "SELECT * FROM orderstatus WHERE factuurnummer = ?";
   $sql->prepare($query);
   $sql->bind_param("s", $factnr);
   $sql->execute();
   $result = $sql->get_result();
   $data = mysqli_fetch_all($result, MYSQLI_ASSOC);

   foreach ($data as $row) {
        print_r($row);
   }
}

Then do what you want with the result

edited Jul 15 '19 at 11:29

answered Jul 05 '17 at 21:24

Tom

389
2
15

1

`bindParam()` is PDO. Don't you mean `bind_param()`? The OP is using MySQLi. – Jay Blanchard Jul 05 '17 at 21:28
Nice one mate typo on my phone haha – Tom Jul 05 '17 at 21:31
1

when I use your code, I only get "123" visible on my screen and shouldnt I have something like this in the code as well? $result = mysqli_query($connection, $query); – Jeroen Broerse Jul 05 '17 at 21:38
This code uses the object-oriented style which is a lot less verbose and harder to mess up because of that. `query()` executes a query in one shot, this approach does the proper prepare, bind, execute cycle. – tadman Jul 05 '17 at 21:42
That is correct mate, add this and it should be accessible and you will be able to iterate through with a for each loop $row=mysqli_fetch_assoc($result); – Tom Jul 05 '17 at 21:44
@JeroenBroerse made some ammendments should work smoothly now – Tom Jul 06 '17 at 10:17
I feel really dumb.... I inserted the code but I only get "123" on my screen, not the info I want... – Jeroen Broerse Jul 06 '17 at 17:46
So has this helped? – Tom Jul 06 '17 at 17:55

score 1 · Answer 2 · edited Jul 06 '17 at 07:16

1

You can go with:

$query = "SELECT * FROM orderstatus WHERE factuurnummer = ". $factnr;

edited Jul 06 '17 at 07:16

Carl Binalla

5,393
5
27
46

answered Jul 05 '17 at 21:18

Krish

387
2
13

Yeah... otherwise you are querying for '"123"' – EvgenyKolyakov Jul 05 '17 at 21:19
@Krish is right, you're appending your variable to your string incorrectly. – Ryan Kozak Jul 05 '17 at 21:20
$factnr is a variable which varies so we cont go with 123 only @EvgenyKolyakov – Krish Jul 05 '17 at 21:20
1

Also, it's most advisable to use `mysql_real_escape()` to sanitize the input from potential sql injections. – EvgenyKolyakov Jul 05 '17 at 21:23
2

Turn the tide against teaching/propagating sloppy and dangerous coding practices. If you post an answer without prepared statements [you may want to consider this before posting](http://meta.stackoverflow.com/q/344703/). Additionally [a more valuable answer comes from showing the OP the right method](https://meta.stackoverflow.com/a/290789/1011527). – Jay Blanchard Jul 05 '17 at 21:27
1

[Escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe @EvgenyKolyakov – Jay Blanchard Jul 05 '17 at 21:27
@JayBlanchard I agree you need to learn to program first and know what what you are doing in the first place... – EvgenyKolyakov Jul 05 '17 at 21:29

Don Nico · Answer 3 · 2017-07-05T22:14:40.713

0

Use it correctly over "doted" concat. Following will just work fine:

$factnr = 123; 
$query = "SELECT * FROM orderstatus WHERE factuurnummer = " . $factnr;

UPDATE:

here is $factnr is passing as argument that supposed to be integer. Safe code way is DO NOT use havvy functions even going over more complicated PDO, but just verify, is this variable integer or not before any operation with it, and return some error code by function if not integer. Here is no danger of code injection into SQL query then.

function listOrderComments ($factnr){
global $connection;

if (!is_int($factnr)) return -1

//$factnr = 123; //or $factnr = "123"; (Both work) 

$query = "SELECT * FROM orderstatus WHERE factuurnummer = " . $factnr; 
$result = mysqli_query($connection, $query);

edited Jul 05 '17 at 22:14

answered Jul 05 '17 at 21:19

Don Nico

91
1
5

dotted concat is not more 'correct' than "bla $variable bla" – Jeff Jul 05 '17 at 21:26
Concatenation as-is. – Don Nico Jul 05 '17 at 21:27
2

Turn the tide against teaching/propagating sloppy and dangerous coding practices. If you post an answer without prepared statements [you may want to consider this before posting](http://meta.stackoverflow.com/q/344703/). Additionally [a more valuable answer comes from showing the OP the right method](https://meta.stackoverflow.com/a/290789/1011527). – Jay Blanchard Jul 05 '17 at 21:28
Jay, here is $facrnr is integer constant, isn't it so 'dangerous' to go over query string pasteurisation? And, yes, mysqli_real_escape_string might be used in case when $facrnr can take string value somewhere in code, but it's out of scope of concrete question above. – Don Nico Jul 05 '17 at 21:42

MinistryOfChaps · Answer 4 · 2017-07-05T21:54:13.707

Concatenating your code is not good practise. Your best solution is to use PDO statements. It means that your code is easier to look at and this prevents SQL injection from occuring if malice code slipped through your validation.

Here is an example of the code you would use.

<?php

 // START ESTABLISHING CONNECTION...
$dsn = 'mysql:host=host_name_here;dbname=db_name_here';
//DB username
$uname = 'username_here';
//DB password
$pass = 'password_here';

try
 {

 $db = new PDO($dsn, $uname, $pass);

 $db->setAttribute(PDO::ERRMODE_SILENT, PDO::ATTR_EMULATE_PREPARES);
 error_reporting(0);
 } catch (PDOException $ex)
 {
 echo "Database error:" . $ex->getMessage();
 }

// END ESTABLISHING CONNECTION - CONNECTION IS MADE.


 $factnr = "123" // or where-ever you get your input from.

 $query = "SELECT * FROM orderstatus WHERE factuurnummer = :factnr";

 $statement = $db->prepare($query);

 // The values you wish to put in.
 $statementInputs = array("factnr" => $factnr);

 $statement->execute($statementInputs);

 //Returns results as an associative array.
 $result = $statement->fetchAll(PDO::FETCH_ASSOC);

 $statement->closeCursor();

 //Shows array of results.
 print_r($result);
 ?>

$query = "SELECT * FROM orderstatus WHERE factuurnummer = :factnr"; Is the ":" correct here?? I tried with the : and replaced teh : for $ bot both dont work — Jeroen Broerse, Jul 05 '17 at 21:52
@JeroenBroerse I have added the way you should connect to the database, try it with that and see if it works! — MinistryOfChaps, Jul 05 '17 at 21:54

PHP MySQL Query Where x = $variable from function

4 Answers4