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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wedge’s picture

Anonymous’s picture

Status: Active » Needs review
FileSize
643 bytes

yep at #1, that patch totally brokenated this.

attached patch puts the deletes back.

wedge’s picture

Thanks beejeebus, that fixed it for me. It would be great if this could be backported to D7 as well.

wedge’s picture

And here is a D7 version.

sun’s picture

Issue tags: -user pictures +Needs tests

Let'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 into elseif, thanks ;)

wedge’s picture

Here is a test. It's my first ever...

Status: Needs review » Needs work

The last submitted patch, user-file-delete-1378092-6.patch, failed testing.

sun’s picture

@wedge: Thanks! Dude, if that's really your first test ever, great work! :)

+++ b/core/modules/user/user.test
@@ -1080,6 +1080,55 @@ class UserPictureTestCase extends DrupalWebTestCase {
+   * Do the test:
+   *  Picture is deleted properly
+   *
+   * results: The image should be removed

Should be just one line saying:

"Tests deletion of user pictures."

+++ b/core/modules/user/user.test
@@ -1080,6 +1080,55 @@ class UserPictureTestCase extends DrupalWebTestCase {
+    if ($this->_directory_test) {

I didn't look at the surrounding code in this test case, but I don't think this condition is correct. Just remove it.

+++ b/core/modules/user/user.test
@@ -1080,6 +1080,55 @@ class UserPictureTestCase extends DrupalWebTestCase {
+      $info = image_get_info($image->uri);
+
+      // Set new variables: valid dimensions, valid filesize (0 = no limit).
+      $test_dim = ($info['width'] + 10) . 'x' . ($info['height'] + 10);
+      variable_set('user_picture_dimensions', $test_dim);
+      variable_set('user_picture_file_size', 0);

Aren't these already configured in setUp()?

+++ b/core/modules/user/user.test
@@ -1080,6 +1080,55 @@ class UserPictureTestCase extends DrupalWebTestCase {
+      // Clear out PHP's file stat cache to be sure we see the current value.
+      clearstatcache();

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.

wedge’s picture

Thanks 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?

wedge’s picture

Maybe there should also be an upgrade method that would remove the orphaned files and clear them from the files tables?

sun’s picture

Did 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.

wedge’s picture

New version of the test

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, user-file-delete-1378092-12.patch, failed testing.

star-szr’s picture

Issue tags: -Needs backport to D7

Edit: 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

star-szr’s picture

Issue tags: +Needs backport to D7
oriol_e9g’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
2.32 KB
3 KB

I'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.

Status: Needs review » Needs work

The last submitted patch, user-file-delete-onlytests-1378092-17.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Ops! Patch fixed! The only tests failing patch in #17

oriol_e9g’s picture

This is better! Removed t() from test assertions: #1380620: Remove t() from test assertions

Status: Needs review » Needs work

The last submitted patch, user-file-delete-1378092-20.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

Fixing notices. Failing tests to prove the bug in #17.

duellj’s picture

Status: Needs review » Reviewed & tested by the community

The 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent! Thanks for the fix and the test!!

Committed and pushed to 8.x and 7.x. Yay! :)

wedge’s picture

I used this sql to find any orphaned files, maybe it's helpful to somebody:

select fu.*, fm.uid, u.picture from file_usage fu
inner join file_managed fm on fu.fid = fm.fid
inner join users u on fm.uid = u.uid
where fu.type = 'user' and u.picture != fu.fid

edit: didn't want to change the tags, not sure why that happened.

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