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.
Reproduce:
1. Upload a user picture (to a user which doesn't have a picture) on the My account > edit page
2. Remove it again by checking the 'Delete picture' checkbox and submitting the form.
Result:
The image is still in the file_managed and file_usage table.
Comment | File | Size | Author |
---|---|---|---|
#22 | user-file-delete-1378092-23.patch | 2.97 KB | oriol_e9g |
#20 | user-file-delete-1378092-20.patch | 2.93 KB | oriol_e9g |
#19 | user-file-delete-1378092-19.patch | 2.95 KB | oriol_e9g |
#17 | user-file-delete-1378092-17.patch | 3 KB | oriol_e9g |
#17 | user-file-delete-onlytests-1378092-17.patch | 2.32 KB | oriol_e9g |
Comments
Comment #1
wedge CreditAttribution: wedge commentedThis was probably broken by #999004: user_save() relies on $edit instead of $account
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedyep at #1, that patch totally brokenated this.
attached patch puts the deletes back.
Comment #3
wedge CreditAttribution: wedge commentedThanks beejeebus, that fixed it for me. It would be great if this could be backported to D7 as well.
Comment #4
wedge CreditAttribution: wedge commentedAnd here is a D7 version.
Comment #5
sunLet's create a test-only patch to make sure we can reproduce this bug.
When re-rolling the functional patch, make sure to tweak
else if
intoelseif
, thanks ;)Comment #6
wedge CreditAttribution: wedge commentedHere is a test. It's my first ever...
Comment #8
sun@wedge: Thanks! Dude, if that's really your first test ever, great work! :)
Should be just one line saying:
"Tests deletion of user pictures."
I didn't look at the surrounding code in this test case, but I don't think this condition is correct. Just remove it.
Aren't these already configured in setUp()?
Is this required or a premature optimization? If it's required, then the inline comment should be re-phrased.
7 days to next Drupal core point release.
Comment #9
wedge CreditAttribution: wedge commentedThanks for the review and the kind words sun.
First comment fixed.
if ($this->_directory_test) {
This was in all the adjacent tests, that's why I kept it. But the test works without it, so I have removed it now.
Regarding the setUp(). I don't think this is handled in the setUp, but I might be missing something. Can you please elaborate?
Without the clearstatcache() call the second is_file() call returns true even if the file is removed. This probably happens because is_file() is called with the same path to check that the picture was created correctly a couple of lines above this call. Maybe this first check can be removed from the test since it's already performed in the testPictureIsValid() test. What do you think?
Comment #10
wedge CreditAttribution: wedge commentedMaybe there should also be an upgrade method that would remove the orphaned files and clear them from the files tables?
Comment #11
sunDid you forget to attach the revised patch?
re: setUp(): yeah, sorry, I was mistaken.
re: clearstatcache(): Alright, makes sense. Let's phrase the comment more explicitly then; removing the "to be sure" should more or less cut it.
re: upgrade: Let's discuss that when we have an actual fix and ready patch for this bug.
Comment #12
wedge CreditAttribution: wedge commentedNew version of the test
Comment #13
sunComment #15
star-szrEdit: Patch in #4 did resolve the described issue in 7.10 for me.
1. Create new user via admin account.
2. Add a picture to the user's account via admin account or the user account itself.
3. Remove the picture by checking 'Delete picture' checkbox from the admin account or the user account itself.
The user picture is removed from the file system and the file_managed and file_usage tables.
Possibly related to #1398616: User picture not deleted after cancelling user's account
Comment #16
star-szrComment #17
oriol_e9gI've only built the puzzle:
- First patch: Only tests from #12, should fail to prove the bug.
- Second patch: Tests from #12, the fix from #2 and a small coding standard fix from #5, this should pass.
Comment #19
oriol_e9gOps! Patch fixed! The only tests failing patch in #17
Comment #20
oriol_e9gThis is better! Removed t() from test assertions: #1380620: Remove t() from test assertions
Comment #22
oriol_e9gFixing notices. Failing tests to prove the bug in #17.
Comment #23
duellj CreditAttribution: duellj commentedThe patch in #22 works great. Tested creating a user, adding a picture and deleting the picture, both as admin and logged in as that test user. In both cases, the file was removed properly from the database and the filesystem.
Comment #24
webchickExcellent! Thanks for the fix and the test!!
Committed and pushed to 8.x and 7.x. Yay! :)
Comment #25
wedge CreditAttribution: wedge commentedI used this sql to find any orphaned files, maybe it's helpful to somebody:
edit: didn't want to change the tags, not sure why that happened.