New on-page comment form is deleting all hidden files :(

https://drupal.org/comment/8461455#comment-8461455 has about half the files deleted, I didn't upload anything new to that one.

#2186089: Increase the length of Issue tags field had one file in the file table, and after commenting on it, the file list is zero. In my comment, before saving, I checked the comment collapsed field and it was zero.

[edit] Another example: https://drupal.org/node/2028025#comment-8461851

Another example
https://drupal.org/comment/8462063#comment-8462063

This is terrible!

It looks like the files are not actually being deleted per se, but they are being removed from the issue. It seems to be removing all the files that were marked as "hidden"... maybe anyway?

...btw is not having the ability to hide/rearrange files intentionally not included in the new form (the "Files" fieldset allows only new uploads - doesn't list old files)? Separate issue? Someone already filed one?

As far as I can tell, this happens when you update (including comment) on an issue when the previous comment is from someone else.

#2159813: Display parts of the issue edit form instead of the comment form on issue pages tries to simplify the form by removing the files table. I committed http://drupalcode.org/project/project_issue.git/commit/4bf4847 to remove that change for now. Adding the D.o UX tag since we might want this back at some point.

That code does seem a bit hacky, it is removing the values for rendering, and re-adding them for saving. I think a better way would be to either:

• Make this an option in extended_file_field module, and use it as default in project_issue.
• If possible, hide on HTML rendering, instead of form construction.
• (display: none; in the theme could work, but should be a last resort. Rendering things just to be hidden is not ideal.)

Now deployed. Forms rendered before deployment, even if they are saved afterward.

I'll see what can be done to clean up the mess.

Collecting examples in the summary.

Good news: This script looks like it does a good job of repairing these. Admins can legitimately delete files, so I'm assuming all cid >= 8461165 have this bug.

$files = db_query("SELECT fu.fid, n.nid FROM file_usage fu INNER JOIN node n ON n.nid = fu.id AND n.type = 'project_issue' LEFT JOIN field_data_field_issue_files fif ON fif.field_issue_files_fid = fu.fid WHERE fu.type = 'node' AND fif.entity_id IS NULL");
$nids = array();
foreach ($files as $file) {
  $nids[$file->nid][] = $file->fid;
}

foreach ($nids as $nid => $fids) {
  $node = node_load($nid);
  foreach (comment_load_multiple(nodechanges_get_changed_cids($node, array('field_issue_files'))) as $comment) {
    if ($comment->cid >= 8461165) {
      $affected = FALSE;
      foreach ($comment->field_issue_changes[LANGUAGE_NONE] as $n => $change) {
        if ($change['field_name'] === 'field_issue_files' && !empty($change['old_value'])) {
          $old = array();
          $new = array();
          foreach ($change['old_value'] as $v) {
            $old[] = $v['fid'];
          }
          if (!empty($change['new_value'])) {
            foreach ($change['new_value'] as $v) {
              $new[] = $v['fid'];
            }
          }
          $diff = array_diff($old, $new);
          if (!empty($diff)) {
            $affected = TRUE;
            foreach ($change['old_value'] as $v) {
              if (in_array($v['fid'], $diff)) {
                $comment->field_issue_changes[LANGUAGE_NONE][$n]['new_value'][] = $v;
              }
            }
          }
        }
      }
      if ($affected) {
        print $node->nid . ' ' . $comment->cid . "\n";
        foreach (file_load_multiple($fids) as $file) {
          $file->display = 0;
          $file->description = '';
          $node->field_issue_files[LANGUAGE_NONE][] = (array) $file;
        }
        $node->nodechanges_skip = TRUE;
        node_save($node);
        comment_save($comment);
      }
    }
  }
}

I've tested it with #893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken and #1316692: Convert hook_admin_paths() into declarative properties on routes and it looks okay. The revision history will have them missing for a bit if there are comments between the problem and this fix; I don't think it worth rewriting all of it.

Bad news: yes, this is still happening, but not too quickly now. About one an hour.

#1857256-91: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views

Yes, deleting all hidden fields is also what I've interpreted this as when I've seen it yesterday. Didn't see this issue then though, would have said that earlier otherwise :)

Should we maybe turn off the new feature until this is fixed?

Agree with #17 and #18. Here is one that just happened to me #2182063: Remove db_placeholders() function which does not exist in D7 comment #10. That patch file was hidden.

...why am I getting access denied when I try to view the revision diff of this issue by clicking the "View changes" link in #20? Text format permissions?

Do you need more examples? I just had this happen on https://drupal.org/node/2045927

Please collect examples in the issue summary.

I ran the recovery script to restore the hidden files for now.

The node from menu_get_object() used when building the form in a block is missing the hidden files.

Something else on the page is poisoning that copy of the node object. If straightforward to find & fix, that root cause can be fixed. Otherwise, we can load a fresh copy of the node.

http://drupalcode.org/project/project_issue.git/commit/fab3279 should fix this. I got lucky and quickly found that something generating the issue metadata block was abusing the node object. This is now deployed on Drupal.org.

I think this is another symptom of a core bug we've been working around, #1289336: Calling node_view for the same node with multiple view modes on the same page during node preview does not correctly attach fields.

I ran the cleanup script from #14 again, so this should all be cleaned up.

We could probably safely re-instate the code from #7, with some testing, but I would still like the alternative solutions to be investigated.

This hasn't recurred since #27. This issue can be repurposed to get the files UI in line with #2159409: [Meta] Implement first iteration to improve issue queue workflow; or this issue can be closed and a clean followup opened for the files UI.

I think fixing this has somehow caused #2192497: Match domains on more than $_SERVER['HTTP_HOST']