Add proper API functions and hooks to allow other modules to interact with Feedback messages

We can do this. However:

a) I do not want to see a contrib module called feedback_forward module, ever. The idea is wrong (reasoning in the other issue).

b) The hooks must use the new style of system object hooks in Drupal 7, which means: hook_feedback_load, hook_feedback_insert, hook_feedback_update, hook_feedback_delete

Duplicate of #293512: Abstract back-end for integration with 3rd party issue trackers

We should tackle this issue first and introduce D7-style API hooks for _load, _insert, _update, and _delete.

#353548: Add trigger/action for e-mail notification will have to be revised on that.

Work in progress, have just the feedback_load invoke and started a corresponding feedback.api.php to document the new hooks.

Thanks! Looks like a clean and good start!

Note that this overall issue likely requires us to change and/or introduce proper API functions for feedback messages. If I'm not mistaken, then the CRUD API operations are incomplete currently.

Either way, the goal is to mimic the usual design of

node_load() ( -> node_load_multiple())
node_load_multiple()
node_save()
node_delete()
node_delete_multiple()

Updated the _load function to use _load_multiple and to use similar parameters as the node/user functions. I think that's what you're going for, without going all the way into entities yet.

Yeah, I'm planning to do the rest of the functions including delete even though there is not yet a UI for deletion.

