DBTNG: Node module
Crell - March 7, 2009 - 19:45
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | DBTNG Conversion |
Description
This is going to be non-small, I think.
#1
I can confirm, this is non-small :)
Attached is a first patch.
There are some tricky parts, for example node_access.. funny to see that I re-implemented it the exact same way it was already done for node_query_node_access_alter.
#2
#3
node.api.inc:
- In hook_node_load(), there's no need for that query to use db_select(). If you pass an array in as a value in the placeholders array for db_query(), the DB layer will automatically convert it to the proper placeholders for you for use in an IN clause. It's cool like that. :-)
node.module
- In node_last_viewed(), use fetchObject() explicitly rather than fetch(). It is more self-documenting that way.
- You're somewhat inconsistent in how you handle single-value fields() calls. In some places you break that out so that there's just a single key/value on one line, in others you just inline it on the fields() line. For single-value arrays I think it's preferable to just inline them. Please check to make sure you do so consistently.
- node_page_view() has a db_select() in it that I don't think needs to be. db_query_range() still works the same as before, and there's no tagging, so using a static query will be faster.
- node_access_rebuild(): I'm curious why you're using this:
<?php$result = db_query("SELECT nid FROM {node}", array(), array('fetch' => PDO::FETCH_COLUMN));
?>
Instead of fetchCol(). and then iterating the result. It works, I suppose, but I've not seen that idiom used anywhere else in Drupal. (Depending on your reasoning maybe we should, but we don't right now.:-) )
- _node_access_rebuild_batch_oper(), again I don't think the db_select() needs to be a dynamic query. It should be fine as db_query_range().
Nicely done for a module this big!
#4
> Instead of fetchCol(). and then iterating the result. It works, I suppose, but I've not seen that idiom used anywhere else in Drupal. (Depending on your reasoning maybe we should, but we don't right now.:-) )
I was not sure under which condition this is called and was a bit scared by that "SELECT nid FROM {node}" without any conditions. I thought it would use less memory to iterate over the result instead of loading all nid's into an array but I assume that's only the case when there are many many nid's and in that case one should use batch anyway. I've also not used node_load_multiple there for the same reason.
> - node_page_view() has a db_select() in it that I don't think needs to be. db_query_range() still works the same as before, and there's no tagging, so using a static query will be faster.
Oh, i thought db_query_range needs to be replaced too.
Fixed the other things.
#5
#6
Actually forgot to do that I wrote in #302234-18: DBTNG blog.module - replaced while .. db_fetch_object.. with foreach.
#7
The last submitted patch failed testing.
#8
Re-roll due to conflict with the recent node_delete() commit.
#9
The last submitted patch failed testing.
#10
Re-roll because of the node_counter patch..
#11
The last submitted patch failed testing.
#12
It would help if I would save the updated file before creating the patch... :)
#13
Looks good. Committed to CVS HEAD. Thanks!
#14
#15
Automatically closed -- issue fixed for 2 weeks with no activity.