Filefield reporting all files removed and added

I did a devel backtrace, and found that the "old" versions of the files, as seen below, have an additional "data" parameter that the new ones don't, that leads driven to think that they've changed.

a:1:{
  s:20:"cck:field_test_files";a:3:{
    s:5:"label";s:7:"File(s)";
    s:3:"old";a:2:{
      i:0;a:10:{
        s:3:"fid";s:2:"17";
        s:4:"list";s:1:"1";
        s:4:"data";a:2:{
          i:0;b:0;
          i:1;b:0;}
        s:3:"uid";s:1:"1";
        s:8:"filename";s:11:"LICENSE.txt";
        s:8:"filepath";s:43:"sites/[sitename]/files/LICENSE.txt";
        s:8:"filemime";s:10:"text/plain";
        s:8:"filesize";s:5:"14940";
        s:6:"status";s:1:"1";
        s:9:"timestamp";s:10:"1280246862";}
      i:1;a:12:{
        s:3:"fid";s:2:"18";
        s:4:"list";i:1;
        s:4:"data";b:0;
        s:8:"filename";s:11:"INSTALL.txt";
        s:8:"filepath";s:45:"sites/[sitename]/files/INSTALL_0.txt";
        s:8:"filemime";s:10:"text/plain";
        s:11:"destination";s:45:"sites/[sitename]/files/INSTALL_0.txt";
        s:8:"filesize";i:5791;
        s:3:"uid";s:1:"5";
        s:6:"status";i:1;
        s:9:"timestamp";i:1280246926;
        s:11:"description";s:11:"INSTALL.txt";}}
    s:3:"new";a:2:{
      i:0;a:9:{
        s:3:"fid";s:2:"17";
        s:4:"list";s:1:"1";
        s:3:"uid";s:1:"1";
        s:8:"filename";s:11:"LICENSE.txt";
        s:8:"filepath";s:43:"sites/[sitename]/files/LICENSE.txt";
        s:8:"filemime";s:10:"text/plain";
        s:8:"filesize";s:5:"14940";
        s:6:"status";s:1:"1";
        s:9:"timestamp";s:10:"1280246862";}
      i:1;a:9:{
        s:3:"fid";s:2:"18";
        s:4:"list";s:1:"1";
        s:3:"uid";s:1:"5";
        s:8:"filename";s:11:"INSTALL.txt";
        s:8:"filepath";s:45:"sites/[sitename]/files/INSTALL_0.txt";
        s:8:"filemime";s:10:"text/plain";
        s:8:"filesize";s:4:"5791";
        s:6:"status";s:1:"1";
        s:9:"timestamp";s:10:"1280246926";}}}}

@#2 oh my!
that's what comment_driven_inspect is intended for
please, use it

