Posted by Dave Reid on October 9, 2008 at 7:47pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed (fixed) |
Issue Summary
Simply new database API patch for actions.inc. I'm getting a SimpleTest memory error, so for now I'll post my patch until I can get it resolved. Was hoping to test. :/
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| dbtng-actions-inc-D7.patch | 8.18 KB | Ignored: Check issue status. | None | None |
Comments
#1
<?php@@ -176,15 +172,12 @@ function actions_list($reset = FALSE) {
* 'type' and 'configurable'.
*/
function actions_get_all_actions() {
- $actions = array();
- $result = db_query("SELECT * FROM {actions}");
- while ($action = db_fetch_object($result)) {
- $actions[$action->aid] = array(
- 'callback' => $action->callback,
- 'description' => $action->description,
- 'type' => $action->type,
- 'configurable' => (bool) $action->parameters,
- );
+ $actions = db_query("SELECT aid, type, callback, parameters, description FROM {actions}") ->fetchAllAssoc('aid');
+ foreach ($actions as &$action) {
+ $action->configurable = (bool) $action->parameters;
+ unset($action->parameters);
+ unset($action->aid);
+ $action = (array)$action;
}
return $actions;
}
?>
fetchAllAssoc returns an array, why using it as an object?
<?php
/**
+ while ($action = $result) {
+ while ($action = $results) {
?>
Those are infinite loops.
* actions_save() conversion is a strange mix of a db_merge leading to a return, and two db_insert and db_update...
<?php@@ -342,7 +385,7 @@ function actions_save($function, $type,
* The appropriate action row from the database as an object.
*/
function actions_load($aid) {
- return db_fetch_object(db_query("SELECT * FROM {actions} WHERE aid = '%s'", $aid));
+ return db_query("SELECT aid, type, callback, parameters, description FROM {actions} WHERE aid = :aid", array(':aid' => $aid))->fetch();
}
?>
Why changing that SELECT *? This is not required by the DB:TNG.
#2
Ah...thanks, I missed some unfinished stuff. In my own testing, I've actually found that ->fetchAllAssoc returns the format array('key' => object data), which is why I had to cast it to an array. And not sure where you see the infinite while loops...
Revised patch attached.
#3
<?php+ while ($action = $result) {
...
?>
Well those are while, not foreach. If you don't change $result, you will get the same $action at each iteration...
#4
Ah! I didn't see that in the DBTNG api it was a foreach loop instead of a while loop, so I fixed that.
#5
Failed 13 system actions test. Working on it and will post new patch shortly.
#6
Sweet - finally figured out the fun db_merge! Both the actions and trigger tests pass 100%.
#7
Revised to use ->fetchObject() instead of ->fetch()
#8
From visual inspection:
In actions_get_all_actions(), if you want to fetchAllAssoc() as an array you can pass PDO::FETCH_ASSOC as the second parameter to fetchAllAssoc(). That will return an array of arrays rather than an array of objects. That's cleaner than recasting the object to an array.
In the same query, I don't get why you're querying action.aid only to unset it.
In actions_synchronize(), the first foreach() can be collapsed into a single fetchAllAssoc() like so:
<?php$actions_in_db = db_query("SELECT aid, callback, description FROM {actions} WHERE parameters = ''")->fetchAllAssoc('callback', PDO::FETCH_ASSOC);
?>
You'll still get
callbackin the object on each record, but looking at the rest of the function that doesn't hurt anything. It also looks like the rest of the function could handle using an object instead of an array, too, to be more standard.Isn't db_merge() awesome? :-)
So all minor code tightening stuff that should be trivial to tighten. All trigger unit tests pass for me.
Thanks, Dave!
#9
Thanks for the help Crell! I'm unsetting the aid and parameters fields in actions_get_all_actions to match the current documentation. Revised and ready to go. All actions and trigger tests pass 100%.
#10
Rockin'.
#11
Committed to CVS HEAD. Thanks Dave.
#12
Automatically closed -- issue fixed for two weeks with no activity.