Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#33 | 785782-5.patch | 11.6 KB | cafuego |
#30 | 785782-4.patch | 11.62 KB | cafuego |
#23 | 785782-3.patch | 11.31 KB | cafuego |
#20 | 785782-2.patch | 17.99 KB | cafuego |
#16 | 785782-1.patch | 9.93 KB | cafuego |
Comments
Comment #1
chx CreditAttribution: chx commentedhttp://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=/co...
both should work.
Comment #2
Crell CreditAttribution: Crell commentedWebchick said in IRC that this is OK, so let's do.
Comment #3
cafuego CreditAttribution: cafuego commentedUgh, 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
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.
Comment #4
BerdirI 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...
Comment #5
Crell CreditAttribution: Crell commentedComments 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.
Comment #6
BerdirThat's what I meant, yes :)
Comment #7
cafuego CreditAttribution: cafuego commentedWhy only the SelectQueryDefinition and not all queries? It seems a bit arbitrary to limit it thus.
Comment #8
Crell CreditAttribution: Crell commentedHm, 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. :-) )
Comment #9
cafuego CreditAttribution: cafuego commentedRight, 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.
Comment #10
Crell CreditAttribution: Crell commentedaddComment() 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.
Comment #11
Crell CreditAttribution: Crell commentedComment #12
cafuego CreditAttribution: cafuego commentedUpdated as per suggestions.
Comment #14
cafuego CreditAttribution: cafuego commentedFailfuego forgot to fix the patch path.
Comment #15
Crell CreditAttribution: Crell commentedFailfuego uploaded the wrong patch, too. :-) (This is the pager optimization patch.)
Comment #16
cafuego CreditAttribution: cafuego commentedHahaha :-) Too many patch files starting with 7* in that dir. Let's try this one instead.
Comment #17
cafuego CreditAttribution: cafuego commentedComment #18
cafuego CreditAttribution: cafuego commentedRight, that now works. Would it be worthwhile writing a test to make sure it keeps working?
Comment #19
Crell CreditAttribution: Crell commentedYes, tests++.
Also, the docblock for the $comments property should include an @var array declaration. (See other @var declarations in that file.)
Thanks, cafuego!
Comment #20
cafuego CreditAttribution: cafuego commentedRight. 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.
Comment #21
Crell CreditAttribution: Crell commentedThe 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?
Comment #22
cafuego CreditAttribution: cafuego commentedYup, I reckon I can manage that.
Comment #23
cafuego CreditAttribution: cafuego commentedOk, 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.
Comment #25
Dries CreditAttribution: Dries commentedI think it would be good to explain why comments are useful (or when they should be used).
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.
Comment #26
cafuego CreditAttribution: cafuego commentedShould 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 :-)
Comment #27
BerdirHm.
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?
Comment #28
Crell CreditAttribution: Crell commented$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.
Comment #29
Dries CreditAttribution: Dries commentedThat's fair. I still think we need to improve the documentation of those functions.
Comment #30
cafuego CreditAttribution: cafuego commentedRight 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?
Comment #31
Dries CreditAttribution: Dries commentedThe sentence 'If just adding ...' doesn't seem to be relevant at all?
Comment #32
Crell CreditAttribution: Crell commentedThe "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!)
Comment #33
cafuego CreditAttribution: cafuego commentedFully yay! Then I can get back to the query flags patch :-)
Comment #34
Crell CreditAttribution: Crell commentedAs soon as the bot green-lights it.
Comment #35
Crell CreditAttribution: Crell commentedRemoving tag, as there is a test.
Comment #36
Dries CreditAttribution: Dries commentedAlright, works for me! Committed to CVS HEAD. Thanks.