At DrupalCon SF, for the first time someone pointed out that we don't support comments in built queries and there's no way to hack them in. However, they can be useful for debugging because they give you something in the log file to look for.

We should be able to implement DB-specific query comments (one comment per query only) without breaking any existing functionality. The syntax is "-- comment here" at the end of a query on any DB, or "/* comment here */" at the beginning of the query for MySQL.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

http://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=/co...

Double hyphen ( -- ) complies with the ANSI/ISO standard for SQL.
C-style slash-and-asterisk ( /* . . . */ ) comply with the SQL-99 standard.

both should work.

Crell’s picture

Webchick said in IRC that this is OK, so let's do.

cafuego’s picture

FileSize
3.95 KB

Ugh, if they enabled ascii logging in mysql they have bigger issues to worry about ;-) More important is that you can easily spot these queries in 'SHOW FULL PROCESSLIST()' ;-)

Anyway. I was twiddling with #778050 yesterday and snuck in comments on the SelectQuery_mysql as well. Attached is the patch that does this. Is that along the lines of how you were thinking of implementing this?

... /* comment here */" at the beginning of the query for MySQL.

This works anywhere in the query for both MySQL and for SQLite, not just at the start. See

SELECT nid, /* wibbles */ title FROM node;

And I'm told by an Oracle employee that's adding in comments like this is how you can affect the query plan on Oracle - so it also supports it, though more like query options than comments.

Berdir’s picture

I guess so...

- I don't think this is MySQL specific, so it should be enough to simply add it to the base toString() method (It is part of the SQL-99 standard and if a dbms does not support it, they can always override it and ignore comments or add them in a way that is supported by their dbms)
- It also needs a private $comments definition and a addComment() method...

Crell’s picture

Comments need to be supported cross-DB, and included in the SelectQueryInterface definition. Where the comment gets placed in the query string can vary with the DB driver if appropriate, but the API has to be consistent across all DBs.

Berdir’s picture

That's what I meant, yes :)

cafuego’s picture

Why only the SelectQueryDefinition and not all queries? It seems a bit arbitrary to limit it thus.

Crell’s picture

Hm, you're right. It should be part of the Query class. (Which really ought to have been a QueryInterface, but can't really change that now. Ah well. :-) )

cafuego’s picture

Status: Active » Needs review
FileSize
9.15 KB

Right, let's see what the test bot makes of this then.

I've added a private array on the Query class and all __toString() methods now prepend it to their queries.

Crell’s picture

addComment() should return $this so that it's chainable.

getComments() should return by reference, like the other getX methods, for consistency.

Otherwise this is looking pretty good.

Crell’s picture

Status: Needs review » Needs work
cafuego’s picture

Status: Needs work » Needs review
FileSize
10.25 KB

Updated as per suggestions.

Status: Needs review » Needs work

The last submitted patch, 785782-1.patch, failed testing.

cafuego’s picture

Status: Needs work » Needs review
FileSize
8.23 KB

Failfuego forgot to fix the patch path.

Crell’s picture

Status: Needs review » Needs work

Failfuego uploaded the wrong patch, too. :-) (This is the pager optimization patch.)

cafuego’s picture

FileSize
9.93 KB

Hahaha :-) Too many patch files starting with 7* in that dir. Let's try this one instead.

cafuego’s picture

Status: Needs work » Needs review
cafuego’s picture

Right, that now works. Would it be worthwhile writing a test to make sure it keeps working?

Crell’s picture

Issue tags: +Needs tests

Yes, tests++.

Also, the docblock for the $comments property should include an @var array declaration. (See other @var declarations in that file.)

Thanks, cafuego!

cafuego’s picture

FileSize
17.99 KB

Right. My first tests ever, so I hope I did it right. I just copied, pasted and renamed existing test functions in their various classes and stuck in a comment (or two). Do they just get executed by virtue of existing?

I *also* added $query->comment() as a wrapper for addComment(), so the camelCase doesn't look awful and out of place in chained queries.

Crell’s picture

Status: Needs review » Needs work

The docblock on addComment() is more than one line, which is a documentation bug. :-) The second sentence is not necessary. The @return statement is sufficient. I think the standard elsewhere for chaining is "the called object".

We do not want to have both comment() and addComment(). That's needlessly confusing. Let's just pick one. I would also somewhat prefer comment() as most of the method calls dont' have camels yet, although there's nothing wrong with camels.

For the tests, I don't think we need to redo the test on every query type. Rather, we want to pick one query type (probably update), add a comment on it, run the query and make sure it works, and then also cast the query to a string (which is only safe to do a second time on some queries, like update) and confirm that the comment is actually in the query string.

