protected_node_file_download will return -1 for temporary files that are not attached to nodes. This causes managed_file uploads to disappear after validation errors or ajax upload.

Is there a reason the module returns -1 for files with status 0 that I don't see?

Ideally, the module would first ascertain whether the file is actually governed by protected_node before deciding on access.

CommentFileSizeAuthor
#5 protected_node-fix_hook_file_download-2661316-5-bis.patch7.13 KBgrimreaper
#5 protected_node-fix_hook_file_download-2661316-5.patch5.55 KBgrimreaper
#2 protected_node-fix_hook_file_download-2661316-2.patch1.82 KBgrimreaper

Comments

Heine created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Active » Needs review
StatusFileSize
new protected_node-fix_hook_file_download-2661316-2.patch1.82 KB

Hello,

Thanks for reporting this side effect. Here is a patch that may solve the problem.

Can you please test it?

Can you also provide the steps to reproduce the error you encountered? So we can add a test to not forget to catch your use situation.

slv_’s picture

slv_ commented
Status: Needs review » Reviewed & tested by the community

Hi @Grimreaper,

The most important bit. Patch from #2 works fine for me, against the most recent version of the module. Thanks!

As you were asking for some info, this was my scenario:

1.- Add an "Image" field type to a content type. Configure it to use the private filesystem.
2.- Upload any image to it. The thumbnail that should appear is not there. (If used alongside insert module (http://drupal.org/project/insert), inserting the image into a wysiwyg textarea, will fail too, and the image won't be displayed on the node edit form.
3.- Saving the node and re-editing it, fixes it, because at that point the file has $file->status = 1, so the logic triggered is different.

I can confirm all the steps above works flawlessly when using the public filesystem, so it's not a problem in setup, as this just appears when switching to the private filesystem.

I can also confirm the issue is caused by protected_node, as I did debug the problem step-by-step, and protected_node was indeed returning -1 from protected_node_file_download() in a scenario where it shouldn't (the content type wasn't even configured to use protected_node).

Marking this as RTBC. Patch works and I think the logic added makes sense.

Thanks a lot!

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello @slv_,

Thanks for the review.

I will make a test to check that.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new protected_node-fix_hook_file_download-2661316-5.patch5.55 KB
new protected_node-fix_hook_file_download-2661316-5-bis.patch7.13 KB

Here comes two patches. One that add the test and should fail, the other with the test and the patch, it should be green.

The last submitted patch, 5: protected_node-fix_hook_file_download-2661316-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: protected_node-fix_hook_file_download-2661316-5-bis.patch, failed testing.

  • Grimreaper committed c1176a4 on 7.x-1.x 
    Issue #2661316 by Grimreaper, Heine, slv_: Protected node...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Fixed

I don't know why the tests fail on drupal.org and not locally.

But the fail and success I expected are present.

So the patch is merged.

Thanks for the review and tests.

Status: Fixed » Closed (fixed)

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