Attach Multiple Images with image_attach using Drupal upload mechanism

I've modified the image_attach module to allow adding multiple images using the Drupal upload mechanism. This allows content creators to upload multiple images within the current page. Images that are not commited to nodes are deleted to ensure no junk or wasted space. This is my first contribution so I would love to get some feedback on this. Thanks =)

Would be better if I double checked my work before I upload. Sorry guys this is the proper file =)

Did you have to change that much that you have to provide the whole module? Would it be possible to provide a patch, that is easier to review? (http://drupal.org/diffandpatch) Now I have to diff manually to see if the .install has changed and stuff like that.

(btw, if you change the title on your comment you change the title of the issue, I've removed the "RE:")

I replaced the original module by the one provided in your zip and when I try to attach an extra image to the node I get a javascript alter. It is attached to this comment.

For those interested in this, I presented another solution here: http://drupal.org/node/87991

@evil_marty: I think I got the error because I didn't have the upload module enabled. I'll try again soon!

JAX, your javascript error is in fact a 'not found' page.
You get this because your session didn't cache the newly added cachable path 'upload/image_js'. (see the menu hook)
With a new session it will certainly go better!

Cheers, Danny

I have resolved a couple of problems with the 'image_attach' module from evil_marty.

• uploaded images are now linked to the revision instead of the node. This implies another database modification.
• made completely compatible with revisions.
• image nodes are deleted when one deletes a revision, but only if they are not used by other revisions
• when an image gets removed from the current revision, it won't delete the image node if there are revisions that reference it.
• as a by-effect of this, already uploaded images don't dissapear anymore when one adds new image.
• the install now contains a function to upgrade the database.
• By default existing images will be associated with all revisions of a node.
• An alternative is available to associate images only with the current revision
• ATTENTION: i haven't been able to test this, so some feedback would be nice!
• I supplied patchfiles, as requested by Yax, which apply to the CVS version. Evil-Marty's modifications are included.

The module continues to give nasty SQL errors. It seems like some other module goes haywire for some reason.
When one enters the edit page after having uploaded images, it gives this error for every uploaded image:

warning: mysql_real_escape_string() expects parameter 1 to be string, array given in /u/mount/photo/appdata/Logomatica/Sites/immovac/site/includes/database.mysql.inc on line 350.

user warning: Unknown column 'n.name' in 'where clause' query: SELECT n.nid, n.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.moderate, n.sticky, r.timestamp AS revision_timestamp, r.title, r.body, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM node n INNER JOIN users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE n.uid = '1' AND n.name = 'admin' AND n.created = '1161868065' AND n.type = 'image' AND n.images = '' AND n.new_file = '' AND n.title = '' AND n.teaser = '' AND n.status = '' AND n.moderate = '' AND n.promote = '' AND n.sticky = '' AND n.revision = '' AND n.action_on = '' AND n.validated = '' AND n.is_new = '' AND n.nid = '' AND n.vid = '' AND n.changed = '' AND n.priority_override = '' in /u/mount/photo/appdata/Logomatica/Sites/immovac/site/includes/database.mysql.inc on line 121.

This query is clearly generated, which makes me think of search module involvement....
That's not part of this module, though.
Does anyone know where this comes from, and how to solve it?

Cheers, Danny

Here is a new patch for the image_attach module.

• The problem with the SQL error is resolved now.
• Uploading a new image with the 'submit' button instead of the 'attach' button now works. This ---probably--- means that uploading without javascript works too! (but it has to be tried, of course)
• The checkboxes above the image no longer mean 'keep' but have become 'delete' checks, just like upload does.
  
  I just hope Evil-Marty is not gonna shoot me for this...
• General rework of the code. I moved code from the nodeapi function into their own

TO DO:
The module probably doesn't respect access rights yet. It at least needs to check if the user can create images.
The updater in the .install file has not been tried at all.

Let me know what you think of it.

Cheers, Danny

is it possible to preserve the "existing image" funtion ?

sorry i have modify the title of the thread ?

sorry i have modify the title of the thread ?

You could also change it back of course... anyway, I've done it now. ;)

I'm not sure about what you are trying to say.
Do you mean that existing images are discarded when you upload new ones?
That problem should be resolved if you applied the patch.

Danny

In fact i want to preserve the new function : add existing image
and the function : add multiple image

but the patch erase it

are they incompatible ??

Same thing here as above

Adding from existing image(that was upload via other nodes) is gone. That means have to upload new ones everytime even if I already have a existing image already in the image folder
(that was uploaded via other nodes).

It seems like i've overlooked this feature completely.
I've started out with the code from evil-marty, which also doesn't have this feature in it.

I'm working on porting this feature from the CVS version to mine. It's pretty well finished now, but i want to do some extra checks.
I'll upload it on monday.

Stay tuned!
Danny

This new version includes chances to attach existing images, as implemented in the existing CVS version.

To add some extra's, here are the translation files for ITALIAN and DUTCH

Cheers, Danny

I like this approach and the better upload UI. We also get the "detach" feature as a bonus. Unfortunately, the patch doesn't apply anymore.

@David Lesieur
Too much has changed really for a patch, so i'll upload it completely module one of these days.

Currently i've added 'views' integration (first image only) and a feature to select a 'no images' image, which will be displayed with nodes that don't have any images attached.

I'm not finished with this yet, and the italian and dutch translations are outdated, so please hold on till' next week!

Cheers, Danny

Thanks for the update, Danny! BTW, if you implement many features, I think you will have to split your work into separate, smaller patches, if possible, to increase the odds of them ever getting committed. ;-) Also, you might want to have a look at the Views integration that has been committed recently.

what is the status of this? i'd really like to be able to attach more than one image to my cck nodes rather than upload one and do the rest through the file attachment section

It's been almost a month now since i promised to upload a new version of this module.
Sorry about that, but i do have a lot of work on my hands, lately ;)
But at last, here it is!

I really wanted to add a complete zipped module, but this doesn't seem to be possible anymore.
So, i've created a megapatch that will apply agains the 4.7 version.

Status:
- It works very well and seems to be stable. I have it installed on live site, and i haven't had any problems with it, yet.
- Translations are available in Italian and Dutch.
- Views integration works as well. Only the first image can be shown, and you can choose one of the sizes from the image module.

@David Lesieur:
Thanks, i didn't know they added views support in the original. Too bad because i could have copied it, because not suprisingly, it is very similar.
Personally i think that a patch should never leave a module in an unusable state, so splitting it would be really difficult as one feature implies another or wouldn't make sense without the other.

Anyway, when i have a little more time at hand i'll look into this.

Cheers, Danny

dvdweide, the translation should not be included in this patch. please submit it separately. and you should be using cvs diff -up to generate the patches.

I am Wondering if anyone has submitted this functionality, or a patch for the drupal 5 version?

OK, what's the status of this? The last patch seems to mention version 1.2.2.3 ... and I've currently checked out 1.9.2.4 and I can't seem to get it to apply. (yeah, and the po files don't help)

I came looking for the 'detach' button which seems to be missing, but multiples would be nice too, I guess.

I'd test, but currently cannot, as I suspect this patch is incompatable with DRUPAL-5.

Hi there!

I am also interested in this patch very much. What's the current status? Is there a version for Drupal 5.x?

For all those that requested status information on this patch, here are the short facts:

• The patch applies to the 4.7 version only (rev 1.2.2.3 which is the latest in the DRUPAL-4-7 branch)
• Changes in the patch are quite extensive. Simply merging in the changes from 4.7 -> 5.0 didn't work out.
• So, there is no 5.0 version available
• I wish i had time to integrate the multi-image features into the current 5.0 version, but unfortunately i don't :(

Until i finished my current project i can't switch to 5.0 myself, as this would include the porting of several custom-made modules.
I'm really sorry about this, but my hands are tied (yeah, i have a boss...)

Cheers,
Danny van der Weide

Hi everbybody:

I need the path for 5.0, please any notice send a comment.

Thanks

can't image_attach be dropped in favor of http://drupal.org/project/upload_image ? at least with upload_image you have multiple image attach and deletion features ?
so i would suggest to mark it as won't fix

Is there a 5.0 version already?

Is this something that should be bountied? For it to be a true reverse bounty, we need a developer who can scope the amount of work required and what they would want to get for this.

