Make image attach attach images at the revision level

Thanks for making the patch :)

It would need rerolling for D6 though, and the update function id changing to the normal standard.

However, I'm not sure whether it's a good idea to make this big a change at a time when we're inching towards a full 1.0 release. On the other hand, while this is a big change it's a pretty simple patch, and in some ways it's better to make a change like this while in beta than for a 1.1.

I'll ponder it and discuss with the other maintainer.

Nice one. Actually could be considered as security hardening, too.

+++ contrib/image_attach/image_attach.install	2010-03-25 11:12:14.425610672 +1100
@@ -6,27 +6,27 @@ function image_attach_install() {
-        nid int(10) unsigned NOT NULL default '0',
+        vid int(10) unsigned NOT NULL default '0',

Instead of replacing nid, we need to add vid.

Doing so will ensure backwards compatibility to some extent.

However, perhaps I overlooked something, but this patch doesn't seem to address the forward-copying/forking of existing values for new node revisions?

Powered by Dreditor.

It would be great if this could go into the 1.0 release, but I don't have the energy or focus to work on this at all.

@Agileware: any chance you could look at this patch again?

That's in image_attach_image_add_submit() which is the handler for the 'Upload' button.

I don't see why that needs to delete and then update the database, since it's taking us back to the form afterwards. Anyone else got thoughts on this?

Thanks!

+++ contrib/image_attach/image_attach.install	2010-09-19 17:35:16.000000000 +1000
@@ -160,3 +168,24 @@ function image_attach_update_6103() {
+ * Add node vid so we attach at the revision level.

Should be {image_attach}.vid, not "node vid"

+++ contrib/image_attach/image_attach.module	2010-09-19 17:45:20.000000000 +1000
@@ -298,17 +298,6 @@ function image_attach_form_alter(&$form,
-  // Rebuild the attached image data.
-  if (isset($form_state['values']['iids'])) {
-    db_query("DELETE FROM {image_attach} WHERE nid = %d", $form['nid']['#value']);
-    if (count($form_state['values']['iids'])) {
-      $weight = 0;
-      foreach ($form_state['values']['iids'] as $iid) {
-        db_query("INSERT INTO {image_attach} (nid, iid, weight) VALUES (%d, %d, %d)", $form['nid']['#value'], $iid, $weight++);
-      }
-    }
-  }

Starring at the essentials of this code, I can only guess that it is trying to prevent duplicate weight values, as that seems to be the only thing that is actually changed.

However, regardless of whether that is actually needed, the code is surely at the wrong wrong location, since the submit handler basically inserts the new rows already, whereas the API functionality in hook_nodeapi() is actually responsible for data storage, which it already does, and so it seems like we're inserting at least twice here. Though I'm not sure whether we properly take care for the weight values.

+++ contrib/image_attach/image_attach.module	2010-09-19 17:45:20.000000000 +1000
@@ -409,12 +398,15 @@ function image_attach_nodeapi(&$node, $o
+      // If we are not creating a new revision, replace the existing revision's attached images.

Should wrap at 80 chars, and more importantly, we might want to try to explain why we are entirely replacing all rows, instead of just doing what D7 would call a merge query (i.e., inserting new rows or updating existing rows accordingly)

+++ contrib/image_attach/image_attach_views_handler_field_attached_images.inc	2010-09-19 17:26:37.000000000 +1000
@@ -63,26 +63,26 @@ class image_attach_views_handler_field_a
+        $vid = intval($v->{$this->aliases['image_attach_vid']});

Should use (int) casting.

Powered by Dreditor.

> instead of just doing what D7 would call a merge query (i.e., inserting new rows or updating existing rows accordingly)

Is there a nice way to do that on D6? Every module I've seen has this pattern of 'delete all rows for the node, put them all back in again'.

Unfortunately not - and on MySQL, db_merge() effectively is just a short-cut for
(pseudo-code)

db_query('UPDATE foo SET bar = baz WHERE beer = 1');
if (!db_affected_rows()) {
  db_query('INSERT INTO foo VALUES (beer = 1, bar = baz)');
}