Closed (fixed)
Project:
Drupal core
Version:
5.x-dev
Component:
page.module
Priority:
Minor
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
30 Jul 2007 at 15:34 UTC
Updated:
15 Aug 2008 at 19:12 UTC
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 .." [, ...]);
| Comment | File | Size | Author |
|---|---|---|---|
| pager.inc_.whitespace.patch | 538 bytes | bigyellowvan |
Comments
Comment #1
gerhard killesreiter commentedAdhere to the coding standards and you won't have whitespace before SELECT.
Comment #2
mooffie commented1. 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
Comment #3
mooffie commentedYou 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.
Comment #4
jp.stacey commentedI 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:
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.
Comment #5
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.