Posted by EclipseGc on August 31, 2008 at 12:32pm
10 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | blog.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave.Ingram |
| Status: | closed (fixed) |
| Issue tags: | DBTNG Conversion, dc2009 code sprint |
Issue Summary
OK, I think I've taken care of the necessary updates for now. Please review and let me know.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| blog.patch | 3.29 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
Patch failed to apply. More information can be found at http://testing.drupal.org/node/13930. If you need help with creating patches please look at http://drupal.org/patch/create
#2
One more time
#3
Unfortunatly it breaks blog API tests. See the attached picture.
#4
#5
Actually BlogAPI appears to break w/ or w/o this code. Please verify. (also turn clean URLS on it'll run faster)
#6
Hmmz this is strange. I just did a new checkout and all the tests passed. Then I applied your patch and it still passed :S
#7
The dblog test is failing because of the function _blog_post_exists. You typecast a object to boolean here._blog_post_exists
#8
BlogAPI weirdness was here, now passes in head: http://drupal.org/node/297860
(this is why all tests should always pass).
#9
The last submitted patch failed testing.
#10
Working on this as of the DrupalCon DC Code sprint... patch coming soon
#11
#12
OK. So all db_query functions and their counterparts have been converted to the new dynamic db_select syntax throughout the module. As a part of this I also rewrote node_title_list() to use a foreach loop instead of a while loop, as suggested by Crell.
Additionally, two chunks of code in blog.pages.inc could be shortened by simply doing a fetchCol() on the query instead of fetching a result and then looping through the results.
#13
Do not use dynamic queries if you don't need dynamic behavior. It looks like you're converting all SELECT queries to dynamic, which is just a performance hit.
#14
OK. What's the test for me to know which queries are dynamic and which are static?
#15
First try writing the query as a static. If you find that you need to use one of:
- String concatenation
- Paging
- Tablesort
- Node access restrictions
Then stop and make it dynamic instead.
That's not a perfect rule, but that's a good guideline. The vast majority of queries will be static.
#16
I would add to that ruleset: If you think there is a significant likelyhood that sites will want to alter the query to achieve custom needs, use db_select(). Static queries are not alterable.
#17
Also, if a query currently uses db_rewrite_sql() then you must include '->addTag('node_access')'. Thats missing in a few places.
#18
Reroll.. there were no candicates for static queries, just added addTag() a few times and changed the extend() coding style according to #425872: DX problem with DB extenders. I also "removed" the node_title_list() change from the patch... I think it's easier to convert module per module.
#19
#20
The last submitted patch failed testing.
#21
Patch chained a addExpression(1) call... replaced by fields('n', array('nid')).
#22
#23
Looks good, and all tests pass. Committed to CVS HEAD.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.