Hi

I have found 3 options for Drupal 5 which all do something similar.
I have not tried any of them yet.

http://drupal.org/project/node_images
http://drupal.org/project/pictures
http://drupal.org/project/slideshow

hope they work!
cheers
Sean

hi guys, thanks for all your interests in the update. I'm sorry I haven't responded in a long time, unfortunately priorities at work were shifted etc. I am now working on migrating my company's sites to 5.0 and was going to update the module to work with it but as gaurdian (post #30) mentioned, I've replaced my use of image_attach with upload_image. This serves as a better implementation and intergrated well with other uploads. In saying that I believe it to be a better solution and unless there is a need for it I wont update the module. Sorry to all those that want this module, if you really do need an update let me know and if I have time can see what I can do. Thanks again guys.

I'm using a multiple image_attach.module by evil_marty (with no patches 'cause I don't know how to patch a files) and I have a lot of errors in my site - in php "invalid argument supplied foreach()" and awful "duplicate entry" errors in my database which are off course very bad thing.

Do anybody have a solutions for my problems?

Maybe someone have a working module, already patched?

marked http://drupal.org/node/220986 as a duplicate. i'd like to get this into 6.x-1.x and then backport to 5.x-2.x,

Hey folks,

for a recent project of mine I wrote a version of the image_attach module using methods of the image module and the upload module. It needs xajax to work (for the removing of queued uploads). I just thought I could post it here, maybe it will be useful to somebody.

I hope you can excuse, that it's not a patch, but I didn't intend to provide it, so at the time of the project I simply started hacking and made a new module. I use it, it works fine as to my installation. If there are problems using it I can not guarantee to be able to help though. Right now I have different priorities.

But let me know if it works.
best regards,
Hadubard

Marked http://drupal.org/node/228560 as duplicate.

Subscribing.

subscribing

Subscribing.

i'd like to get this into 6.x-1.x and then backport to 5.x-2.x,

Updating back to maintainers wishes

Also subscribing... this is popular!

Subscribing.

Hi,

How would one go about installing xajax? I can't find the module :/

Best regards,
Michael :)

subscribing

I found out how to do it...

Download the module here: http://daryl.learnhouston.com/2005/11/21/xajax-in-drupal/

also see http://drupal.org/node/253503.

+1 this feature, it solves a huge hole in drupla functionality, look forward to the development

subscribing

subscribing, I'm waiting for this feature to fix all current problems with image_attach in drupal 6.x ;)

add

D5ers: have a look at this patch:
http://drupal.org/node/267806

Closed #267806: Attach Multiple Images with image_attach and #253503: Attaching Multiple Images using Image_Attach as duplicates.

subscribe

Lots of subscribing, but no action. I've had a look around but no one seems to have created a stable D6 patch for this yet. And no one seems to be working on it. Has anyone tried the D5 patch maulwuff created? It went from "patch code needs work" to "duplicate" without much in the way of feedback, but within that thread there was a semi-detailed description of how the patch was achieved and which functions were altered. If community feedback is this D5 patch works well, it could be a good starting point.

Here is maulwuff's patch from http://drupal.org/node/267806#comment-877363 rerolled against latest image 6. Image select box allows multiple. Teaser theme just shows first image while full view shows all images. I didn't touch any CSS so the layout probably isn't ideal. This is a quick patch, but hopefully gets development and discussion happening under image 6.

Error when image_attach option attach existing images is disabled:
warning: Invalid argument supplied for foreach() in /home/dev/contributions/modules/image/contrib/image_attach/image_attach.module on line 217.

It looks like we will be tackling this problem at some point in the next two weeks. We will almost certainly need this feature working in Drupal 6.x. Watch this space.

This patch is really good. I am scheduled in to spend a few hours on it, during which time I will endeavour to:

