0

for my current project I'm building a simple QueryBuilder class. Everything works fine, except the parameter binding with mysqli's bindParam() method and I just can't find the error.

Code and procedure:

  • The request handler invokes following method from my User model:

    public function getPasshashByUsername($username) {
      $qb = new SelectQuery();
      $qb->select(['passhash'])
         ->from($this->sourceName)
         ->where('nickname', $username);
      $stmt = $qb->build();
      $resultset = $qb->execute($stmt);
      return $resultset->fetch_assoc()['passhash'] ?? null;
    }
    
  • For parts with potential parameters, the SelectQuery class works like this:

    public function where($col, $val) {
      $this->aQueryParts['where'] = $this->addCondition('WHERE',$col, $val);
      return $this;
    }
    
    private function addCondition($type, $col, $val) {
      $sExpr = $type . ' ';
      $sType = '';
      if ( is_int($val) || is_bool($val) ) {
        $sExpr .= '? = ' . $val;
        $sType .= 'i';
      } elseif ( is_double($val) ) {
        $sExpr .= '? = ' . $val;
        $sType .= 'd';
      } else {
        $sExpr .= '? LIKE \'' . $val . '\'';
        $sType .= 's';
      }
      $this->sTypes .= $sType;
      $this->aParams[] = $col;
      return $sExpr;
    }
    
  • Now the build() method comes in action, where the params will be bound:

    public function build() {
      if ( isset($this->aQueryParts['from']) ) {
      $sQuery = '';
      foreach ( $this->aQueryParts as $part ) {
        if ( is_string($part) ) {
          if ( !empty($part) ) {
            $sQuery .= $part . ' ';
          }
          continue;
        }
        if ( is_array($part) ) {
          foreach ( $part as $entry ) {
            if ( !empty($entry) ) {
              $sQuery .= $entry . ' ';
            }
          }
        }
      }
      $this->sQuery = $sQuery;
      // Marker 1
      //$this->sQuery = 'SELECT * FROM vwuser WHERE nickname LIKE \'TestUser\'';
      } else {
        Logger::send('You must at least call from() to get a valid query.');
        die();
      }
      $this->con = (new DatabaseAdapter())->connect();
      if ( !$stmt = $this->con->prepare($this->sQuery) ) {
        $msg = 'Error while preparing statement: "' . $this->sQuery . '"';
        Logger::send($msg);
        die();
      }
      // make params by-ref
      $params = [];
      foreach ( $this->aParams as $param ) {
        $params[] = &$param;
      }
      // Start Marker 2 
      if ( !empty($this->sTypes) ) {
        if ( !$stmt->bind_param($this->sTypes, ...$params) ) {
          $msg = 'Failed to bind parameters to Query: "' . $this->sQuery . '"';
          Logger::send($msg);
          die();
        }
      }
      // End Marker 2
      return $stmt;
    }
    

The execute() method just wraps the mysqli execute() in the usual way. No Error/Exception is thrown, it just returns an result set with no matches. Regarding to XDebug, these are the relevant values when calling bind_params():

$this->sQuery = "SELECT passhash FROM vwuser WHERE ? LIKE 'TestUser'";
$this->sParams = "s";
$this->aParams = [ "nickname" ];

If i uncomment hardcoded assignment of the query string in the line below "Marker 1" and comment out the block of Marker 2 (call of bind_params), everything works fine, so it seems like the connection itself is valid. I've inserted the "make by-ref" block after reading at php.net that bind_params requires references, but it didn't change anything.

Dharman
  • 30,962
  • 25
  • 85
  • 135
stuck1a
  • 13
  • 6
  • How could `'nickname'` ever match `'TestUser'`? These two are completely different strings. – Dharman Dec 25 '21 at 10:50
  • I don't understand why you do it with MySQLi. This would be just a couple of lines with PDO. – Dharman Dec 25 '21 at 10:53
  • @Dharman nickname is the col name in vwuser – stuck1a Dec 25 '21 at 10:55
  • It shall compare the value 'TestUser' against the real value of vwuser.nickname, like in the working query from marker 1 -> "SELECT passhash FROM vwuser WHERE nickname LIKE 'TestUser'" – stuck1a Dec 25 '21 at 11:00
  • Oh okay, I guess I confused something a little bit =D Time for a break – stuck1a Dec 25 '21 at 11:03
  • No, you can't bind column names. You can only bind data. So both 'nickname' and 'TestUser' are strings not column names – Dharman Dec 25 '21 at 11:11
  • This part doesn't make any sense `$sExpr .= '? LIKE \'' . $val . '\'';` – Dharman Dec 25 '21 at 11:12
  • Please don't put solutions in the question edits. If you want to share a solution then post an answer below – Dharman Dec 25 '21 at 11:22

2 Answers2

0

Here is the corrected version. Thanks @Dharman to give me the hint. The mistake was some confusion about the placement of the question mark in the line:

$sExpr .= '? LIKE \'' . $val . '\'';

which should be of course:

$sExpr .= $col . ' LIKE ?';

so the correction version of addCondition() is:

  private function addCondition($type, $col, $val) {
    $sExpr = $type . ' ';
    $sType = '';
    if ( is_int($val) || is_bool($val) ) {
      $sExpr .=  $col . ' = ?';
      $sType .= 'i';
    } elseif ( is_double($val) ) {
      $sExpr .=  $col . ' = ?';
      $sType .= 'd';
    } else {
      $sExpr .= $col . ' LIKE ?';
      $sType .= 's';
    }
    $this->sTypes .= $sType;
    $this->aParams[] = $val;
    return $sExpr;
  }
stuck1a
  • 13
  • 6
-1

You really should consider learning PDO instead of mysqli or using some existing database access library.

You can't bind column names. The whole point is that you bind the data. You are doing it the other way around. I would recommend to get rid of the method addCondition as it doesn't really make any sense and will be the cause of many bugs in your code. Instead just pass the arguments straight into bind_param and use str_repeat() for the types.

Here is how you could do it:

public function where($col, $val)
{
    $this->aQueryParts['where'] = 'WHERE '.$col. '=?';
    $this->aParams[] = $val;
    return $this;
}

public function build()
{
    if (isset($this->aQueryParts['from'])) {
        $sQuery = '';
        foreach ($this->aQueryParts as $part) {
            if (is_string($part)) {
                if (!empty($part)) {
                    $sQuery .= $part . ' ';
                }
                continue;
            }
            if (is_array($part)) {
                foreach ($part as $entry) {
                    if (!empty($entry)) {
                        $sQuery .= $entry . ' ';
                    }
                }
            }
        }
        $this->sQuery = $sQuery;
    // Marker 1
    //$this->sQuery = 'SELECT * FROM vwuser WHERE nickname LIKE \'TestUser\'';
    } else {
        Logger::send('You must at least call from() to get a valid query.');
        die();
    }

    $this->con = (new DatabaseAdapter())->connect();
    $stmt = $this->con->prepare($this->sQuery);
    // Start Marker 2
    if ($this->aParams) {
        $stmt->bind_param(str_repeat('s', count($this->aParams)), ...$this->aParams);
    }

    // End Marker 2
    return $stmt;
}

As you can see, I also removed the if statements. You really should consider enabling mysqli error reporting instead of manually checking for errors.

Dharman
  • 30,962
  • 25
  • 85
  • 135
  • Thank you for your suggested solution, I will think about it. This is only minimal exception handling in case the user turned off mysqli_error_reporting manually in the settings of DatabaseProvider. But I'm thinking about removing this possibility since it seems to don't give any benefits – stuck1a Dec 25 '21 at 11:34