On deleting images, the database entry remains

I agree. I would also love to see this too although I believe you could do it with just a little extra code in you template file. Unfortunately, I don't know what the code would need to be.

that looks good. Now my question is how could you do this with the imagecache print theme?

If someone want to undertake this task, we can add a default image field to to the 'field settings' with an upload like user pictures. Nothing fancy, and modify the theme function to use that file if (!is_file(...)). I don't have the bandwidth to get around to it at the moment, but would be happy to commit such a feature.

Great feature to have!

I am getting x on nodes without pictures in Views Table and Views List.

This feature could be accomplish as follows:

option (1) - Imagecahe only
- Assign a default image for each preset.
- Display default image when cck node image not available.

option (2) - Imagecache with Profile module
- Assign default image per user
- Display default profile picture when cck node image not available.

Other options may be preferred.

Darly

sorry to move this to a bug.. but the bug i am seeing somewhat fits with this.. and breaks the sugegsted solutions..

i have cck nodes which i guess must have failed trying to get imagecache to upload pic correctly (i was/am having a lot of issues with this last night)... and now i get code that looks like:

<div class="field field-type-image field-field-picture">
  <h3 class="field-label">Picture</h3>
  <div class="field-items">
          <div class="field-item"><img src="http://l.rv.liquidcms.ca/files/imagecache/content_product_thumbnail" alt="" title=""  /></div>

      </div>
</div>
</div>

so imagefield has put ni the the empty path since i suspect imagecache didnt return a image file name.

this casues a few issues:

- it doesnt show as being populated if i edit the node - so i can't fix the situation (short of going into db directly)
- my pages load VERY slowly since i guess browser )Firefox) is kinda stupid when it cant find images
- the default image idea of checking the field entry first won't work since there is "something" there.. just not anything valid.

imagecache didn't recieve a filename. Something else is wrong, and this one will remain a task/feature request. The real problem is hanging entries in imagefields, which is partially a cck issue and neds more research. I don't remember where the issue for that is off the top of my head though. It is in the queue though.

a temporary (workaround) solution for empty image fields: substitute the dreaded MSIE browser default missing image placeholder with a transparent gif, as follows:

1.- add, in imagecache.module in "Theme an img tag for displaying the image" and also in
"Verify the image module and toolkit settings"

name="default-missing-image" onError="SubstituteDefaultMissingImage()"

2.- add in your page template:

<script>
if (navigator.appName=="MSIE")
function SubstituteDefaultMissingImage()
{document.images['default-missing-image'].src = '/resources/images/transparent.gif';}
</script>

Firefox and Netscape do not display any default placeholder for missing images.

this is presented only as a temporary remedy - the issue of empty image fields must be resolved from within the module itself.

the essence of the matter is that no output should be created if no image has been uploaded, so this issue must be addressed from within the module code as soon as somebody finds the time, this being a critical flaw.

revisiting the workaround:
strange things happen when it functions, which I currently do not understand and have to investigate : so I would advise you not to use it in its present form, as ineffective.

hope the developer of imagecache may provide a solution to this issue soon, and respond on this matter here - but in view of the fact that this issue is outstanding since early October 06 (meaning 6 months now) I would not be the one to hold my breath . . . . .

lamentably, this in many cases is the drupal way . . .

this is not a feature request as previously designated - it is a bug report.

empty imagefields should not generate any html output.
it is because an empty imagefield generates (erroneously) html output that a browser default image is presented, and this should not happen.

Ok guys I fixed it

in imagecache.module

replace this: (on line 684)
return '<img src="'. $imagecache_path .'" alt="'. check_plain($alt) .'" title="'. check_plain($title) .'" '. $attributes .' />'; }

with this:

  if ($path) {
  return '<img src="'. $imagecache_path .'" alt="'. check_plain($alt) .'" title="'. check_plain($title) .'" '. $attributes .' />'; }

Now imagecache will not return an anything at all if an image file was not uploaded. Enjoy!

Sorry, a little typo up there. There is no "}" on the original code from line 684. Either way, as long as you replace line 684 with the new code it will work.

