Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
I could be wrong but it looks like file is not deleted after the corresponding file entity was deleted.
How to reproduce
1) Create a node with file attached via File Entity.
2) Delete the node.
Expected result
The node is deleted.
The file entity is deleted.
The file itself is deleted.
Real result
The node is deleted.
The file entity is deleted.
The file itself is NOT deleted.
Solution
I guess we should implement hook_entity_delete in order to achieve it, something like this:
/**
* Implements hook_entity_delete().
*/
function file_entity_entity_delete($entity, $type) {
if ($type != 'file') {
return;
}
file_usage_delete($entity, 'file');
file_delete($entity);
}
Comment | File | Size | Author |
---|---|---|---|
#13 | 2629120-file_entity-implement_hook_entity_delete-1.patch | 865 bytes | PatchRanger |
|
Comments
Comment #2
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedComment #4
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedIt 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?
Comment #5
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedMarking as major as it leads to needless costs when using together with AWS.
Comment #6
Dave ReidI 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. The only reason I would think of this happening is if file_valid_uri() is returning FALSE. Is this happening only for specific files like ones using the Amazon S3 stream wrapper?
Comment #7
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedHello Dave, thank you for getting in!
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!
Comment #8
justafish@PatchRanger that patch and the instructions on the module's d.o page only apply to the 2-x branch. Check the README for the 1-x branch for any relevant instructions, but I suggest you upgrade to the new version if possible.
Comment #9
PatchRanger CreditAttribution: PatchRanger as a volunteer commented@justafish Thank you for assistance! I am going to upgrade to 7.x-2.x.
Comment #10
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedIt 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
@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 triggerfile_delete()
? Do you see the point of the patch? Does it make sense? Why do you think it fails?Comment #11
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedAgain, marking as major as it leads to needless costs when using together with AWS.
Correct me if I am wrong.
Comment #13
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedComment #14
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedComment #15
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedComment #16
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedComment #17
PatchRanger CreditAttribution: PatchRanger as a volunteer commented@Dave Reid , @justafish Looks like I've found the reason.
file_delete()
invokes bothfile_delete
&entity_delete
hooks beforefile_unmanaged_delete()
. It seems it is intended to reduce the counter here by calling tofile_usage_delete
. But - hey! - here is how File module implementshook_file_delete
: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
?Comment #18
PatchRanger CreditAttribution: PatchRanger as a volunteer commentedI'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.
Comment #19
joseph.olstadNice patch, I'll review it more thoroughly soon.