perhaps those (data, destination, description, etc) should be ignored when comparing
but I didn't got enough much feedback at the time #727546-4: Supporting FileField fields
(now I don't have much time)

BTW, while you are at it, let me warn you about pending #851118: compatibility with filefield_paths combined with imagefield
which is also related, since FileField's extensions might bring more properties in

Subscribing, this adds Image Fields to every comment whether or not a change was made. Pretty much makes it incompatible with FileField. Happy to help test patches whenever you get around to it.

#5 you can start enabling comment_driven_inspect and posting back its dump
also state whether you're using other filefield-related modules

Along with filefield and imagefield I was using imagefield tokens, filefield sources and filefield paths but I turned those three off and the problem persisted.

Here you go, I hope these work for you:

I noticed there was some extra stuff in the diff_values because of the FileField Sources module, so here's the dump of the one I tested with that module turned off:

cck:field_image (-) array ( 0 => array ( 'fid' => '3', 'list' => '1', 'data' => false, 'uid' => '1', 'filename' => '92581.jpg', 'filepath' => 'sites/default/files/story/2010/July/Wednesday/92581.jpg', 'filemime' => 'image/jpeg', 'filesize' => '1119830', 'status' => '1', 'timestamp' => '1279761313', 'origname' => '92581.jpg', ), )

cck:field_image (+) array ( 0 => array ( 'fid' => '3', 'list' => '1', 'uid' => '1', 'filename' => '92581.jpg', 'filepath' => 'sites/default/files/story/2010/July/Wednesday/92581.jpg', 'filemime' => 'image/jpeg', 'filesize' => '1119830', 'status' => '1', 'timestamp' => '1279761313', 'origname' => '92581.jpg', ), )

it's ok to ignore data when comparing
actually, it is between the ones I thought should be discarded from start

it might not be the only one
(in addition other filefield-related modules might add more irrelevant keys)

#7 second screenshot shows filefield_remote key
do you know which module introduces it? (if so, please open a separated issue for it)

BTW, in the 1st screenshot: is the tooltip's image broken due to non-clean URLs?

#2 where is destination key coming from?

Patch works... but does not work with Filefield Sources enabled (that's where the filefield_remote was coming from).

I'd hate to have to get rid of that, it lets you upload an image from a URL which is really handy. Is it possible to tell Driven to just ignore "'filefield_remote' => array ( 'url' => '', 'transfer' => 'Transfer', ),"?

I have no idea why the tooltip's image was broken, I have clean URLs enabled.

Is it possible to tell Driven to just ignore filefield_remote?

yes, of course,
but I shouldn't mix up different issues
I need to identify where each property comes from, and if it is indeed dispensable

we are dealing here with vanilla filefield
there are also filefield-related modules that add more properties or interfere with data handling (for instance #851118: compatibility with filefield_paths combined with imagefield)
it would be better if each of them get their separated issue

Just wanted to say thanks for the fast patches.

@#12 #872726: Compatibility with FileField Sources

it'll be 2010-08-03_872726_support_filefield_sources_also_865388[unix].patch instead (which already have both, this and filefield_sources #872726: Compatibility with FileField Sources)

PS: right now I have no CVS (temporary firewalled)
could you (O'Brien) or Dane commit that one for me?

right (I thought you were set already, but that was for cdriven)
now both of you have granted access for Driven API as well, go ahead

Tagging to keep track of issues related to the mailhandler-2.x/notifications-4.x workflow

Committed patch from #18

it seems you missed first chunck
comparing the patch at #18 against your commit

unless my CVS tagging was incorrect (which is a possibility).

there is no need to tag anything (dev is the HEAD)

@#23 ops, my mistake, the two chunks of the patch are on the same spot,
(now I saw it when diff-ing)
everything seems ok

thanks obrienmd

(this is probably unclear, but driven is basically showing that field changing from no files to no files).

without the dumped data (from comment_driven_inspect) it is impossible to take a guess

PS: nevertheless, don't assume false hopes, currently I'm not working on this module, just driving its issue queue,
I'm convinced that this module needs to be rewritten already, preferably with more than a single developer to avoid falling into core mistakes that brought the undesirable misbehaviors

I'm able to reproduce this following #28, I'll investigate.

The immediate cause is how a null field value is handled. My filefield is named field_xyz. When the existing version of the node is loaded by comment_driven_nodeapi_nodeapi (so that it can be compared to the updated version), this field appears as $node->field_xyz[0] = NULL. But in the node that's actually getting passed (the updated version), the field appears as

$node->field_xyz[0] = array(
fid => NULL,
list => NULL,
data => NULL,)

I'm not sure if this discrepancy is due to this patch or something else. I've got meetings to go to right now so I won't be able to pursue this further until later, maybe tomorrow.

That's clearly the source of the problem, but I'm not really sure what to do about it. What I do know:

This problem is unique from the OP, as it deals with hook_nodeapi rather than the traditional FAPI handling (so perhaps a new issue is warranted).

It doesn't occur for other types of fields (i.e. if you create a text field, make it driven, then follow steps in #28, the text field doesn't show up as being changed).

I suspect the problem is with the retrieval of the existing version of the node - I say this because with text fields (which work fine), null values are represented by $node->field_abc[0] = array(value => NULL,)- in other words, the attributes of the field are null, but the field itself is not null. With Filefields, the field itself is null ($node->field_xyz[0] = NULL), which implies that Filefield is breaking from CCK standards.

Thus, a simple fix might (haven't tried this yet) be to alter filefield_field_load from

if (empty($items)) {
    return array();
  }

to

if (empty($items)) {
    return array(fid => NULL,
list => NULL,
data => NULL,);
  }

However, I'm a little confused, because I don't think there should BE a 'load' operation for hook_field, according to the documentation in content.module:

/**
 * Implementation of hook_field(). Handles common field housekeeping.
 *
 * This implementation is special, as content.module does not define any field
 * types. Instead, this function gets called after the type-specific hook, and
 * takes care of default stuff common to all field types.
 *
 * Db-storage ops ('load', 'insert', 'update', 'delete', 'delete revisions')
 * are not executed field by field, and are thus handled separately in
 * content_storage.

Sorry for the rambling post- in a nutshell, I suspect the problem here is with FileField, not Comment Driven.

I'm really stumped... while what I said in 34 is true, if you look at the diffs produced by driven_inspect_diff_nodes based on the old and new filefields, you see that the old and new filefields differ in a slightly different way: the old looks like array(fid => 0, list => 1, data => 1,) while the new looks like array(fid => NULL, list => NULL, data => NULL,). The former looks like the format produced by filefield_widget(); I don't know where the latter comes from.

I'm sorry, but I'm just not sure I'm the best person to deal with this- Driven and CCK are both massively complex modules that I clearly don't completely understand.

Arhak, do you have any idea what's going on here based on this evidence?

With Filefields, the field itself is null
($node->field_xyz[0] = NULL), which implies that Filefield is breaking
from CCK standards.

from a quick peek to filefield_field.inc

/**
 * Implementation of CCK's hook_field($op = 'load').
 */
function filefield_field_load($node, $field, &$items, $teaser, $page) {
  if (empty($items)) {
    return array();
  }
  foreach ($items as $delta => $item) {
    // Despite hook_content_is_empty(), CCK still doesn't filter out
    // empty items from $op = 'load', so we need to do that ourselves.
    if (empty($item['fid']) || !($file = field_file_load($item['fid']))) {
      $items[$delta] = NULL;
    }
    else {
  ...

do you have any idea what's going on here based on this evidence?

according to patch @#10
my note regarding this was

// fieldfield module has some inconsistencies regarding wether data is FALSE, an empty array or completely missing

I'm sorry, but I'm just not sure I'm the best person to deal with this-
Driven and CCK are both massively complex modules that I clearly don't
completely understand.

I don't seem to grasp the whole internals of CCK contribs either,
I'm missing a coherent... guidance

right now I have no time/intention to dig into it

nevertheless, I trust cdriven is a valid proposal from its approach being similar to what triggered #942310: Field form cannot be attached more than once,
just needs some love from the right gurus (like those involved in #939836: combofield / fieldentity / field-collection / embeddables),
but they seem to be enough busy #977898: Filefield upload widget broken in combofield

I've found a "fix". Change line ~136 of filefield_field.inc to $items[$delta] = array('fid' => NULL, 'list' => NULL, 'data' => NULL,);. I filed an issue in the Filefield queue: #1132622: Inconsistent handling of null (empty) filefields

@#37 did you tried unset($items[$delta]) instead?

Good guess arhak, unset($items[$delta]) does fix the error as well. What's the significance of that in your mind? Is that a better solution?

What's the significance of that in your mind?

unset is closer to nullifying an array's key

Is that a better solution?

according to CCK, is_empty should work with your proposal, but that could unleash a cascade of other misbehaviours (would there be another potential bugs around it?, what about third party modules already relying on it?)
offering a closer option to what is currently settled will give you a better shot to get the fix in,
and unset is guaranteed not to break, because of AJAX add_more implementation (which considers the disappearance of deltas, if I recall correctly)

note that, I think your proposal "should" be valid, but it could break something else (depending on whether the remaining code honors CCK specs) consider mostly third party modules

@#40 PS: note in driven_cck.diff.inc, comment at line 216, followed by a workaround for the same misbehavior on other CCK modules

function content_set_empty won't complete the missing columns with NULL it just handles the missing whole items

perhaps, cdriven could handle this filefield inconsistency with a similar workaround
(which BTW, I haven't checked why it isn't being handled right at that spot, that would require some debugging)

@#41 perhaps it isn't being handled there (in the workaround) due to filefield assigning a direct NULL instead of using content_set_empty?

I agree that unset seems like a cleaner solution then. All I know beyond that is that Filefield seems to behave differently from other CCK modules, and they should all really get their ducks in a row when it comes to handling nulls. Realistically, though, no-one may ever respond to the Filefield issue, in which case we'll need to implement a workaround here.

I went with a slightly different solution, simply calling content_set_empty on the items array, as you can see in the corresponding Filefield issue: #1132622: Inconsistent handling of null (empty) filefields. Please try that out and let me know if it works. I've also included the patch in the makefile at the howto page: Setting up an email-driven workflow (you'll need to wait a few minutes for alpha6 of the mailflow stack to roll)

It's been a while since I've looked at this, but it seems like there's really nothing to fix in Driven- this is simply waiting on #1132622: Inconsistent handling of null (empty) filefields

Hi guys,

Using this (still :) and have just applied patch at #10, it works great and I'm wondering if I can rebuild diff_values in the db or have some other workaround so past comments don't show the same file being removed and added.

Apologies for commenting on an old issue, just wondering if this might be a real easy answer from someone.
Really appreciated if anyone has any suggestions.

Thanks

@#46

Apologies for commenting on an old issue

Oh my! really?! After all these years?!

have just applied patch at #10

Wasn't it committed already? (see obrienmd @#22)

if I can rebuild diff_values in the db or have some other workaround

Phew! Who knows if there would be an easy way to rebuild it. I suspect there might be a way, but who would dig into it?
I rather recommend finding a workaround.

Wasn't there an option named live_render or something like it?
Have you tried it already?

appreciated if anyone has any suggestions.

Rebuilding past diff_values is an interesting but unrelated issue.
I think this discussion deserves its own ticket, from where you can link to this issue as precedent or trigger. IMO...