Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'll dbtngify the comment module. When i'm ready, i'll post a patch, i'm just creating the issue so nobody is doing redundant work.
Comment | File | Size | Author |
---|---|---|---|
#23 | comment_dbtng9.patch | 24.58 KB | jsaints |
#20 | comment_dbtng8.patch | 24.58 KB | jsaints |
#16 | comment_dbtng6.patch | 19.84 KB | jsaints |
#14 | comment_dbtng5.patch | 19.14 KB | jsaints |
#12 | comment_dbtng4.patch | 20.79 KB | CorniI |
Comments
Comment #1
Crell CreditAttribution: Crell commentedSweet, thanks! I look forward to seeing it.
Comment #2
CorniI CreditAttribution: CorniI commentedSo the first draft is open to review...
I hope there's not too much wrong :D
Comment #3
CorniI CreditAttribution: CorniI commentedWith some help form RobLoach less debug comments...
Comment #4
RobLoachThere are also some db_select statements that don't quite warrent the dynamic query builder, but they do work.......
But of course, it looks good to me!
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe conversion looks good, but still need some work on some points:
- there are some debugging stuff
- there are several code style issues, especially regarding spacing:
should be:
and
should be:
(full reference http://drupal.org/coding-standards)
- equality is the default operator for condition, so '=' can be removed in all those three examples:
- some queries don't have to be dynamic:
And I think we could omit the alias if equal to the name of the column.
Thanks for your great work on this!
Comment #6
RobLoachAddressed some of the issues brought up by DamZ at #5 and fixed some write space problems.
The last query you mentioned actually does need the dynamic builder because of the following if statement:
Comment #7
CorniI CreditAttribution: CorniI commented@RobLoach: sorry, your patch broke it completly, as addField doesn't return the actual query object but the field alias...
I scrapped it and started from my patch again with the hints by Damien.
@omitting the alias for addField: This would break all the code, as the default alias for a field is tablealias_tablefield, but all code needs just tablefield (the alias is the name in the result object, too).
@the unneeded '=' in conditions: I prefer having it there and actually seeing it, I know it can be ommitted. Maybe someone other can have a 3rd opinion (Crell?)
Most if not all from the whitespace issues should be fixed now, I reformatted some of the queries, too.
Comment #8
Crell CreditAttribution: Crell commentedFor convention I'd prefer to not have the '=' specified explicitly, as it is the default and I can't see that changing. We generally don't specify defaults we don't have to. I don't care strongly enough to hold back the entire patch for it, though. webchick may. :-)
Re the alias for addField(), Cornil is correct that the default is $table_$field, to help ensure uniqueness. You could arguably accept the default and then use the returned value as the field, as the unit tests do, if you wanted. The odds of that making a difference here are extremely remote, however, so I don't care much either way.
I'll try to give this a thorough review this weekend.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commented@Crell: wouldn't it make sense to change the default
addField()
to default to$field
, which is more inline with what most of core does?Comment #10
CorniI CreditAttribution: CorniI commentedI'll remove the '='s today, but imho this shouldn't stop the review. Imho, the default alias of $table_$field isn't so user-friendly as you see eg from my code, because elsewhere just the field name and not $table_$field is used, so defaulting to $field, then $table_$field and then to whatever you want makes sense (too me...)
Comment #11
Crell CreditAttribution: Crell commentedLet's discuss the alias over here: #316868: Default SelectQuery::addField() alias to $field, not $table_$field
Comment #12
CorniI CreditAttribution: CorniI commentedSorry for beeing late, but finally here it is :)
Comment #13
Crell CreditAttribution: Crell commentedGr. Several broken hunks. :-(
Comment #14
jsaints CreditAttribution: jsaints commentedRe-roll patch. It now applies cleanly against HEAD and passes all tests.
I also fixed line in the patch that caused line 288 of comment.test to fail.
Error was in the patch at line:
Comment #15
Crell CreditAttribution: Crell commentedcomment.module:
line 320, that query doesn't need to be dynamic. It can just be a a db_query_range() call. Also, the foreach() loop is unnecessary. Just call ->getCol() on the result object, which can be chained directly on the db_query_range() call.
Line 337 and following, those lines need to be indended within the if statement. Also, you can now just use fields() rather than addField to add multiple fields from a single table.
Line 385, as discussed with webchick we can break long queries across multiple lines in the same string, like so:
Line 387, that query has multiple arguments so the argument array should be broken across multiple lines.
Lines 620, 621, 660, 661, I believe webchick wanted those broken across multiple lines, even if they're short. Anything longer than one chained method should be multi-line.
Line 760, break argument array across multiple lines.
Line 979 and following, use fields() there instead of addField(). Much easier to read, and that's I think the reason that fields() was added. :-) (See #11 above.)
Line 1016 and following, there's 2 unconverted dynamic queries there. It looks like they use db_rewrite_sql and pager. Bah. Until we get pager queries updated to the new API it's impossible to convert that properly. Please leave a TODO on them in the code, with a link to the appropriate issue ("Extenders" and the db_rewrite_sql issues) so that we know to fix it later.
Line 1155, argument array should be broken out across multiple lines.
Line 1182 and 1564, the array( should not be on its own line. It should be on the same line as the db_query( portion. Same applies to the various places above that need their args broken out.
Line 1188, the } should be on its own line. Looks like an accidental backspace to me. :-)
Line 1225, those chained methods should be on their own lines.
Lines 1915, 1930, array( should not be on its own line.
The function comment_operations() is unconverted, and it's weird enough that I'm not entirely sure how to convert it properly. :-)
comment.pages.inc: I think this file was skipped entirely. It only has a few queries so let's go ahead and do it while we're at it. Note line 125, which calls the weirdness that is comment_operations().
comment.admin.inc: Ibid. Let's convert this while we're here.
Comment #16
jsaints CreditAttribution: jsaints commentedI rolled a new patch with the changes listed above, except for the queries relating to
comment_operations()
. All tests pass. This is progress, but its not complete.Here are some thoughts and questions on the changes I made:
db_query_range()
or something like$query->range(5, 10)
?$query->addField('u', 'name', 'registered_name');
comment_operations()
. Perhaps we could get some input from the comment.module author as to how we might best go about refactoring for DBTNG here. I will do the work, just need some input from the group.Its getting close. Thanks.
Comment #17
cburschkaComment #18
Crell CreditAttribution: Crell commented#1: db_query_range() is fine to use there, and probably a bit faster than db_select()->range().
#2: Great. That's exactly how it's supposed to work. :-)
#3: I don't know that there is a single author of comment.module anymore, really. I suggest posting to the dev list with a link here and a request for help from anyone with strong comment.module-fu.
Comment #19
jsaints CreditAttribution: jsaints commentedOn my run this morning , I thought of a clean way to do the update to DBTNG that will not require changing comment_operations().
This way we can talk about comment_operations() in a separate thread and patch if we want.
I will work on this today and have the patch soon.
Comment #20
jsaints CreditAttribution: jsaints commentedChanges are made to comment_operations(). Made some other touchups.
All tests pass. It now applies to HEAD cleanly. This is ready for review.
Comment #21
cburschkaJust going over that... am I reading it wrong, or are you executing the same query twice here?
Comment #22
cburschkaComments need to end in a period, as far as I know.
Comment #23
jsaints CreditAttribution: jsaints commentedGood catch. Here is a new one.
Thanks so much for your feedback.
Comment #24
Dries CreditAttribution: Dries commentedI gave this a good review and it certainly looked good to me. Additional improvements can go in a follow-up patch. Thanks all.
If this break the comment module, please submit a test with your fix.
Comment #26
Crell CreditAttribution: Crell commentedTagging.