Hacker News Reader

Comment by davidw | Hacker News Reader

> And inline SQL is non-forgivable

What do PHP folk typically do for a big, hairy query that isn't really doable via an ORM? Not that this is the case here, just curious. I haven't touched PHP for years.

View on HN

Oops, "inline SQL" is probably a wrong way of putting it. Actually, even in Symfony2 which has an ORM (Doctrine), I sometimes find myself writing SQL queries in code because Doctrine can be so inefficient in some situations. Writing pure SQL makes your code less portable (i.e. tied to a specific database, in this case MySQL). Now I don't think its bad at all to have somewhat complex group/(sub)queries written this way if portability is not a big concern, and if the number of those queries is not as big, it is something you just need to be aware of.

What we're actually talking about here which is really bad is string concatenation without parameterization.

Meaning, it seems the code is not using a database driver. PDO has been the accepted way of doing this. You pass the parameters to PDO separately from the query for sanitation. We can all see why this is bad:

     if (isset($_GET['limit'])) {
         $limit = (int)$_GET['limit'];
         if ($limit < 1) $limit = 1;
         if ($limit > 10000) $limit = 10000;
      }
 $req = 'SELECT * FROM `Money_Bitcoin_Node` WHERE `Status` = \'up\' AND `Last_Checked` > DATE_SUB(NOW(), INTERVAL 6 HOUR) AND `Version` >= 31500 AND (`Last_Down` IS NULL OR `Last_Down` < DATE_SUB(NOW(), INTERVAL 2 WEEK)) AND `First_Seen` < DATE_SUB(NOW(), INTERVAL 2 WEEK) ORDER BY RAND() LIMIT '.$limit;

Exactly. Parameterization is #1 priority. Second you could also move the statement string itself to a separate resource (disk file), which helps for maintainability.

so... the statement above refactored to slightly better pseudo-code:

    $limit = getLimitParameter();
    $sqlString = SqlResourceLoader::Load("BitCoinNodeSelect.sql");
    $statement = $db->prepare($sqlString);
    $statement->bindValue(1, $limit, PDO::PARAM_INT);
    $statement->execute();

..or the query with the SQL along with parameter parsing logic could be contained in a separate class altogether.

Excuse me for asking, how saving the SQL query in a file helps maintainability?

Storing the query in a file means an additional IO call, right? Isn't the better approach to wrap the sql statement in a function if it's about to be reused?

I'm not arguing, I'd like to know the point and learn why saving that into a file helps maintenance.

Scenario: You're asked to rewrite SQL for UseCase#14 because it's slow. I'd argue the developer find it more quickly if it's stored in a separate .SQL file with a file name related to the use case (discoverability). It would also separate better in source control history as you'd see when you did SQL changes and when you did PHP changes. Could be valuable when bug-hunting. edit: And the obvious: instead of scrolling to line 762, column 18 of PHP file X, you open file Y on line 1, column 1. It's also an .SQL file, which tells an IDE to syntax highlight it as SQL and not PHP.

Yes, there's a performance cost. Hard disks are fast, though. And then there's caching. I always value maintanability higher than performance, as maintainability often is correlated with number of bugs.

You suggest to wrap the SQL statement in a function, and that might be just as fine. There's no single best solution here. Just a lot of solutions that are better than the Bitcoin-class ;-)

Hopefully the SqlResourceLoader would do some caching.

Ok, so just parametrized queries, not the actual lack of SQL in the source code. That makes a bit more sense.

I feel as though my head is about to explode.

What is wrong with stored procedures?

The typical argument against stored procedures is they are opaque, distant from the rest of the code, and difficult to manage under source control.

I'm sure there are good refutations to each, but these are commonly cited.

I'm not arguing against stored procedures, but for input sanitation and separation of SQL and PHP-code. Pseudo-code above was just an example of better code than OP's.

This solution is just reimplementing stored procedures in a less efficient way. If you want to keep the SQL separate, stick it in a stored procedure.

It's always better to use prepared statements, since it prevents SQL injection attacks. If you do not use neither an ORM or prepared statements, you either have to with trusted input, or need to sanitize that input really well (to the point it's often easier to just use prepared statements).

Personally, I'd extract it into its own .sql file; if your query is complex, it should be treated as code.

Reply on news.ycombinator.com