Pager.inc regexp misses whitespace use case

Matching that last space is, in my opinion, a waste of precious bytes.

Attached patch simply removes the space from after the FROM statement, from the regexp pattern and replacement, to allow a more structured style (see below). This fixes the pager counter, which would otherwise fail and return garbage page numbers.

$query = 'SELECT
my_field
FROM
my_table';

Hm. As I see it, this would fail on:

SELECT NID, VID, FROMAGE FROM {cheese}

Here's a second attempt at solving this issue, now matching pretty much any acceptable character. I also added this character class to between SELECT and FROM, so that it also will not break, as HEAD currently does, on following query:

SELECT NID, VID, AGEFROM FROM {cheese}

Because of the similarity in characteristic of below issues, I also added the trim() method to the query, to solve them as well.
#163118: pager_query() misses multi.line SELECT\tab\FROM\tab.. SQL-Strings
#110093: pager_query does not work when there is whitespace in query

Because the new database patch will change that part of the code extensively, there is no point in fixing this in D7, so the version here is correct.

Please take into account Gabor concerns in http://drupal.org/node/163118#comment-620588 ("Why do we restrict the ORDER BY to work with spaces only?").

Were duplicates:

#163118: pager_query() misses multi.line SELECT\tab\FROM\tab.. SQL-Strings
#110093: pager_query does not work when there is whitespace in query

Made the ORDER BY part also work with all sorts of whitespaces.

This should be bumped to D7 (applies cleanly there). Looks good to me.

#39302: multi-line queries failure was a duplicate, even if much older.

The old #39302: multi-line queries failure issue of which this one is a duplicate (and not the other way round) also raised another question: Killes wrote there that multi-line queries were not allowed by the coding standards, even if this is now fixed by the proposed patch.

If there is indeed a policy about not allowing multi-line queries, it should be added to the SQL coding conventions page which currently doesn't list such a requirement. Otherwise, that same page should probably be updated with a recommended formatting for long queries, even if only to say the single-line is recommended, as seems to be the case in core.

@fgm: that's a different issue. As far as I know, multiline queries *are* allowed (there are even some in core), and the SQL coding conventions made no reference to that issue. The current practice is to avoid multi-line queries when there are no readability issues.

This sounds quite logical, but is not in agreement with what Killes wrote in #39302, so it would be better if the practice you describe was indeed codified. The multiline query which used to be in book.module is no longer there in D7, and I'm not sure there are others in core, so it's all the more important to have a proper documentation.

Not sure. I'd rather not make those regex slower.

How about skipping regexp all together by simply enclosing the actual query in a sub select when creating the counter query. Something like $counter = 'SELECT COUNT(*) FROM (' . $query . ') counter';. That'd make it faster and make it handle any query. I can't see any downside to this method either - maybe you can?

A regex is the Wrong Way(tm) to be dealing with SQL. Instead we should be moving pager query to the new dynamic query builder. That requires first implementing #299267: Add "Extender" support to SELECT query builder. Let's get back to this after that goes in so we can do this right.

(Any place we're regexing a query needs to become a dynamic query, without exception.)

A regex is the Wrong Way(tm) to be dealing with SQL. Instead we should be moving pager query to the new dynamic query builder. That requires first implementing #299267: Add "Extender" support to SELECT query builder. Let's get back to this after that goes in so we can do this right.

(Any place we're regexing a query needs to become a dynamic query, without exception.)

Should this go back to 'code needs work' then? :)

Yeah, probably. :-) I'm tempted to go postponed, but I am not 100% certain of how the dynamic pager builder will work yet.

Also, multi-line queries not allowed? That's nonsense. If that's a current standard that needs to change, as any query of remotely interesting complexity is going to go well past 80 characters anyway.

It seems that pager.inc#PagerDefault no longer includes any regex use, so the issue seems moot for D7 ?

Regarding the multi-line coding style question, which should be less frequent anyway with DBTNG query builders, the SQL coding style still does not explicitly allow or disallow multi-line queries, but the "Indentation" paragraph includes a multi-line example, so http://drupal.org/node/39302#comment-359323 is probably no longer applicable in D7 either.

There's no more regex, so this is a non-issue for Drupal 7. Drupal 6, that's Gabor's call.