@diovi : many thanks for looking into this.
but issue seems to be deeper with imagechache than that - and it has not gone away with the fix proposed.

issue steps to reproduce:
- create cck imagefield
- upload image (through imagefield)
- view image on page (i.e. confirm it exists on the page)
- delete image (through imagefield)
- you still get html output for an image that does not exist, thus a browser default image (square with red x)

image entry in database apparently is still there, not deleted.

Also, if this feature is going to be implemented into imagefield/cache please post an actual patch we can apply. The issue is fixed when it has been committed to CVS.

Attaching a patch that doesn't fix the real issue (getting the image deleted from the database), but should be applied anyways (defensive coding) as it avoids possible other cases when the file is not available but would otherwise be displayed. (As far as I can see, imagefield doesn't display the file anyways - in opposition to imagecache - but with this patch, less work is done to "not display it".)

This is the same patch as for this issue in imagecache, only transferred to imagefield. Applies to the current DRUPAL-5--2 branch.

Might be a good idea to rename the issue, because it's actually quite unrelated to imagecache (except that it only shows there). Also, I think it's grave enough to assign "critical" status to this issue, because this is really about database consistency and hard to recover from once it's introduced into the database entries.

At my place, the remaining database entry resides in {content_type_TYPE}. Knowing that CCK does all kinds of refactoring when the field layout changes, it could be that the concerned entry is somewhere else in your database.

See also : http://drupal.org/node/97861 (CCK - data records not deleted when a field instance is removed)

I'm switching this back to active. The proposed output update was applied, but the issue itself isn't resolved.

> I'm switching this back to active. The proposed output update was applied, but the issue itself isn't resolved.

Actually, I don't see this in either of the commit logs of filefield, imagefield, or imagecache, as well as it doesn't show in cvs up. So, pardon me if I switch back the status until it has been applied, or is declined.

The patch you've posted does not apply to the issue. It only deals with covering the symptoms on output. I've applied a variation of your patch to the drupal-5--2 branch of imagefield. The orphaned values in the database are not addressed by you patch at all, please leave the status of this issue set to active until a patch relevant to the issue is ready to be reviewed.

patch is attached, if you want to pursue the patch further please open a new issue for it.

I got a patch working for filefield that solves the database issues, you can find it here. Should be a breeze to port over to imagefield, but I've got no more time right now.

@jpetso and @dopry : big thanks to both of you for your efforts - issues at other modules are not so fortunate to attract solutions.

@dopry : regrettably patch does not work : neither does it display a default image (even though url for image provided).

This issue is closed an continued in... http://drupal.org/node/137833

Jpetso, why did you post a patch for this particular issue in a new issue? I understand this issue is currently quite confused with several subjects... So I'm closing it, so hopefully we can remain on topic.

abramo, this issue queue has absolutely nothing to do with default images or themed output. please do not raise that issue here or in the new issue jpetso opened for the original orphaned values in database issue.

please open a new issue for un related bugs that you aren't 100% sure are related. Its much easier to triage and mark dupes than it is to unconfuse a single issue.

marcob, When I have a fix for d5, I'll backport to 4.7. My energy is focused on Drupal 5/Drupal 6 currently.
4.7 will probably remain in bug fix only mode, and the 4.7.x-2.x branch will probably never be released with new features. I would suggest upgrading if you want the latest and greatest goodies.

> Jpetso, why did you post a patch for this particular issue in a new issue?

Because the patch is for filefield, not imagefield. I planned to port it over to imagefield and post the ported version here.

Here's the database consistency patch, ported to imagefield. See the related filefield issue on the how it works, what it fixes, etc.

I'll take the liberty to reopen this issue, as the patch is similar to the filefield one but not the same. Also, having it committed in filefield doesn't mean it's fixed in imagefield, and vice versa. So I think those should be two separate, even if related, issues.

@dopry: very upset with your mannerisms.
its this arrogance or is it something much worse?

