After entity was removed the file should be removed too

It looks weird that those quite unrelated unit-tests get failed by this patch. It seems like the logic of replacing a file relies on some side-effect, which erroneously becomes broken after introducing the changes of the patch - or I am missing something?

Marking as major as it leads to needless costs when using together with AWS.

Hello Dave, thank you for getting in!

Is this happening only for specific files like ones using the Amazon S3 stream wrapper?

We're using AmazonS3 module, which provides custom stream wrapper. Default download method is set to it (option called "Amazon Simple Storage Service") site-wide. So, answering your question, yes, in our case it only relates to that stream wrapper.

I have checked whether File Entity is properly patched as described at AmazonS3 project page - and discovered that one of the required patches is missing: #2479483: Add hook_file_entity_upload_destination_uri_alter() for remote stream wrapper / s3 support. Looks like in our case drush make failed to apply it - and I missed it.
Sorry for bothering - and thank you for assistance!

@justafish Thank you for assistance! I am going to upgrade to 7.x-2.x.

It seems upgrading didn't help, the problem persists.

Problem (described Behat-style :) )

Given File Entity module is installed
And Composer Manager module is installed
And AmazonS3 module is installed
And all required by AmazonS3 patches are successfully applied
And Status Report has no errors
And "Amazon Simple Storage Service" is set as default download method
And I create a node
And I upload a unique (to be sure it is not used anywhere else) file as a file entity of the node
And a file is for sure available for download from AWS
When I delete the node
Then the file is still available for download
(But the file must not be available for download)

Environment

PHP 5.5.9-1ubuntu4.11
Drupal 7.41 (install profile OpenAtrium 7.x-2.50)
File Entity 7.x-2.0-beta2
Composer Manager 7.x-1.8
AmazonS3 7.x-2.1-beta1

I see that both file_delete() (provided by core) and file_delete_multiple() (provided by file entity) attempt to call file_unmanaged_delete() on the files.

@Dave Reid I see your point, it's reasonable. Could it be the case that file_delete() is not being called at all? I mean, if it would be called, then everything is ok, - but what if deleting the entity doesn't trigger file_delete()? Do you see the point of the patch? Does it make sense? Why do you think it fails?

Again, marking as major as it leads to needless costs when using together with AWS.
Correct me if I am wrong.

@Dave Reid , @justafish Looks like I've found the reason. file_delete() invokes both file_delete & entity_delete hooks before file_unmanaged_delete(). It seems it is intended to reduce the counter here by calling to file_usage_delete. But - hey! - here is how File module implements hook_file_delete:

/**
 * Implements hook_file_delete().
 */
function file_file_delete($file) {
  // TODO: Remove references to a file that is in-use.
}

See? It does nothing to unlink existing files. We should add file_usage_delete to some hook implementation. And here is where the question appears: where we should add it? I see 3 options:
- In File module: replace that @todo with file_usage_delete($file, 'file');
- In File Entity: implement hook_file_delete
- In File Entity: implement hook_entity_delete
What do you think? Which hook implementation should call file_usage_delete?

I've finally debugged it. The solution is 2 patches: #13 from this issue & #5 from #2639230: After entity was removed the file should be removed too. See solution description in that issue. I've tried both patches separately - they work only together, so it looks like they are both necessary.