The patch adds in an \s* to allow whitespace at the begining of the select statement.

how to reproduce the bug:
pager_query(" SELECT .. FROM .." [, ...]);

CommentFileSizeAuthor
pager.inc_.whitespace.patch538 bytesbigyellowvan

Comments

gerhard killesreiter’s picture

Status: Active » Closed (won't fix)

Adhere to the coding standards and you won't have whitespace before SELECT.

mooffie’s picture

1. Where is it written, in the handbooks, "You shall not commit a whitespace before your SELECT"? I've just searched.[1][2]

2. This 'issue' is the last in a long line of duplicate issues that show you the great demand for this "feature".

3. Even the great illuminato, chx, splits his SQLs (in menu.inc, book.inc) over several lines. And when doing so he also puts whitespace in front of his SELECTs --because you can't esthetically split SQL otherwise.

4. Not to mention two other Illuminati, Matt Westgate and John K. VanDyk, who, in their celebrated book, "Pro Drupal Development", freely put whitespace in front of their SELECTs (for the same reason chx does it) --while ignoring the bugs this causes at Drupal's DB layer.

5. Not to mention the bugs a sane, spaced formatting of SQL statements could have saved the Drupal community. Take for example the 400 characters long SQL statement in node_update_index(), which required "reverse engineering" (i.e., line splitting) before the errors could be seen.

6. Besides, the core Drupal developers themselves don't "adhere to the coding standards." They use SQL reserved words in their SQLs (see [2]).

[1] http://drupal.org/coding-standards
[2] http://drupal.org/node/2497

mooffie’s picture

Status: Closed (won't fix) » Active

You may "close"/"wontfix" this issue if you like, but please remember that programmers will always want their code to be easy to read --and spacing their long SQL statement is necessary for this to happen.

jp.stacey’s picture

Status: Active » Fixed

I think mooffie originally had a point, although he put it a bit heavily.

I can't see anything on those two links either, apart from a vague mention of "no trailing whitespace" in the indentation section, which as far as I can gather refers to whitespace prior to a module's opening <?php tag. That in itself seems reasonable as it's dictated by PHP's whitespace handling; but declaring that the same rule should apply to SQL without justification is a bit too ex-cathedra for me.

Also: "Note: The Drupal Coding Standards applies to code that is to become a part of Drupal." If they're going to affect the code people write on their computers without intending for it to become a part of Drupal, then they should be warned. So I don't see why we should squash this bug without consideration.

However... as far as I can see it's fixed in D5.9 and D6.3 . Here's the contents of the test module I ran on both just now:


 $q = "SELECT COUNT(*) as num, TYPE FROM node GROUP BY TYPE";

 echo "<p>Without leading space:</p>";
 $results = pager_query($q);
 while ($obj = db_fetch_object($results) ) {
   var_dump($obj);
 }

 echo "<p>With leading space:</p>";
 $results = pager_query(" " . $q);
 while ($obj = db_fetch_object($results) ) {
   var_dump($obj);
 }

 exit();

Both returned the query I expected - a count of a handful of test nodes.

So I'm closing this bug, but not because of the coding standards reasons cited in Gerhard's comment, as that would be a misapplication of them: rather, because probably someone else has seen it worth their while to fix it; maybe they considered it a bug too.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.