+++ feedback.module	3 Jan 2011 22:19:51 -0000
@@ -126,10 +126,10 @@ function feedback_form($form, &$form_sta
-      $feedbacks = feedback_load(array('f.status' => 0, 'f.location_masked' => feedback_mask_path($_GET['q'])));
+      $feedbacks = feedback_load(array(), array('f.status' => 0, 'f.location_masked' => feedback_mask_path($_GET['q'])));
...
-      $feedbacks = feedback_load(array('f.status' => 0, 'f.location' => $_GET['q']));
+      $feedbacks = feedback_load(array(), array('f.status' => 0, 'f.location' => $_GET['q']));

Those should use feedback_load_multiple().

+++ feedback.module	3 Jan 2011 22:19:51 -0000
@@ -199,24 +199,54 @@ function feedback_format_message($entry)
  * Load feedback entries from the database.
  *
+ * This function should be used whenever you need to load more than one entry
+ * from the database.

We can drop the additional phpDoc description, but let's update the summary to be in the third-person form; i.e., "Loads" instead of "Load".

+++ feedback.module	3 Jan 2011 22:19:51 -0000
@@ -199,24 +199,54 @@ function feedback_format_message($entry)
+ *   An associative array of conditions on the {feedback}
+ *   table, where the keys are the database fields and the values are the
+ *   values those fields must have.

Wraps too early (before 80 chars).

+++ feedback.module	3 Jan 2011 22:19:51 -0000
@@ -199,24 +199,54 @@ function feedback_format_message($entry)
+  ¶

Trailing white-space.

+++ feedback.module	3 Jan 2011 22:19:51 -0000
@@ -199,24 +199,54 @@ function feedback_format_message($entry)
+  return feedback_load_multiple(array($fid));

feedback_load_multiple() returns an array, so this needs to be updated to correspond to similar loader functions in core; essentially:

  $entries = feedback_load(array($id));
  return (isset($entries[$id]) ? $entries[$id] : FALSE);

Powered by Dreditor.

I made those changes and added feedback_save and hook_feedback_insert/hook_feedback_update.

Still need to do the delete functions.

Just fixed one comment length.

This patch has the load, load_multiple, save, delete and delete_multiple functions with hooks insert, update, load and delete. Also has all the db queries using these functions.

This looks really awesome!

+++ feedback.admin.inc	5 Jan 2011 20:37:54 -0000
@@ -113,21 +114,15 @@ function theme_feedback_admin_view_form(
 function feedback_admin_view_form_submit($form, &$form_state) {
...
+        feedback_save($entry);

+++ feedback.module	5 Jan 2011 20:37:54 -0000
@@ -197,26 +200,116 @@ function feedback_format_message($entry)
+function feedback_save($entry) {
+  global $user;
+
+  $entry->uid = (empty($entry->uid)) ? $user->uid : $entry->uid;
+  $entry->message = trim($entry->message);
+  $entry->location_masked = feedback_mask_path($entry->location);
+  $entry->url = url($entry->location, array('absolute' => TRUE));
+  $entry->timestamp = REQUEST_TIME;
+  $entry->useragent = $_SERVER['HTTP_USER_AGENT'];

When updating feedback entries through the admin UI, then various properties are unintentionally reset/updated.

For now, we may want to wrap all of the property updates/overrides into separate !isset() conditions.

In the long run, we're going to need a feedback_new() resp. feedback_create() [Entity API module call it _create; elsewhere I called it _new] function that takes an array of key/value pairs and initiates a new feedback entry object with those property/value pairs + defaults for all others. We can already do this now, if you want.

+++ feedback.module	5 Jan 2011 20:37:54 -0000
@@ -197,26 +200,116 @@ function feedback_format_message($entry)
+ *   are the database fields and the values are the values those fields must have.
...
+ *   The feedback entry object to modify or add. If $entry->fid is omitted, a new
...
+ *   The updated feedback entry object upon successful save, or FALSE if the save

Those minimally exceed 80 chars. (see Dreditor)

+++ feedback.module	5 Jan 2011 20:37:54 -0000
@@ -197,26 +200,116 @@ function feedback_format_message($entry)
+  return $entry;

Save functions normally do not return the entity -- objects are references in PHP5.

+++ feedback.module	5 Jan 2011 20:37:54 -0000
@@ -197,26 +200,116 @@ function feedback_format_message($entry)
+    $entries = feedback_load_multiple($fids);
+    foreach ($entries as $fid => $entry) {
+      module_invoke_all('feedback_delete', $entry);
+    }
+    db_delete('feedback')

Normally, the module hook is invoked after the fact; i.e. after deletion. Let's make this consistent.

+++ feedback.module	5 Jan 2011 20:37:54 -0000
@@ -232,45 +325,23 @@ function feedback_mask_path($path) {
 function feedback_user_cancel($edit, $account, $method) {
   switch ($method) {
     case 'user_cancel_reassign':
-      db_update('feedback')
-        ->fields(array('uid' => 0))
-        ->condition('uid', $account->uid)
-        ->execute();
+      $entries = feedback_load_multiple(array(), array('uid' => $account->uid));
+      if (!empty($entries)) {
+        foreach ($entries as $entry) {
+          $entry->uid = 0;
+          feedback_save($entry);
+        }
+      }
       break;
 
     case 'user_cancel_delete':
-      db_delete('feedback')
-        ->condition('uid', $account->uid)
-        ->execute();
+      $entries = feedback_load_multiple(array(), array('uid' => $account->uid));
+      feedback_delete_multiple(array_keys($entries));
       break;

These changes look great, but I don't think all of them are necessary.

For the reassign method, we can leave the direct database update, I think.

The delete method is outdated and no longer invoked for hook_user_cancel(). Instead, modules need to implement hook_user_delete().

Powered by Dreditor.

Thanks for the great reviews.

Changed everything above except:

The comment line lengths. They look right to me in dreditor, but I think I'm missing something since you're the dreditor expert. They're supposed to be just left of the vertical line, right?

Placement of the delete hook. I put it above the actual deletion to be consistent with user_delete_multiple and node_delete_multiple. node_delete_multiple has a comment explaining that rationale: // Delete after calling hooks so that they can query node tables as needed.

Thank you! This looks great!

The only two remarks that remain from my side are:

+++ feedback.module	8 Jan 2011 00:57:48 -0000
@@ -197,26 +200,110 @@ function feedback_format_message($entry)
+ * @param $entry
+ *   The feedback entry object to modify or add. If $entry->fid is omitted, a new

All comments should wrap at 80 chars; so for example, this line wraps exactly at 81 chars (+1). In Dreditor, the line is supposed to end right before the vertical bar (whereas this line ends "on" the vertical bar).

After adjusting this one we can move forward. :) [You probably know that I take Drupal coding standards überseriously. ;)]

+++ feedback.module	8 Jan 2011 00:57:48 -0000
@@ -197,26 +200,110 @@ function feedback_format_message($entry)
+    $entry->uid = $user->uid;
+    $entry->message = trim($entry->message);
+    $entry->location_masked = feedback_mask_path($entry->location);
+    $entry->url = url($entry->location, array('absolute' => TRUE));
+    $entry->timestamp = REQUEST_TIME;
+    $entry->useragent = $_SERVER['HTTP_USER_AGENT'];

This still feels a bit sloppy for me, since modules cannot preset any of these properties without getting overwritten. But unless you want to shift every single of those assignments into a corresponding !isset($entry->property) conditional now, we can also leave that for later (and potentially go with the _new()/_create() function instead then).

Powered by Dreditor.

Ok, finally got the comment length figured out in my text editor, and I wrapped all those pre-insert values in issets.

+++ feedback.module	12 Jan 2011 20:15:37 -0000
@@ -197,26 +200,111 @@ function feedback_format_message($entry)
+ *   are the database fields and the values are the values those fields ¶

;)

Slightly changed the order of functions (to have _load() come before _load_multiple()). Also changed the conditional defaults in feedback_save() into cleaner !isset() control structures.

Also added very basic initial tests in a separate commit, in order to verify that the basic Feedback functionality still works.

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Some minor follow-up clean-ups. Should technically be a separate issue, but I'm too lazy right now.

Also committed the follow-up in #17.

@Jody Lynn: Please make sure to have a look at those changes, thanks :)