THE ORIGINAL TITLE OF THIS THREAD WAS:
"default image for empty imagefields"

you have highjacked the thread, and now you are attempting to force everybody else out of it.
if you have no desire to provide a solution, TO THE ORIGINAL ISSUE, so be it.
but *please* keep your arrogance to yourself.

@jpetso Thanks for your patch, I'll review and commit as soon as possible. At first glance it looks good.

marcob, complaining that the module author is arrogant isn't likely to speed development. Reopening the issue is more effective and less offensive. When there are 2-4 issues open for the same reason (which happens very often) maintainers try to close the duplicates to keep the conversation focused. It wasn't meant to force anyone away from the issue, we're all trying to make the best software possible.

this issue is open since october 06 - almost 7 months now.
for me, this says it all.
I have nothing more to add.

@abramo: Actually, the original title was "Imagecache issue with empty imagefields", and the problem was that there is a does-not-exist placeholder where the image should be. Placing a default image was one of the proposed solutions, but not the bug that has lead to this situation. So in fact it was me who hijacked the issue, while trying to fix the underlying bug (database inconsistencies).

dopry is really busy, and imagefield is only one of a big heap of modules and code that he has to take care of. In general, module maintainers can't be expected to implement all features that are requested by all the people, as a maintainer you've got to set priorities or you'll run out of time and motivation quickly. This is open source, so the golden rule goes like, "Enjoy what you get, improve what you can do by yourself, or pay others to implement what you want". Everything else is voluntary, and dopry has contributed too much good stuff to be called ignorant. Really.

