Hello, here's an official feature request for FileField Sources compatibility. That module adds something like "filefield_remote' => array ( 'url' => '', 'transfer' => 'Transfer', )," to the diff dump, if Driven ignored "filefield_remote" it should probably work.

Thanks

Comments

arhak’s picture

Status: Active » Needs review
StatusFileSize
new2.52 KB

this patch contains both: support for filefield_sources and #865388: Filefield reporting all files removed and added
(so you only have to apply this one to 6.x-dev to get them both)

PS: this issue started from #865388-12: Filefield reporting all files removed and added
this is a quick patch that should work, but I haven't reviewed filefield_sources yet

arhak’s picture

Title: Compatiibility with FileField Sources » Compatibility with FileField Sources

typo

jthomasbailey’s picture

Seems to work, thanks a lot

bibo’s picture

I had the same problem with filefield_sources. Hopefully this fix will be in the next dev-release :)

bibo’s picture

Status: Needs review » Reviewed & tested by the community

Setting to RTBC.

vito_a’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new2.93 KB
new1.69 KB
new876 bytes

Needed to fix it some more to show filefield diffs properly though, hope some simple illustrations will help to understand why that was needed:

(Attachments is a filefield which was used to replace comment_upload functionality due to the http://drupal.org/node/739690 and yes, the steps given at the http://drupal.org/node/739690#comment-2803752 are working, though found no time to write a handbook page or FAQ on that issue also, hope someone can do that)

The issue for me was content_field() doesn't always return $old_view and $new_view properly if $node is given to it like

    $node = (object)array(
      'type' => $node_type,
      'build_mode' => NODE_BUILD_NORMAL
    );

and it performs better if there is a node:

  // Trying to load a node if that is possible
  if (arg(0) == 'node' && is_numeric(arg(1))) {
    $node = node_load(arg(1));
  }
  else {
    $node = (object)array(
      'type' => $node_type,
      'build_mode' => NODE_BUILD_NORMAL
    );
  }

Patch attached was done against the latest 1.x-dev version and includes the http://drupal.org/files/issues/2010-08-03_872726_support_filefield_sourc... from #1 so can be applied alone. Big thanks to arhak . Proposed way of loading the full nodes seems to be far from ideal, hope we can discuss that and find a better solution, though it's working for some particular cases, as shown by the second picture.

arhak’s picture

doesn't always return $old_view and $new_view properly

how would "improperly" precisely looks like?

Proposed way of loading the full nodes seems to be far from ideal, hope we can discuss that and find a better solution

I guess it is a matter of figuring out what is actually missing for the dummy $node to work properly
(relying on args is unacceptable, it only solves the vanilla case, consider panels for instance)

though it's working for some particular cases

by "some" you mean "not every"?
do you know a case which still fails? even with your proposed patch

if (is_array($old_view[$field_name])) {

what made you think it wouldn't be an array?
did you hit that line with that index undefined? or any other kind of data?

vito_a’s picture

Thank you for paying much attention to these issues, as some of these seem to be very annoying sometimes, and it's very good to see someone who cares :)

Though it seems that some more investigation of the case is needed.

As for "improperly", it just looks like driven_filefield_notpatched.png for me, $old_view and $new_view are both returned Array() (empty arrays).

Yes, exactly what I was talking about, that seems to be solving that node "view" vanilla case, and there may be much other cases, that is only one $op. Also I'm considering it just a quick solution. However don't we need these diffs shown at the "view" case first of all?

Yes, it will be very good to figure out what is actually missing for the dummy $node to work properly. I hope someone will have some time to find it. Can someone help us with this case please as some time seems to be needed to check it thoroughly?

Unfortunately I can't provide the full case study at the moment thus can't say exactly in which cases diffs would fail and would these fail at all. As for what made me think there wouldn't be arrays in the latter case, the above warnings did:

# warning: Invalid argument supplied for foreach() in /sites/all/modules/driven/props/driven_cck/driven_cck.diff.inc on line 382.
# warning: Invalid argument supplied for foreach() in /sites/all/modules/driven/props/driven_cck/driven_cck.diff.inc on line 394.

Hope that helps :)

arhak’s picture

Status: Needs review » Needs work

@#8 thanks

so the right direction would be knowing better the internals of content_field() for filefields, to provide whatever the dummy $node is missing

arhak’s picture

Status: Needs work » Fixed
arhak’s picture

@vito_a
still pending your cases
could you please file a new issue?

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.