Does that make sense?

cafuego’s picture

Yup, I reckon I can manage that.

cafuego’s picture

Status: Needs work » Needs review
FileSize
11.31 KB

Ok, done. I went with comment() and comments() as set and retrieve method names. Turns out that there's no problem having a variable and a method sharing a name. Makes it nice and consistent.

The test function does a select and makes sure that the comment is embedded *and* that the rest of the query is there. The newlines around "FROM" had me tricked for a moment - why does a SelectQuery insert them?

Updated the comment() docblock to only have one line.

Status: Needs review » Needs work

The last submitted patch, 785782-3.patch, failed testing.

Dries’s picture

Status: Needs work » Needs review
+++ includes/database/query.inc	2010-05-13 17:17:58.285537105 +1000
@@ -262,6 +269,39 @@
+  /**
+   * Adds a comment to the comments array for the query.
+   *
+   * @param $comment
+   *   The comment string to be inserted into the query.
+   * @return Query
+   *   The called object.
+   */
+  public function comment($comment) {
+    $this->comments[] = $comment;
+    return $this;
+  }

I think it would be good to explain why comments are useful (or when they should be used).

+++ includes/database/query.inc	2010-05-13 17:17:58.285537105 +1000
@@ -262,6 +269,39 @@
+  public function &comments() {
+    return $this->comments;
+  }

Should this be getComments() and setComment()? In general, having explicit setXXX and getXXX functions is a good idea but it has to fit the rest of the DBTNG API of course.

cafuego’s picture

Should this be getComments() and setComment()? In general, having explicit setXXX and getXXX functions is a good idea but it has to fit the rest of the DBTNG API of course.

Next up, Dries v. Crell in a 4 round match! ;-)

The API isn't consistent as things stand, but all chained DB calls seem to only use the lowercase methods as far as I've seen. Some of them are wrappers
though I think. eg field() calls addField... and there is addExpression too. That's why I initially did camelCase and provided a wrapper mehod.

I'm happy to go either way, but I would
like a final decision first :-)

Berdir’s picture

Hm.

Yes, we are definitly inconsistent here...

- Most methods that are chainable don't have a set
- Most methods that are not have a set
- Both these rules have exceptions
- Get functions have a get prefix usually...

Yes, you had both, but both were chainable. In contrast, fields() is different than addField() and the latter is not chainable so that makes sense (somewhat... ;)).

My suggestion would be just comment() and getComments().

Maybe we can make a rule of the first two points in D8?

Crell’s picture

$query->comment($comment_to_add);

$query->getComments(); // Return comment array by reference.

That's most consistent with the pattern in SelectQuery, which is the only one with getX methods right now. We'll use that.

@Dries: These are not Bean-style getters and setters, so the common Java conventions for those do not apply. (They're also rather silly as I've never actually had a use for Bean-style classes in PHP.)

Also, I don't think SelectQuery is going to work as the sample query here because it's not reusable. The alternative is to use query logging to get the compiled query string, I suppose, but that's a little more involved.

Dries’s picture

That's fair. I still think we need to improve the documentation of those functions.

cafuego’s picture

FileSize
11.62 KB

Right then, the next revision.

- comments() replaced with getComments()
- Fixed the test, so it actually passes
- Added documentation in the comment() function doc block.

Should that explanatory documentation go in the doc block for the variable instead? Or in all three associated doc blocks?

Dries’s picture

+++ includes/database/query.inc	2010-05-14 13:00:39.235934894 +1000
@@ -262,6 +269,44 @@
+   * Because this method returns by reference, alter hooks may edit the comments
+   * array directly to make their changes. If just adding comments, however, the
+   * use of comment() is preferred.

The sentence 'If just adding ...' doesn't seem to be relevant at all?

Crell’s picture

Status: Needs review » Needs work

The "If just adding" language is used by all of the SelectQuery getX methods, too. It's a warning that people really shouldn't use those unless they're doing a query_alter. So that's fine.

"Adds a comment to the comments array for the query." - That, however, tells the caller too much about the implementation. :-) "Adds a comment to the query." is all we need here. That it's stored internally as an array is an implementation detail.

Other than that, I think this is good to go. cafeugo, can you fix that comment and then we'll RTBC? (yay!)

cafuego’s picture

Status: Needs work » Needs review
FileSize
11.6 KB

Fully yay! Then I can get back to the query flags patch :-)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

As soon as the bot green-lights it.

Crell’s picture

Issue tags: -Needs tests

Removing tag, as there is a test.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright, works for me! Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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