1. Allow you to upload multiple images without saving the node each time a re-editing (e.g. mimic the standard File behaviour)
2. Separate the image mark-up in to different parts (it's not flexible to have them all lumped together - what if I want one pic at the top and one at the bottom of a page?) ... making the $node->content array look like this:

    [content] => Array
        (
            [image_attach] => Array
                (
                    [0] => Array
                        (
                            [#value] => <div style="width: 250px" class="image-attach-body"><a href="/content/blue-hills"><img src="http://boston/sites/default/files/images/Blue hills.article.jpg" alt="Blue hills" title="Blue hills"  class="image image-article " width="250" height="188" /></a></div>
                            [#weight] => 0
                        )
                    [1] => Array
                        (
                            [#value] => <div style="width: 250px" class="image-attach-body"><a href="/content/lilies"><img src="http://boston/sites/default/files/images/Water lilies.article.jpg" alt="Lilies" title="Lilies"  class="image image-article " width="250" height="188" /></a></div>
                            [#weight] => 0
                        )

                )

        )

Hope I don't run out of time! =(

greg: that would be pretty awesome.

I was working on implementing the new D6 theming layer but I don't remember if I got as far as Image_attach... t any rate it should be fairly simple to get the theming working with this in at least a basic manner that's good to go with a first release!

Sadly, I'm running Microsoft TFS here for version control (don't ask!) and I can't work out how you make a patch file with it. (At home I use Subversion and Eclipse so patching is easy, but here the Eclipse TFS plug-in seems to kill Eclipse's patch-making abilities - I guess it relies on Subversion for the diff.)

DONE:

- separate the image mark-up in to different parts

TO DO:

- investigate the warning coltrane reported
- allow multiple uploads a la the Upload module

If someone could combine a patch of my attached module file against 6.x-1.0-alpha2 with the patch of the .install file in coltrane's original patch, that would be awesome. Sorry I can't do it myself. Pesky Source Safe! =(

Actually, I used WinMerge to create a patch. It's not the "usual" format, but it is a patch. Hope it works.

IMPORTANT - It omits the changes to the .install file for now. Couldn't figure out how to make WinMerge do a unified diff, so you must apply the schema changes separately using the patch in coltrane's post. What a pain! Sorry guys. =(

Ok - I've got the ability to upload more images prior to submitting the node form working, which is great! B)

Problem is I can't update the returned node form to reflect the newly attached image. Reason is it is very difficult to get a handle on the nid of the newly created image and add it in to the thumbnail loader and default values for the image selection box. I am really struggling to get hold of the new nid. Any suggestions?

Not true. I can get hold of the new image nid. What I can't seem to achieve is getting it back in to the $form array so Drupal uses it when it rebuilds the node form! =(

New submit function for the image attach button looks like this so far - explanatory comments inline:

I think the submit function is fine - if I can get the VALIDATE function working correctly (and making the correct changes to $form) then this will work.

Well I've worked out how this used to work. In the image_attach_validate function there was this line:

form_set_value($form['image_attach']['iid'], $image->nid, $form_state);

Which I changed for this:

$form['image_attach']['iid']['#default_value'][] = $image->nid;

We're no longer *changing* a value, we're *adding* one.

My new button looks like this in image_attach_form_alter:

      //additional submit button which adds image and brings you back to node edit
      $form['image_attach']['image_attach_multiple'] = array(
        '#type' => 'submit',
        '#value' => t('Attach'),
        '#submit' => array('image_attach_image_add_submit'),
        '#validate' => array('image_attach_validate'),
      );

Trouble is, when I examine $form['image_attach']['iid']['#default_value'] in the $form array in image_attach_image_add_submit it does not contain the information the validation function tried to add. Somehow any changes made to $form during the validation phase are lost. I'm sure this is just me missing something in the form API, but I really need someone to jump in here and tell me what, or we're totally stalled! =(

For reference, validate and submit functions look like this:

/**
 * This is practically unchanged from the original
 */
function image_attach_validate(&$form, &$form_state) {
  $validators = array(
    'file_validate_is_image' => array(),
  );
  if ($file = file_save_upload('image', $validators)) {
    $image_title = $_POST['image_title'] ? $_POST['image_title'] : basename($file->filepath);
    // This func now does all the things to initialize an image properly
    $image = image_create_node_from($file->filepath, $image_title, '');
    if ($image && !form_get_errors()) {
      drupal_set_message(t("Created new image to attach to this node. !image_link", array('!image_link' => l($image_title, 'node/'. $image->nid) )));
      // ALTERED LINE HERE SHOULD UPDATE $form_state, BUT THESE CHANGES ARE
      // LOST TO THE SUBMIT FUNCTION! =(
      $form_state['values']['iid'][$image->nid] = $image->nid;
    }
  }
}


/**
 * New submit function for our new Attach button
 */
function image_attach_image_add_submit($form, $form_state) {

  //rebuild the attached image data - if validate works properly, this will work too
  if (isset($form['image_attach']['iid'])) {
  	db_query("DELETE FROM {image_attach} WHERE nid=%d", $form['nid']['#value']);
    if (count($form['image_attach']['iid']['#default_value'])) {
      $form['image_attach']['iid']['#default_value'] = is_array($form['image_attach']['iid']['#default_value']) ? $form['image_attach']['iid']['#default_value'] : array($form['image_attach']['iid']['#default_value']);
      foreach ($form['image_attach']['iid']['#default_value'] as $iid) {
        db_query("INSERT INTO {image_attach} (nid, iid) VALUES (%d, %d)", $form['nid']['#value'], $iid);
      }      
    }
  }
  
  //rebuild the node edit form
  node_form_submit_build_node($form, &$form_state);
}

For the record, I've re-read the manual and tried both of these variations in my validate function, in an attempt to change the $form array:

$form_state['values']['iid'][$image->nid] = $image->nid;
$form['image_attach']['iid']['#default_value'][] = $image->nid;

Both get "lost" by Drupal form API. I'm at my wits end. This should work, so far as I can tell, but I must be doing something silly I just can't see.

Code so far is attached. Again, apologies for lack of proper patch. My IDE here is screwed up. =(

You've got the devel module installed right?
Stick a dsm($form) into the validation function to see what's going on if you haven't already.

As for your IDE, try TortoiseCVS if you can.

Sorry I can't offer much more help than that and encouragement :)

Thanks!

Unfortunately I've already checked $form - when the validate function has run its course $form is correct, but when the submit function is fired and you expose the $form array again it has dropped anything that was added during validation. I need to hear from someone who can tell me why this might be happening.

Got TortoiseSVN installed, but it can't make a diff without the code being stored in a Subversion repository, which it isn't. It's in MS TFS (the replacement for SourceSafe). =(

This seems ok now. I tried the $form_state['values'] approach again and it worked! =)

Being a Drupal 5-x'er until recently, I didn't really quite know what to do with $form_state. Docs didn't help much, but I *think* (and correct me if I'm wrong) if you want to change *data* in the form and have it retained by the other form functions then you need to make those changes in $form_state['values'].

New image_attach_validate and look like this:

/**
 * Capture node form submission and immediately create an image if one has been
 * uploaded.
 * Note that the new image nodes are created even on preview. Taking several
 * attempts may create trash.
 */
function image_attach_validate(&$form, &$form_state) {
  $validators = array(
    'file_validate_is_image' => array(),
  );
  if ($file = file_save_upload('image', $validators)) {
    $image_title = $_POST['image_title'] ? $_POST['image_title'] : basename($file->filepath);
    // This func now does all the things to initialize an image properly
    $image = image_create_node_from($file->filepath, $image_title, '');
    if ($image && !form_get_errors()) {
      drupal_set_message(t("Created new image to attach to this node. !image_link", array('!image_link' => l($image_title, 'node/'. $image->nid) )));
      // This line changed to append to array of images instead of reset the single value
      $form_state['values']['iid'][$image->nid] = $image->nid;
    }
  }
}

/**
 * This submit function adds a new image and returns you to the
 * node edit form directly afterwards, without creating the new node yet
 */
function image_attach_image_add_submit(&$form, &$form_state) {
  // Rebuild the attached image data
  if (isset($form_state['values']['iid'])) {
  	db_query("DELETE FROM {image_attach} WHERE nid=%d", $form['nid']['#value']);
    if (count($form_state['values']['iid'])) {
      $form_state['values']['iid'] = is_array($form_state['values']['iid']) ? $form_state['values']['iid'] : array($form_state['values']['iid']);
      foreach ($form_state['values']['iid'] as $iid) {
        db_query("INSERT INTO {image_attach} (nid, iid) VALUES (%d, %d)", $form['nid']['#value'], $iid);
      }
    }
  }
  
  // Rebuild the node edit form
  node_form_submit_build_node($form, &$form_state);
}

--
http://www.drupaler.co.uk/

Code so far attached again.

DONE:

- separate the image mark-up in to different parts
- allow multiple uploads a la the Upload module

TO DO:

- investigate the warning coltrane reported
- allow individual weights to be set on images (see node_images - http://drupal.org/project/node_images - for a user case)
- seems to now be a bug with taxonomy selection on refresh after attaching an image, to be investigated

TortoiseCVS. Can't you just use that to grab the CVS from Drupal.org, then overwrite the files with yours, then do a diff?

Weights for images will probably be needed at some point, but I don't think they should be a bar to getting this into a release.

No more being done on this for at least a week. Anyone else going to run with it??

I guess not. Has anyone else even tried it?????

It was fine, I did a clean install and now I get this when I attach an image to a node:

user warning: Unknown column 'n.0' in 'where clause' query: SELECT n.nid, n.type, n.language, n.uid, n.status, n.created, n.changed, n.comment, n.promote, n.moderate, n.sticky, n.tnid, n.translate, r.vid, r.uid AS revision_uid, r.title, r.body, r.teaser, r.log, r.timestamp AS revision_timestamp, r.format, u.name, u.picture, u.data FROM node n INNER JOIN boston_shared.users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE n.0 = '1' in C:\projects\Defaqto Websites\boston\www\modules\node\node.module on line 749.

=(

I'm not sure it's my changes though. Other references to this finger the image module as the cause and, like I say, it was fine.

Problem lies in image_attach_block() ... starting line 77 of the patched module:

          if (isset($node->iid) && $node->iid) {
            $image = node_load($node->iid);
            if (node_access('view', $image)) {
              $img = image_display($image, variable_get('image_attach_block_0_size', IMAGE_THUMBNAIL));
              return array(
                'subject' => t('Attached Images'),
                'content' => l($img, "node/$node->iid", array('html' => TRUE)),
              );
            }
          }

Again, it expects a single iid, not an array of iid's. Another TO DO!

Hello

I applied the patch in image_attach.module, but i got the following warning:

"warning: Invalid argument supplied for foreach() in C:\Programas\xampp\htdocs\lugares\modules\image\contrib\image_attach\image_attach.module on line 217."

But despite the warning, i can only upload one image. OOps i noticed now that is supposed to be that way...

This is an important functionality, hope to live to see it in image core.:)

I really need to start programming...

Fixed #74 - new code is:

          if (isset($node->iid) && $node->iid) {
            $output['subject'] = t('Attached Images');
            foreach ($node->iid as $this_image) {
              $image = node_load($this_image);
              if (node_access('view', $image)) {
                $img = image_display($image, variable_get('image_attach_block_0_size', IMAGE_THUMBNAIL));
                $content .= l($img, "node/$this_image", array('html' => TRUE)).'<hr />';
              }
            }
            $output['content'] = $content;
            return $output;
          }

Patch to follow.

Who's interested in this issue and at conference? How about we put it to bed once and for all at code sprint?

Sadly I can't make the conference any more - otherwise I would've gladly been involved. =(

I'll try and post a proper patch of the work so far for D6 before the conference start date.

... which was yesterday :)

If anyone of the dozens subscribed to this issue is at Szeged right now, please come and find me and we'll try to get a code sprint arranged.

Oops! Sorry. WinMerge-generated patch attached, patched against alpha2 (*not* alpha3). It's actually nearly there. I've also attached the full .module file (image_attach_new.txt) so someone can create a patch against alpha3.

TO DO:

- investigate the warning coltrane reported
- allow individual weights to be set on images (see node_images - http://drupal.org/project/node_images - for a use case)
- seems to now be a bug with taxonomy selection on refresh after attaching an image, to be investigated

And if you get that done, there is another task I've pitched in as a feature request here:
http://drupal.org/node/301006

=)

Here's a reroll of the previous comment's code as a patch on CVS HEAD.

The uploading of a new image as an attachment to a new node doesn't seem to be working, and I think it's the wrong approach anyway.
It should work the same way attaching a file to a new node works: ajax. When I add a new file to a new node, clicking the 'Attach' button gets some ajax mojo running rather than reloads the entire page.

So we either use stuff from upload.module or shamelessly copy the code.
I'm not familiar with upload.module at all so I could really use some advice on which way to go.

Some of the theming stuff will change once http://drupal.org/node/271287 gets in.

It should work the same way attaching a file to a new node works: ajax. When I add a new file to a new node, clicking the 'Attach' button gets some ajax mojo running rather than reloads the entire page.

I completely agree - I just didn't have time to do it, so what you have is a quick fix. =(

Along with #103793: Add token support for image file directory and image names (and fixing some bugs in #292039: Image node can be created without any image) this issue is something of a must-have for what I'm doing with images in D6.

I've applied the #81 patch and will test as best I can in the next few days. I am also ignorant of the AJAX mojo unfortunately :)

FYI anyone, I've been getting the SQL error in comment 73 above. Turns out many weeks ago I had created my own override theme_image_attach_body in template.php and forgotten it was there. It still expected to receive one parameter with $node.

Once I changed it to work like the patched theme_image_attach_body() it was OK.

I fixed it. It's in the patch. ;-)

Yes I could see how comment 76 resolved the problem, but it took me some time to realise that I needed to make a change in my template.php to get the benefit :)

Oh, I see. I didn't understand. Glad it's fixed! I wonder if joachim managed to get any further at the Drupalcon? =)

I've been having a little problem with dropdown taxonomy fields. If I selected a taxonomy term and then uploaded an image to attach, when the form was redisplayed the dropdown field was reverting to "Please choose".

I think the cause is that when image_attach_image_add_submit() calls node_form_submit_build_node() the taxonomy data is presented to taxonomy_form_alter() as an array, however $form_state['node_preview'] is not set so the function taxonomy_preview_terms() is not called within taxonomy_form_alter().

Setting $form_state['node_preview'] does not appear to work adequately as the value gets rendered as a raw text character on the redisplayed form. Hence I have tried performing the taxonomy format conversion within image_attach_image_add_submit() directly before the call to node_form_submit_build_node():

  if (module_exists('taxonomy') && !empty($form_state['values']['taxonomy'])) {
    $tnode = new stdClass();
    $tnode->taxonomy = $form_state['values']['taxonomy'];
    $form_state['values']['taxonomy'] = taxonomy_preview_terms($tnode);
    unset($tnode);
  }

When I upload an image to attach, my taxonomy selection is not lost. At least not so far :)

I think the creation of a link back to the image node in theme_image_attach_body() needs a tweak.

The patched code that reads

      $output .= l(image_display($image, $img_size), "node/$iid", array('html' => TRUE));

creates an incorrect href - at least for me. I found

      $output .= l(image_display($image, $img_size), "node/$iid['nid']", array('html' => TRUE));

worked.

Another little problem with image weighting. Having imported 120+ Story nodes and successfully created automatic Image Attachment associations for those nodes that needed them (images having been imported beforehand) I was finding that the weighting did not appear to work - all my images were rendered at the end of the respective Story node.

I couldn't guarantee that this is the complete solution but I found things improved if I created the weighting array item in image_attach_nodeapi in a more similar way to how it was before:

    case 'view':
      if (count($node->iid)) {
        $teaser_or_body = $teaser ? 'teaser' : 'body';
        $node->content['image_attach'] = array();
        $node->content['image_attach']['#weight'] = variable_get("image_attach_weight_{$teaser_or_body}_{$node->type}", 0);
        if ($teaser_or_body == 'teaser') {

Other than that and my previous comment I am finding this enhancement to be the answer to my prayers, so thanks!

This has stalled big time, and it's a really useful patch. Any chance of resurrecting it? =(

ditto to greg - it would seem to me that people may want to attach more than one image!

The functionality actually works as of this post if you need it. The continuation is all about improving the UI, which is a pig at the moment (I hacked it together and I freely admit it's rubbish)!

It's so nearly there. joachim tried to organise a sprint to finish it at the Drupalcon, but I guess he didn't manage. =(

Nope, nothing happened at DrupalCon.
The situation basically is that the core coders I spoke to at conference feel that image module is a dead end: they are concentrating on filefield, introducing a hook_file into core, and then rewriting imagefield as a plug-in for filefield.

If this and other patches on image module are going to happen, it's going to have to come from the community of this module's users. I was going to try and get something done this week but I have a stinking cold :(

Hmm, that might explain why the Image issue queue seems to be being ignored. I wish they'd say so on the main page and give people some warning! As it is, people are working on and adopting Image for Drupal 6.x with no idea it may be dropped. Which is really quite bad.

I submitted an image service the other week and have received no feedback at all. Looks like I'll have to convert it to an ImageField service going forwards. =(

Ps - I would normally use ImageField anyway, but sadly the module was not ready for 6.x when I embarked on my current project.

@greg.harvey - i'm using imagefield, filefield, etc on d6 (all alpha and beta versions right now) and it works well - there's also a long post with instructions for migrating from image.module to imagefield by moshe that may solve your problem:
http://drupal.org/node/201983#comment-880481

Image already in AcquiaDrupal so suppose Image never should be broken

Image already in AcquiaDrupal so suppose Image never should be broken

I don't think that's the case at all. Drupal maintainers don't necessarily take any notice of who else might be packaging their modules. In fact, I'd say most don't! If no one works on Image, no one works on it - regardless of whether it's part of some third party package or not.

I had a good chuckle at the Acquia announcement. I'm a developer on several of their listed modules and I haven't heard a thing from them or seen any bug reports/patches.

@drewish - i can't imagine that they won't hold to their word - they literally just launched, so i suspect that it will be a few days before everything rolls back...as far as dedication goes, that killer new theme for carbon was up on drupal within about 6 hours of the acquia announcement (6 hours), so i'm impressed...

As a newcomer to Drupal who's put in many days of effort on issues with Image and with the above patch for Image Attach, a phrase like

adopting Image for Drupal 6.x with no idea it may be dropped

is worrying. Had it not been for a crisis with a (non-Drupal) site I look after, I would have spent more time this week trying to find a trade-off between #292039: Image node can be created without any image and #298702: Properly validate image uploads that plugs all the gaps.

Apart from nailing down that problem, I have gone a long way with #103793: Add token support for image file directory and image names to the point where it's usable for my purposes.

So to hear that the "movers and shakers" favour ditching the Image module is less than encouraging :(

Don't be too disheartened, sp3boy. Many people still use the image module and, as is the great thing about open source, if enough people want it then it will survive. But there may be a period of inactivity while the maintainers sort themselves out (old ones leave and new ones take over). Actually, a good way of looking at this, if you're dependent on the Image module and want to see it survive, is as an opportunity to become a maintainer of a key module.

But another reason not to be disheartened is that the Image Field module is very good too, and the migration script zilla pointed out is a huge time-saver. You've done some great work with Image, and perhaps some of that can be applied to Image Field too? I understand why the community is moving towards the File/Image Field CCK approach, because it offers everything the Image module does, but with more flexibility. You should check it out. =)

There's nothing wrong with image, and image attach for that matter. I'll be continuing to use it as long as I can. I like metadata with my images, which imagefield doesn't do well.
I use image_attach, because I like to nominate just one representative image for my nodes --- which is why I don't care about this thread, it's trying to do something that imagefield DOES do better.
But I like one image, and so I use image attach, and image_attach is better at doing this one thing than imagefield is.

they both have their place, and are even slowly merging as imagefield starts getting more of the image.module features. There is no dead end here, there is even a trivial migration path possible, but I think image.module will be with us for a while.

Totally off topic, but:

I like metadata with my images, which imagefield doesn't do well.

What's the difference between a normal content type with Title, Body and a single Image Field CCK field, restricted to one item, and an Image content type as presented by the Image module? Am I missing something?

Good point about Image Attach though. An Image Field Attach module would be a good contribution to the community... =)

which is why I don't care about this thread, it's trying to do something that imagefield DOES do better.

Not really. I don't think Image Field has any ability for you to REFERENCE multiple images attached to other nodes, which is what this patch is trying to achieve.

What's the difference between a normal content type with Title, Body and a single Image Field CCK field, restricted to one item, and an Image content type as presented by the Image module? Am I missing something?

Very very little these days,
apart from automatic set-up, reduced dependancies, and a bunch of extra support from a large suite of existing modules,
but I basically don't see why there's any competition between the two approaches.

Just earlier I was imagining what it would take to make an image content type that truly DID behave like an image node. You can make it look the same on the surface, like you say, with a few minutes work. Although even that's a few more steps than simple users should have to do, and it's not the same from a compatability POV.
A bit of clever code tweaking inside can make it behave essentially the same eventually, just enough so it responds to all the image API things. I think that's one eventual path - image.module implimented as a special case imagefield node type preset. That would be an interesting proof-of-concept project - create a true hybrid. With full backwards compatibility with all the other image.module extensions.

You can, if you think hard enough, create something like image attach with a combination of the custom 'image' content type, and then node reference, and then a custom widget that works as nice as image_attach, and then some extra theming work. And it would all make sense. And could eventually be made to support multiple images sanely, although the UI and API implications are tricky.
But basically it's a configuration recipe, and not a cooked biscuit.

Or you could decide: I want:
- to be able to choose a single image to represent my node,
- to re-use them from a pool without re-uploading,
- to tag them, date them, own them and caption them,
- to select them from a list on the node edit form,
- or to upload them there and then
- to preview thumbnails of them on the node edit screen
- to have lots of control over how they are displayed
- to change or remove the images later and have the referring nodes handle that
- and be able to set that up in 2 minutes.

I say use image.module until there is a point where imagefield provides an advantage.

Both choices are fine. When I run into a solution I can't solve with image.module, I'll take on imagefield and see if it's more suited to my needs that day. Until then, I'll leave it to the others who are keen on improving it...

.dan.

Good post. Thanks! =)

I just wanted to provide some clarification here. The plan that's been tossed around is not to abandon the image.module. It's one of the most popular Drupal modules. The idea is to get the filefield and imagefield modules finished up in D6 then basically follow the plan dman laid out. Create a Image 6.x-2.x branch that moves the image storage into an imagefield and uses imagecache to generate the resized images. image_attach then depends on nodereference to store the linkages and becomes a custom field formatter. image_gallery will be done with views, etc.

The main goal is to provide an upgrade path for existing image users and an easy setup for new users.

Sounds awesome! I wouldn't mind helping out on this one. I've provided additional FileField CCK functionality before and I've spent the last month getting quite up to speed on the Image module because of the content migration modules (and web services) I've been writing. =)

So this patch is never going to make it?
I don't see much progress on the imagefield side, and image.module is working well. I also often post images in an image gallery and at the same time post a news report about it to promote it, into which I like to include some of the pictures (ideal situation for image_attach). With imagefield, I'd have to upload the image again.

It has made it as far as I'm concerned :) My first D6 site has just gone live with some News nodes having multiple attached images, themed appropriately. The code includes the patch from comment 81 plus the bug fixes I've suggested subsequently (although comment 89 may have been me confusing myself).

I guess if I hit any problems with the patched code, I'm just gonna have to fix it myself :)

You should re-post a final patch. =)

OK, it will take me a few days to get together because I'll have to strip out from "my" version some code that restricts the dropdown list of images available according to some personal taxonomy vocabularies, and I don't expect that will make anyone else's life easier.

I really like the idea of using filefield and imagefield, this will be a great benefit to both modules while allowing maximum flexibility (such as multiple image support, and the advantages of CCK) and usability (allowing users to easily setup an image gallery).

Well, with D7 this is sensible, because files finally get a real management through the drupal API. But I don't see how it could be beneficial for Drupal 6. I don't see the point of multiple images per node anyway. With the current system of imagefield, you cannot reference a single image in such a node, except by fid, which doesn't really work well with the other modules. Whats so bad with using taxonomy for grouping pictures? imho it makes perfect sense.
bot: thanks in advance sp3boy, i'm much looking forward to the patch.

OK here it is, built against HEAD. It has the same enhancement as the patch on comment 81, plus the tweaks I mention above:

• Retaining taxonomy selection when an image is uploaded for attachment
• Applying the weighting to the image_attach content item as a whole so it is picked up OK
• Adding an enclosing DIV around the attached images with a class of all-attached-images so that they can be themed more effectively around text

Having gone to the trouble of publishing the patch, I think a status change is appropriate.

I just tried out this patch and it works flawlessly. The patch applies only from the parent directory (over image/) though. I'm not sure about the teaser, is it desired to show only one image if more than one is attached?
Thanks for your great work already! Now we only need a visual browser to choose the image ;)

Ah, sorry about that patch issue - I'm a bit new to building them and it doesn't help that I seem to restructure my development folders every couple of months. I do test the patches (honestly) but obviously in a way that doesn't reveal that limitation. I'll try to fix that when I have a little spare time.

The teaser is indeed only showing one image - this is a feature of the baseline patch which I didn't mess with. For my site layout that was actually better than trying to fit multiple thumbnails into a teaser. I guess that's something that could be parameterised via the admin page if there was a demand.

Just a note - usually you'll be asked to provide a separate patch for each file affected, like this:
http://drupal.org/node/339754

=)

Regarding my earlier comment about ajax: I've had a delve into the innards of upload.module and it's pretty convoluted stuff. With javascript disabled, Upload module falls back to the same behaviour we have here, so that's fine -- I take it all back ;)
In theory, we could add in the funky ajax at a later date.

I've just noticed a usability flaw with the select box: it's too easy to lose the list of currently attached images. If you forget to SHIFT-click, or click by accident, the selection is lost. A user who doesn't know they can reload the edit form, or who has other edits already made that they want to save is scuppered :(

I'd like to see the currently attached images listed with a tickbox to remove them, and the select box (or a visual thumbnail browser -- but that could be a LOT of thumbnails...) be only for selecting more images to attach. This would also open up the way to ordering the images with the drag and drop widget. We'd need a weight column on the table for that.

I'd like to hear from database people -- is it ok that we have no index in the image_attach table?

i've been working on converting the audio_attach module which was based on the image_attach to use a CCK nodereference and thinking that's a much better long term approach. the only hitch is that i need to get some code for handing an upload and then sticking it into the cck field.

Hehe... today I've finally sat down and started a rough scaffold for something that's been brewing in my mind for ages: a generalization of image_attach. I've ducked the problem of how to embed generic node add forms within a node add or edit form by just adding a tab to nodes for managing and adding child nodes. I'm managing to create trimmed-down versions of the node add forms, though can't figure out how to elegantly hide authorship and options without getting anonymous unpublished nodes.
I wasn't going to go with noderef though -- no weights. And it seems much simpler to have a table of parent and child nids that I can manipulate myself rather than add information into the CCK field. I've also had trouble hiding the noderef field on the node edit form (for security, preventing unwanted editing etc): it doesn't work.

joachim, at least on the weights point it totally has weights... at least if you're doing the autocomplete (which IMHO is much more useful than the select drop down list). the big plus to using noderefs is that you get all the views code for free. take a look at the current audio_attach code to see what it's doing.

Looked at your code...
Sudden revelation at using noderef on PARENTS to point to children, rather than the other way round... it certainly clears up the problems I was having with user access (regular users should only attach nodes to nodes they own, but they probably should be able to attach anything to a node they own).
I made a noderef formatter to show the audio player a month ago or so -- sorry for not filing it as a patch.
But the noderef and formatter approach like this is limiting in that you can only attach one content type at a time. I guess a formatter could be made to show a view though. (An admin page could define new formatters, each based on a view?)

I haven't actually started working on the data side of things with my code, so there's no reason why it couldn't sit on top of noderef.
So far I'm working on getting a set of forms to add children and some kind of nice listing of existing children. Currently stuck on this though: http://drupal.org/node/340376

Some time ago I post my solution (http://drupal.org/node/267806#comment-890728)
Idea from node_images but image nodes instead files

There's Tab for every node that allowed to attach images
At the top - list of already attached images
Below - list of all images (already attached with disabled checkbox)

See screenshot

I'm a bit confused by comment 118 - I thought there was only one file affected, or is that not what Greg meant?

No need to worry, I think he looked at a different patch (perhaps the original from the thread starter). You patch seems fine to me.

Hi,

I was commenting on #116 else saying the patch was against the Image module's directory, not the file...

The patch applies only from the parent directory (over image/) though.

I was noting you should provide patches at file level, not directory level ... and *if* you have more than one, one patch per file. Sorry if I caused any confusion. You probably knew that anyway, but if others read it I was attempting to clarify the comment in #116 and why that was noted as something to fix. =)

greg.harvey, multiple patches is not recommended... just do a single patch for the entire change set.

My sincere apologies...! I had expressly been told otherwise in the past (can't find it now) hence me labouring under the illusion this was the case. I guess the person at the time must've been imposing their preference while implying it was community standard.

I stand corrected! :-/

Er... I sit corrected, having been under the same impression and even created patches on another issue as one-per-file.

If for example the entire change set were to encompass a file in image/ and a file in image/contrib/image_attach/, wouldn't the patch have to be made so that it ran from image/?

Either way here's a corrected version of my patch.

I applied patch #130. Now I get this error for each attached image:

user warning: Unknown column 'n.0' in 'where clause' query: SELECT n.nid, n.type, n.language, n.uid, n.status, n.created, n.changed, n.comment, n.promote, n.moderate, n.sticky, n.tnid, n.translate, r.vid, r.uid AS revision_uid, r.title, r.body, r.teaser, r.log, r.timestamp AS revision_timestamp, r.format, u.name, u.picture, u.data FROM dru6node n INNER JOIN dru6users u ON u.uid = n.uid INNER JOIN dru6node_revisions r ON r.vid = n.vid WHERE n.0 = '106' AND n.1 = '100' AND n.2 = '99' AND n.3 = '101' AND n.4 = '104' in /var/www/drurh/modules/node/node.module on line 749.

the column names are numbered. looks like a switched var name like using index instead of value :)

i tested with minipanels and added the "attached images" block. I can't find this block in admin/build/block. why that? (it's missing with standard image attach, too)
the attached images show in the block, but not in the node.

furthermore, eclipse could not apply this hunk on its own.
@@ -369,55 +428,55 @@ function image_attach_theme() {

nevertheless it worked by forcing it to apply it, but reverse patching will fail.

Do you have a override for theme_image_attach_body() in your theme? See comments 73 and 84 above.

Don't know about the block question, sorry.

I think the reason the patch now fails to apply cleanly is that a different patch has been committed to image_attach.module at the end of last month. So I will have to re-make the patch when I can find the time.

OK I found some time rather sooner than I expected. This patch should apply cleanly.

this patch does work flawlessly on 6.x-1.x-dev 2009-Jan-02
no error while applying the patch and the attached images do show up.

the block is mysterious. on rootcandy it shows up, but not in Acquia Marina or garland. strange thing

Will this become a part of the main release or the plan is to be maintained only as a patch?

adding this to cvs would be a big step forward. the module suggests for a long time being able to attach multiple images to a node. the block is called: attached images, the node edit section says: attached images)

Well I guess if enough people try my latest version of the patch and get it working OK we could progress the status of this issue on to "reviewed and tested by the community" after which I would hope that the patch will get officially adopted. I would certainly be in favour.

I think it's an excellent idea. +1 to getting it properly tested. I see no reason why it wouldn't be included. =)

Looking pretty good. Mainly coding standards issues. I know that image isn't very consistent but I'd like change that.

+        foreach ($node->iid as $iid) {
+          $image = node_load($iid);
+          $form['image_attach']['image_thumbnail'][] = array(

could we use $iid as the array key?

           '#options' => _image_attach_get_image_nodes(),
           $value => empty($node->iid) ? NULL : $node->iid,
-          '#description' => t('Choose an image already existing on the server if you do not upload a new one.')
+          '#description' => t('Choose an image already existing on the server if you do not upload a new one.'),
+          '#multiple' => TRUE,
+          '#size' => 6

a multi-select seems like a horrible interface for picking several files...

+        foreach ($node->iid as $iid) {
+          $form['image_attach']['iid'][] = array(
+            '#type' => 'hidden',
+            $value => empty($iid) ? NULL : $iid,
+          );
+        }

can we make this a value? much more secure than a hidden

+      //additional submit button which adds image and brings you back to node edit

Needs to be a proper sentence.

 /**
+ * This submit function adds a new image and returns you to the
+ * node edit form directly afterwards, without creating the new node yet
+ */

PHPDoc blocks should start with a single sentence explaining their purpose. The have additional paragraphs as needed. Also needs to be a proper sentence and end with a period.

+  // Rebuild the attached image data

Needs a period.

+  	db_query("DELETE FROM {image_attach} WHERE nid=%d", $form['nid']['#value']);

use spaces rather than tabs for indenting.

+    $tnode = new stdClass();
+    $tnode->taxonomy = $form_state['values']['taxonomy'];
+    $form_state['values']['taxonomy'] = taxonomy_preview_terms($tnode);
+    unset($tnode);

$tnode is a horrible name. $taxonomy or $term seem more appropriate. See what core does elsewhere and copy that.

+  // Rebuild the node edit form

Needs a period.

+      // This line changed to append to array of images instead of reset the single value
+      $form_state['values']['iid'][$image->nid] = $image->nid;

The comment should describe what's going on rather than the change made in the patch.

+      while($iid = db_fetch_array($res) ){
+        $iids[] = $iid['iid'];
+      }

Need a space between while and opening paren and remove the space between the closing parens.

-        $node->content['image_attach'] = array(
-          '#value' => theme("image_attach_{$teaser_or_body}", $node),
-          '#weight' => variable_get("image_attach_weight_{$teaser_or_body}_{$node->type}", 0),
-        );
+        $node->content['image_attach'] = array();
+        $node->content['image_attach']['#weight'] = variable_get("image_attach_weight_{$teaser_or_body}_{$node->type}", 0);

I think you could just drop the '#value' line and leave the '#weight' inside the array.

+function theme_image_attach_teaser($node, $iid) {

Need to document the parameters.

+function theme_image_attach_body($node, $iid) {

Same goes for this one.

Thanks for spending some time on this. I guess the ball is in my hands as far as following up on your points, even though the formatting, comments etc. are largely "as I found them"!

I agree that multi-select is not ideal - if there is a large number of images a single line of text doesn't make it easy to pick the right one(s) whereas displaying more info about each one would take up screen space.

Will try to make time for it in the next few days. I've now got three sites where this functionality is important.

I'm sure I speak for many when I say your effort is hugely appreciated, sp3boy! Wish I had some time to help... =(

OK hopefully I've rectified all the straightforward points from comment 139.

I'm open to suggestions of how to display 'attachable' existing images better. On my first Drupal site, there are 133 images eligible for (multi) attachment, so multi-select is at least able to cope with that concisely.

I think there's no perfect interface for everyone. It's not important for now, but a future option might be to choose the selection interface for existing images - AJAX look-up, drop-down menu or perhaps even an optional scrolling div showing thumbnail and clickable title? =)

But as I say, this is a feature request - definitely not important for finishing this and getting it applied to the next release.

Patch against latest HEAD.

Sorry, only a code review from me for now:

+          '#size' => 6

Always add a trailing comma to multi-line arrays.

+      $form_state['values']['iid'] = is_array($form_state['values']['iid']) ? $form_state['values']['iid'] : array($form_state['values']['iid']);
...
+          $node->iid = is_array($node->iid) ? $node->iid : array($node->iid);
...
-      if ($node->iid) {
+      if (count($node->iid)) {

This should use if (!is_array(...)) { ... } instead. However, in general we should not use two different data types for the same property. Either we store those iids in a separate property, or we always store an array (which probably requires further changes to other image* modules). It seems like the rest of the code that is introduced here always assumes an array for $node->iid though... Is it possible that we are trying to abuse the regular $node->iid property of image.module here? This needs to be much cleaner. I would not mind if Image Attach would use an own property, e.g. $node->image_attach.

+  // Rebuild the node edit form.
+  node_form_submit_build_node($form, $form_state);
+
+}

Remove this blank line, please.

+        } else {

Please read http://drupal.org/coding-standards

+            $content .= theme("image_attach_{$teaser_or_body}", $node, $iid);

E^ALL notice - $content is never declared.

+ * @param $iid
+ *   Nid of image to display.

While technically correct on a low-level view, it's still an image id by Image module. Otherwise, the argument would have to be named $nid.

+function theme_image_attach_body($node, $iid) {
+  $output = '';
   $img_size = variable_get('image_attach_size_body_'. $node->type, IMAGE_THUMBNAIL);
 
   if ($img_size != IMAGE_ATTACH_HIDDEN) {
     drupal_add_css(drupal_get_path('module', 'image_attach') .'/image_attach.css');
 
-    $image = node_load($node->iid);
-    if (!node_access('view', $image)) {
-      // If the image is restricted, don't show it as an attachment.
-      return NULL;
-    }
-    $published = $image->status ? '' : ' image-unpublished';
-    $info = image_get_info(file_create_path($image->images[$img_size]));
-
-    $output = '<div style="width: '. $info['width'] .'px" class="image-attach-body'. $published .'">';
-    $output .= l(image_display($image, $img_size), "node/$node->iid", array('html' => TRUE));
-    $output .= '</div>'."\n";
+      $image = node_load($iid);
+      if (!node_access('view', $image)) {
+        // If the image is restricted, don't show it as an attachment.
+        return NULL;
+      }
+      $published = $image->status ? '' : ' image-unpublished';      
+      $info = image_get_info(file_create_path($image->images[$img_size]));
 
-    return $output;
+      $output = '<div style="width: '. $info['width'] .'px" class="image-attach-body'. $published .'">';      
+      $output .= l(image_display($image, $img_size), "node/$iid", array('html' => TRUE));
+      $output .= '</div>'."\n";
   }

Wrong indentation here.

#223331: no log entry for images create as attachments has been marked as duplicate of this issue. Image Attach should add a watchdog message for new created image nodes. This should be incorporated into this patch.

subscribing

OK here are some more improvements. Formatting should be fixed, indenting corrected etc.

There are some bigger changes as well:

• I agree about it being unclear when $node->iid and $form_state['values']['iid'] are arrays or not. Hence I have updated the code so that these references should always be to an array. I have changed the property / array key names to "iids" throughout to reflect this.
• I found that if the first "None" item on the multi-select was picked as well as another image on the list, it could result in a zero iid being inserted into {image_attach}. There are now stricter checks each time an iid is referenced that it has a value.
• I have added a check to image_attach_validate() to warn the user if they click the Attach button without selecting a file to upload. Otherwise the action fails silently but image_attach_image_add_submit was still being executed.
• I think there was a bug in the RSS item handler of image_attach_nodeapi, due to confusion over when $node->iid was or was not an array.
• I have added one watchdog message as requested when a new image file is uploaded, but there are very few similar watchdog calls in other parts of Image.
• I realised that the recently added database index on {image_attach} will not work with this patch. Hence the patch now includes a suitable change in image_attach.install for hook_schema() and an update for the indexing. Update.php should be run after applying this patch but anyone who has tried an earlier version of this patch, has multiple rows for the same nid in {image_attach} but has not run the previous updates already, may find that part of the 6100 update will fail. However the index should then be created correctly so that particular error can be ignored.

I have also noticed errors from image_access() when creating a new uploaded image. I do not feel they are particularly related to this feature patch so have not looked into the cause or remedy.

Re-made against latest HEAD.

Hi,

Subscribe!
Any news on this one? Do you guys think it will make it to core soon enough?

Also, does this patch apply to D6?

Thanks,

Merc.

Who knows? I responded as quickly as I could to the concerns in the mid-January comments but as you can see there's been no response to my work for a month or more. What's even more frustrating is that the same is true of #298702: Properly validate image uploads which is a pretty fundamental validation bug fix.

The patch that is up for review is for D6 only.

Sadly, image attach module is awful and it's really terrible thing to know, since ability to attaching images is an essential function in every cms system. Attaching multiple images is only another feature which doesn't works, but what about updating from old version into the new? Well apparently nobody bothered to check if this works at all… http://drupal.org/node/142783

I'm not a php dev. though even if I did, I wouldn't touch this module because it's a box of worms and bugs, and repairing such a thing is horrible. I'm into theme development and I know that sometimes only thing that you can do is start from a scratch.

@sp3boy: Sorry, I can feel your pain. However, if you look at my view of things, you'll notice that I'm trying hard to keep everything in shape, which requires a lot of time on its own. I marked several other issues as duplicate of this one and the other one in the past days, in the hope that some other developers would join your efforts. If you are on IRC, the best thing you could do is to "trade" a patch review and test with someone else in #drupal, #drupal-support, or #drupal-wysiwyg, who is in a similar situation like you - that is known to be the best practice to gain traction on issues.

How's this looking by now? I tried using the latest patch, but got the following error when applying
Hunk #10 FAILED at 444.

This relates to all the teaser and node styling, so while the node edit page seems to be working great, I can't get any images to show in the post itself

I really appreciate all the work being put in to migrating image attach fully to D6 - this multiple image issue is fairly critical to all my sites

Sorry, I'd actually re-made the patch (again!) weeks ago but not got around to posting it. I just tested this against HEAD so you should be all right.

Please post your test results if you can get the patch applied.

sp3boy, you're a lifesaver - the patch works.

I've managed to create a View with Row Style = Node (full node), and it's displaying all attached images fine. It doesn't seem to work for Teasers though, but maybe that's on purpose.

Some tips:
- you must enable 'select existing images' from the Image Attach configuration page to attach multiple images.
- To remove one of the multiple attached images after saving, deselect it from the list of available images and then save again

Top work, thanks again

sp3boy, you're a lifesaver - the patch works.

I've managed to create a View with Row Style = Node (full node), and it's displaying all attached images fine. It doesn't seem to work for Teasers though, but maybe that's on purpose.

Some tips:
- you must enable 'select existing images' from the Image Attach configuration page to attach multiple images.
- To remove one of the multiple attached images after saving, deselect it from the list of available images and then save again

Top work, thanks again

+1 to getting this into the core image contrib.

Great work, sp3!

Test #155 idea is great it's easy to make it with preview as I show in #124

I was test last patch with 3 langs enabled so it works fine except case: when you change lang of main node widget is not updates with current lang imageset :) but it works!

+1 to include put it in image_attach and make new issue about usability

I would agree with #159 -- for an issue this important that has been open for so long, we should commit something that *works* and think about usability and good UI in a second phase (as long as we don't leave that too long! ;)

However, it's best if the data storage part is right first time, and I am concerned about the lack of a delta for image attachments. There is no way to order the attached images arbitrarily and I am sure this will be requested as a feature.

What about getting delta column into the patch, even if for now it doesn't have a UI and merely stores the order the images are attached in?

I see 2 edges

1) delta but Weight is more preferable (more common) this column is required to make lightest image (ex. for cover, views)
2) UI - why ugly listbox as we have sortable tables

So before commit I prefer to make weight column for attached_images and little rework UI

Opinions?

Delta / weight: basically a number that indicates order.
The difference is:
- delta: runs from 0 upwards, in sequence. Each is unique. So 4 items will have 0 1 2 3.
- weight: runs from -100 to +100 (or whatever number). Can have gaps and items can have the same value.

CCK uses a delta value -- the tabledrag JS appears to mess around and sometimes put in negative values while you're dragging as if they were a weight, but these are probably normalized somewhere in the submit process.
I think we're better off following that approach.

Is the patch in post 155 the current one? I tried applying it against version 6.x-1.0-alpha4 of the image module and got the following:

(Stripping trailing CRs from patch.)
patching file image_attach.install
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 107.
2 out of 2 hunks FAILED -- saving rejects to file image_attach.install.rej
(Stripping trailing CRs from patch.)
patching file image_attach.module
Hunk #12 FAILED at 469.
Hunk #13 succeeded at 476 (offset -1 lines).
Hunk #14 FAILED at 499.
2 out of 14 hunks FAILED -- saving rejects to file image_attach.module.rej

What version of the module should I apply it against?

I just applied it to 6.x-1.x-dev an it works OK

(subscribing)

@@ -107,3 +107,12 @@ function image_attach_update_6100() {
+}
Index: image_attach.module

Please add another newline to the end of the file.

@@ -77,15 +77,19 @@ function image_attach_block($op = 'list'
+                  $content .= '<div class="attached-image">'.l($img, "node/$this_image", array('html' => TRUE)).'</div>';

1) Should we move the DIV outside of the foreach loop here? Not sure.

2) Wrong string concatenation. Please use a space before _and_ after the dot.

3) "$this_image" looks a bit wrong. At least, it should be "{$this_image}" - however, $this_image is not defined anywhere, so I can only guess that should have been simply "{$image->nid}".

@@ -77,15 +77,19 @@ function image_attach_block($op = 'list'
+            $output['content'] = '<div class="all-attached-images">'.$content.'</div>';

Wrong string concatenation here, too.

@@ -77,15 +77,19 @@ function image_attach_block($op = 'list'
+            foreach ($node->iids as $iid) {
+              if ($iid) {
@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
+      $iids = array();
+      while ($iid = db_fetch_array($res)) {
+        $iids[] = $iid['iid'];
+      }
+      return array('iids' => $iids);
@@ -185,26 +187,32 @@ function image_attach_form_alter(&$form,
+        foreach ($node->iids as $iid) {
+          if ($iid) {
@@ -213,10 +221,14 @@ function image_attach_form_alter(&$form,
+        foreach ($node->iids as $iid) {
+          if ($iid) {
@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+      foreach ($form_state['values']['iids'] as $iid) {
+        if ($iid) {
@@ -271,10 +329,14 @@ function image_attach_nodeapi(&$node, $o
+          foreach ($node->iids as $iid) {
+            if ($iid) {
@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
+          foreach ($node->iids as $iid) {
+            if ($iid) {

I don't get why we are constantly checking for if ($iid) all over the place, while image_attach_nodeapi($op = load) adds a straight database result?

@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+      // Provice an additional submit button which adds image and brings you back to node edit.

Typo in "Provice". Also, this comment does not wrap at 80 chars.

Additionally, many shortcuts in here - how about:

"Provide an additional submit button, which adds an image and redirects the user to the node edit form."

@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+        '#submit' => array('image_attach_image_add_submit'),
+        '#validate' => array('image_attach_validate'),

Please always declare #validate before #submit.

@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+    db_query("DELETE FROM {image_attach} WHERE nid=%d", $form['nid']['#value']);

There should be spaces between "nid", "=", and "%d".

@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+          watchdog('image', 'Image %iid attached to node %nid.', array('%iid' => $iid, '%nid' => $form['nid']['#value']), WATCHDOG_NOTICE, l(t('view'), 'node/'. $iid));

Huh? Why a watchdog message here (at all)?

@@ -228,8 +240,47 @@ function image_attach_form_alter(&$form,
+  // Convert taxonomy format from Preview to Object.
+  if (module_exists('taxonomy') && !empty($form_state['values']['taxonomy'])) {
+    $temp_node = new stdClass();
+    $temp_node->taxonomy = $form_state['values']['taxonomy'];
+    $form_state['values']['taxonomy'] = taxonomy_preview_terms($temp_node);
+    unset($temp_node);
+  }

This looks a bit scary, but makes most likely sense. A comment would be helpful.

@@ -238,17 +289,24 @@ function image_attach_form_alter(&$form,
   if ($file = file_save_upload('image', $validators)) {
...
+    // Only raise error if user clicked specific Attach button.
+    if ($form_state['clicked_button']['#value'] == 'Attach') {
+      form_set_error('image_attach', t('No image was selected to upload and attach.'));

The error message is a bit fuzzy. I think the file is not an image, or may be invalid due to some other reason. Maybe you'll find a similar error message in the existing code to replace this here.

@@ -271,10 +329,14 @@ function image_attach_nodeapi(&$node, $o
     case 'insert':
     case 'update':
-      if (isset($node->iid)) {
+      if (isset($node->iids)) {
...
+        if (count($node->iids)) {

If you replace the first condition with !empty(), then you can safely skip the second condition.

@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
+      $res = (db_query("SELECT iid FROM {image_attach} WHERE nid=%d", $node->nid));

Please remove the wrapping braces around db_query().

@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
-      if ($node->iid) {
+      if (count($node->iids)) {

Conditions like this we want to replace with !empty().

@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
+          $node->content['image_attach']['#value'] = '<div class="all-attached-images">'.$content.'</div>';

Wrong string concatenation here.

@@ -284,28 +346,41 @@ function image_attach_nodeapi(&$node, $o
     case 'rss item':
       $ret = array();
-      if ($node->iid && $image = node_load($node->iid)) {
+      if (count($node->iids) && $image = node_load($node->iids[0])) {

Please also use !empty() here.

@@ -372,18 +444,23 @@ function image_attach_theme() {
+ * @param $iid
+ *   iid of image to display.
...
+function theme_image_attach_teaser($node, $iid) {
@@ -400,15 +477,21 @@ function theme_image_attach_teaser($node
+ * @param $iid
+ *   Nid of image to display.

"iid of image" is a bit cryptic -- "Node id of image to display." would be more accurate, I think.

@@ -400,15 +477,21 @@ function theme_image_attach_teaser($node
+ * Theme an image shown in body
+ *
+ * @param $node
+ *   The node object to theme.
+ * @param $iid
+ *   Nid of image to display.
+ *

Please remove the blank, last PHPDoc line.

This review is powered by Dreditor.

I think calling the theme function in a loop is the wrong way to do it -- theme all the images together so a wrapper DIV can be placed around them all, for example. There may be a case for then theming each one too.
I want to completely change the theming layer for this module anway: #412288: restructure theme_image_attach_body/teaser .
So let's leave the theming stuff in this patch as is, and as soon as this patch gets in, I'll look at that other issue.

I wanted to get Lightbox 2 working with multiple image attachments to display a slideshow.
I have managed to get the patch installed after I uninstalled the image module and re-installed it with the patch.
However, I was wondering is it possible to haev just the first attached image showing that when clicked goes to the slideshow rather than all thumbnails of attached images appearing?

thanks :)

Probably not -- lightbox takes all images it finds.

Can you confirm you've tested the patch in #155 and it works?

The patch in #155 won't have worked since Commit #227620, no. I will try and get the requests from comment #166 built in to a new patch in the next day or two.

That would be awesome.
Let's see if we can get image module release in time for or at conference! :D

OK I think I've hit all the definite points from #166, taking into consideration the point about theming in #167...

I've tested against Image HEAD. The warning in #166 about the undeclared variable in the image_attach_block() function was a bit of a giveaway that I have not myself previously tested the block display. Well I've fixed the bug and done that now, however it is necessary to apply #426724: Attached Image block doesn't appear after initial install, until manual database edit to be able to see the Attached Image block in the admin page and assign it to a region.

I hope this moves us forward. Unfortunately I will now be unavailable for a week or so.

Note that if #426724 is committed first, it will be necessary to change the image_attach_update_6101() function name in this patch, because #426724 also uses it.

#162: "There is no way to order the attached images arbitrarily and I am sure this will be requested as a feature. What about getting delta column into the patch, even if for now it doesn't have a UI and merely stores the order the images are attached in?"

+1

If anyone's interested in the one image display go to this post:

http://drupal.org/node/349166 :)

hope it helps somebody

Let's get the order field into the DB even if it's just sitting there doing nothing.

Also, I'm committing #426724: Attached Image block doesn't appear after initial install, until manual database edit so the update function needs to change to 6102.

OK here is a patch with a dummy weight column on the {image_attach} table, no UI but populated with values 0... 1... in the same order that the images are uploaded for attachment (or the order in which existing images selected from the form's list appear).

Update function name changed.

If testing against Image HEAD, the derivatives will probably not get created for images that are uploaded during parent node creation, which I think is connected with #226121: don't manipulate images on hook_load. The bulk rebuild will create them.

Sorry, here's a replacement patch - I'd overlooked one function where the weight column should have been populated.

Another replacement patch I'm afraid - I had not changed the hook_schema to include the new weight column during module install.

Applied (with fuzz due to other changes elsewhere, but all fine).

Seems to work perfectly. Weight column keeps up with images added, removed, etc.

This patch is the latest in a long line, and those have been tested too -- so I'm going with it.

Committing, fixed, and big thank you to maulwuff, greg.harvey, sp3boy, and everyone who's persevered with this long-running issue!