Posted by Crell on April 29, 2010 at 10:45pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | task |
| Priority: | normal |
| Assigned: | Crell |
| Status: | closed (fixed) |
Issue Summary
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.
Comments
#1
http://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=/co...
both should work.
#2
Webchick said in IRC that this is OK, so let's do.
#3
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?
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.
#4
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...
#5
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.
#6
That's what I meant, yes :)
#7
Why only the SelectQueryDefinition and not all queries? It seems a bit arbitrary to limit it thus.
#8
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. :-) )
#9
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.
#10
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.
#11
#12
Updated as per suggestions.
#13
The last submitted patch, 785782-1.patch, failed testing.
#14
Failfuego forgot to fix the patch path.
#15
Failfuego uploaded the wrong patch, too. :-) (This is the pager optimization patch.)
#16
Hahaha :-) Too many patch files starting with 7* in that dir. Let's try this one instead.
#17
#18
Right, that now works. Would it be worthwhile writing a test to make sure it keeps working?
#19
Yes, tests++.
Also, the docblock for the $comments property should include an @var array declaration. (See other @var declarations in that file.)
Thanks, cafuego!
#20
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.
#21
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?
#22
Yup, I reckon I can manage that.
#23
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.
#24
The last submitted patch, 785782-3.patch, failed testing.
#25
+++ 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.
#26
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 :-)
#27
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?
#28
$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.
#29
That's fair. I still think we need to improve the documentation of those functions.
#30
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?
#31
+++ 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?
#32
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!)
#33
Fully yay! Then I can get back to the query flags patch :-)
#34
As soon as the bot green-lights it.
#35
Removing tag, as there is a test.
#36
Alright, works for me! Committed to CVS HEAD. Thanks.
#37
Automatically closed -- issue fixed for 2 weeks with no activity.