NO : the original title was : "default image for empty imagefields"
(see post #17)

contributing a solution, or not, is anybody's privilege.
flaming others in an arrogant manner is not.

let's get back to the task at hand and try to resolve coding issue as stated.

@coupet : correct.

@abramo, I'm just trying to keep issue queues short and to the point. I don't really have time to read through the entire history of this thing, but it was opened as a feature request for adding default images. Change to a bug because broken images were regularly displaying and that is why users wanted default images, several attempts to fix by massaging the theme function were offered. Jpetso notice the underlying problem that empty array is making its way into a function that should not be called with an empty array.

I don't really like the idea of defensive coding, which is what we're doing by altering the theme. We are only fixing a symptom of an underlying problem. Theme output doesn't get cached for many users, while the output of field loads do get cached. In the end solving the underlying problem will lead to better, faster, stronger.... too much heinlein for me... code.

I'm not trying to be arrogant, but this issue queue is somewhat my responsibility and I want to keep the signal to ratio high. I may be abrasive in the process.

If you really want to engage the process of seeing this bug properly resolved please test jpetso's patch. If you really want default issues or to update module provided theme functions, please open them as tasks or feature requests in new issues.

@jpetso,

Looking good so far... I'm still curious about

           if (!empty($file)) {
-            $node_field[$delta] += _filefield_file_load($file['fid']);
+            // only load the file if it has got a valid database entry
+            $file = _filefield_file_load($file['fid']);
+            if (empty($file)) {
+              unset($node_field[$delta]);
+            }
+            else {
+              $node_field[$delta] = array_merge((array)$node_field[$delta], $file);
+            }
           }

so if we got data from field.module in node_field and don't get anything from the files table (no file)
we unset the delta... this would be fid==0 in most cases, which is representative of an empty single value field stored in the content_type_* tables? if we do get results we merge it all into the node_field['delta'] and the world is happy?

if so that is the kind of stuff I like to see documented inline... the external dependencies and interactions that might not be readily apparent when looking at this particular conditional just within the context of imagefield. Although, I like captian obvious comments as well, they're better than no comment.

I hope to actually get around to running the issue queue and testing again on friday. I didn't manage it on sunday as intended, the first spring like day lured me outdoors.

also does that also pertain to single value fields shared across multiple content types? it's a case where a field will have it's own table, but $multiple = FALSE.

@dopry : I am in total agreement with your statements : imho as well, it is the correct approach.
(minus any abrasiveness, of course, now in the past - needless to say important contributors like dopry deserve some behavioural leeway, not extended to other mere mortals, but up to a point . . .)

I have tested all proposed patches in a clean fully-functioning local test site (exact issue steps are as in post #21) and they do not work. if this is of any help, I can test very easily any proposed patches with this test site, so I am at your disposal for "consumer testing by programming-challenged persons". clarification: everything I have tested up to now refers to 5.x-1.x-dev as stated at the top of this thread, the most recent version. if something else, other than the latest version of 5.x-1.x-dev needs to be tested (with patch applied) please let me know.

@dopry: Good point with the inline documentation, I'll write up a few additional lines and add them to the patch.

> also does that also pertain to single value fields shared across multiple content types?
> it's a case where a field will have it's own table, but $multiple = FALSE.

At the moment I don't know, but if I read content.module:427 (content_field($op='update')) the right way, then what I do is correct. Quoting those concerned lines from content.module:

case 'update':
  if ($field['multiple']) {
    // Delete and insert, rather than update, in case a field was added.
    db_query('DELETE FROM {'. $db_info['table'] .'} WHERE vid = %d', $node->vid);
  }
  (...)

Which means that CCK only deletes database entries when $field['multiple'] == TRUE, and I got my condition from there. Might be a bug in CCK, but at the moment it seems to me that I'm doing the right thing. Gotta test this use case nevertheless, I'll report back afterwards.

@abramo: My patch is for the DRUPAL-5--2 branch from CVS, and I'm testing solely on that one. I don't know what has changed since the original DRUPAL-5 branch (which 5.x-1.1 is deriving from) but it may well be that the difference between the two branches is what makes my patch unfunctional. Do the patches even apply to 5.x-1.1? What does 'patch' say when it merges the patch?

New version of the patch, with better documentation. Hope you like it this way :)

(once more:)
> also does that also pertain to single value fields shared across multiple content types?
> it's a case where a field will have it's own table, but $multiple = FALSE.

Yes, you're right, the database entries for such fields are not deleted but only assigned the default value. Nevertheless, I think it's ok to go with content.module: when the condition for deleting database entries (as mentioned above) changes, we will change ours as well. Till then, no chance to get those entries deleted in a clean way, even if CCK has them in a separate table. The $op='load' check removes them nevertheless, so it's not really tragic. Also, being single-value fields, the amount of unnecessarily stored database entries is limited, and will be reduced again when CCK introduces a proper delete condition.

I say let's apply the patch now and look into this corner case (well, not really, but still) later on.

Subscribing

Subscribing

BTW: Until this issue is worked out, the best workaround to prevent empty img tags in your content appears to jyamada1's tip to adding an if ($path) in a custom theme_imagecache in your template.php

Subscribing

BTW: Until this issue is worked out, the best workaround to prevent empty img tags in your content appears to jyamada1's tip to adding an if ($path) in a custom theme_imagecache in your template.php

"adding an if ($path) in a custom theme_imagecache in your template.php"
meaning? can you please provide example code for the benefit of the programming challenged?

Put this in your theme's template.php:
<?php
/**
* Overriden to include temporary measures to prevent empty
* image fields from being displayed as broken images.
*/
function phptemplate_imagecache($namespace, $path, $alt = '', $title = '', $attributes = NULL) {
if ($path) {
$attributes = drupal_attributes($attributes);
$imagecache_path = file_create_url(file_directory_path() .'/imagecache/'. $namespace .'/'. $path);
$output = '';
}
else {
$output = NULL;
}
return $output;
}

:| Sigh - Forgot the php closing tag. No pretty colors for me.

@zach : many thanks indeed : shall test and report back

I was also having the problem with the empty images displaying when items don't have images associated with them. In my case, I never uploaded an image, so it wasn't an issue of the database records not being deleted when they should have. It was an issue of the records being created when they shouldn't have. It's hard to tell exactly what's happening but it doesn't show up when the node is first posted, only after an edit of the node.

I tried pasting the above code into my template.php file, but it's not complete, and being a little new it took me a few moments to figure out what to do with it.

I thought this function looked odd since it's setting output to ''. Is that what you intended. Shouldn't it have been a duplicate of the original theme function but embraced within the conditional statement for path? That's what I tried. Since I also have thickbox installed and have the thumbnails and inline images link to the preview version of the image, here's the code I added to template.php

<?

function THEMENAME_imagecache($namespace, $path, $alt = '', $title = '', $attributes = NULL) {
if ($path) {
$attributes = drupal_attributes($attributes);
$imagecache_path = file_create_url(file_directory_path() .'/imagecache/'. $namespace .'/'. $path);
// added a namespace class to the image.
$output = '';
}
else {
$output = NULL;
}
return $output;
}

function THEMENAME_imagefield_image_imagecache_thickbox($namespace, $field, $path, $alt = '', $title = '', $attributes = NULL) {
if ($path) {
$attributes = drupal_attributes($attributes);
$imagecache_path = file_create_url(file_directory_path() .'/imagecache/'. $namespace .'/'. $path);
$path_to_preview = file_directory_path() .'/imagecache/preview/'. $path;

// added a namespace class to the image.
// force images to link to preview namespace for the image.
$output = '';
}
else {
$output = NULL;
}
return $output;
}

?>

Just replace THEMENAME with the name of your theme. And if you want the thickbox preview to work check the path to preview variable to make sure it references one of your established imagecache namespaces.

@Zach : unfortunately proposed solution does not work for me. module keeps generating output even though an image does not exist. has anybody else tried without success, or is it just me?

Committed to DRUPAL-5--2

@all, I'm committed jpetso's patch to resolve the dangling db entries. If you are still having issues with empty image tags, please open a new issue.

Hi,

I see the patch was posted againt 5-2, but I don't believe that the original issue was against 5.2, unless of of course I'm reading the whole 5.x-1.x-dev thing wrong... and it still seems to exhibit itself in both the official and development releases on the project page. By the way, the development snap shot has a date of June 18, but the only thing updated was the imagefield.info file.

Is there any way to get this applied against 5.1, which I imagine are the majority of the Drupal 5 installations?

thanks,
Patrick

Oops, didn't mean to change the status, setting back to fixed, just meant to ask my question above.

same here

How is this issue fixed? I'm totally having this error on the latest dev snapshot (June 18th).

All right,

in the drupal spirit of "if no one is doing what you need, do it yourself", I've taken jpetso's patch for 5.2 and modified to work against the latest development snapshot of imagefield.module for 5.1 (version 1.30.2.17 2007/04/21 I believe).

The patch is attached and seems to fix that stated problems on my drupal installation. It needs review and someone else to test it, there were a few minor things going on in jpetso's patch that I didn't include, largely compacting of deltas... don't know if was necessary to include that or not, since I'm clueless as to what it means.

Any ways, try it out.

Patrick

By the way, after applying the patch, I had to resubmit old nodes to get the database entries to update appropriately (i.e. I clicked "edit" for the node and then "submit"). Otherwise you probably won't see any change.

@pcdonohue :: many thanks :: much appreciated :: shall test and revert asap.

I haven't reviewed the code, but after applying the patch and editing the affected posts, I don't get the missing image problem anymore, and preliminarily I don't have any side-effects. Thank you!

Thanks pcdonohue and aigeanta for the backport and testing. I've applied to the 1.x version of imagefield also, so the fix will be available in both the 1.x and 2.x versions of the module.

To clear up any confusion about version numbers, imagefield 1.x and 2.x correspond in no way to Drupal 5.1 or Drupal 5.2. The 1.x and 2.x versions of imagefield will work on any version of Drupal 5, regardless of point number. Please don't abbreviate the Drupal 5, 2.x version of imagefield as imagefield 5.2, as it's just darn confusing :)

I also created a development release of imagefield 2.x, which users can download from the project page if interested. It'll be available later tonight when the packaging script runs. Thanks to everyone who helped on this issue!

has this been resolved for drupal 6 as well? I'm having the same issue but with 6 :[