0

I have this code so far

// Starts the transaction
self::$database->startTransaction();
    try {

        $sql  = "SELECT playerId FROM players WHERE name=?";
        $stmt = self::getConnection()->prepare($sql);
        $stmt->bind_param('s', $playerName);
        foreach ($playerNames as $key => $playerName) {
            $stmt->execute();
            $stmt->bind_result($playerId);
            $stmt->fetch();
            echo $playerId . "<br>";
        }

        // commits the transaction
        self::$database->commit();
    } catch (Exception $e) {
        self::$database->rollback();
        throw new Exception(__METHOD__." | ".$e->getMessage());
    }

The array $playerNames contains the names of the players, e.g.

array('Player1', 'Player2', 'player3')

The code from above should select the playerId of those players from the database. I have some issues:

  1. It just returns the last playerId (in this case the Id of 'player3'). I don't know why.

  2. I use a foreach-loop to execute(). is this bad for the performance, if there were hundreds of names in the array?

  3. In generell: Is this the correct approach for SELECTing or INSERTing stuff from or into a database?

I read this question: How can I prevent SQL injection in PHP?

But it didn't really work because of this:

$result = $stmt->get_result();
while ($row = $result->fetch_assoc()) {
    // do something with $row
}

I get an error with the getResult()-method. It says, the method doesn't exist. I think my webspace doesn't support it and I can't install it manually. So I need to stick with fetch().

Or might it have other reasons?

Community
  • 1
  • 1
SwiftedMind
  • 3,701
  • 3
  • 29
  • 63
  • why do you think there will be hundreds of names? – Your Common Sense May 25 '14 at 12:19
  • I don't know if there will be hundreds of names. Just in case it would be that many, I wanted to ask if there is a better way to improve performance. – SwiftedMind May 25 '14 at 12:27
  • @Your Common Sense: I can't really post **all** of the code. That would be to much. The only thing missing is the database-class which just connects to the database and `self::getConnection()` just returns the connection to the database. Nothing more – SwiftedMind May 25 '14 at 12:29
  • Seems like you are familiar with php, so why don't you just do a step by step deubgging with echo, print_r, var_dump and exit, to find the origin of this issue? should only take a few minutes – Nikita 웃 May 25 '14 at 12:33
  • 1
    The better way to improve performance is apparently to *avoid* situations where you need to select hundreds of names one by one. – Your Common Sense May 25 '14 at 12:35
  • you're `execute`-ing the query without using the results of that execution. of course it's only going to have the last value. – serakfalcon May 25 '14 at 13:00
  • Wait a second. That's not fair! I **did** look up the stuff before asking this. I even wrote it! Sorry, but I think this is exaggerated. – SwiftedMind May 25 '14 at 13:25
  • @YourCommonSense what's help-vampire mode? :p – Nikita 웃 May 25 '14 at 14:06

1 Answers1

2
    $sql  = "SELECT playerId FROM players WHERE name=?";
    $stmt = self::getConnection()->prepare($sql);
    $stmt->bind_param('s', $playerName);
    $stmt->bind_result($playerId);
    foreach ($playerNames as $key => $playerName) {
        $stmt->execute();
        $stmt->fetch();
        echo $playerId . "<br>";
    }
  1. You are fetching results of only last execute
  2. Running long loops is apparently bad for performance. Try to avoid them.
  3. Yes, in general.
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Thank you, it worked. But now I embedded it in my Code and now I get an error like: 'Commands out of sync; you can't run this command now' Because I started a transaction and commited it. If I delete the `commit()` - it works fine. I think it doesn't work because he can't commit more than one query at the time. But how can I fix this? Or shall I post another question? – SwiftedMind May 25 '14 at 13:13
  • You're right :) I just can add `$stmt->store_result();` into the loop – SwiftedMind May 25 '14 at 13:21
  • 1
    It suffices to bind the result once. – Gumbo May 25 '14 at 15:47