Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User Model forceDelete doesn't remove the record from the DB #951

Closed
lcharette opened this issue Mar 30, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@lcharette
Copy link
Member

commented Mar 30, 2019

When using the User model forceDelete option ($user->delete(true)), the record is still soft deleted, not removed from the db. The delete method doesn't seam to return true as supposed to. Possible user facing error too : https://chat.userfrosting.com/channel/support?msg=QbMRJX7uhHHahFLZi

@lcharette lcharette added this to the 4.2.x milestone Mar 30, 2019

@lcharette lcharette self-assigned this Mar 30, 2019

lcharette added a commit that referenced this issue Mar 30, 2019

@claudious96

This comment has been minimized.

Copy link

commented Apr 1, 2019

My setup:

  • Apache 2.4.37
  • MariaDB 10.1.37 (tested also with MySql 5.7.25)
  • PHP 7.3.0 (tested also with 7.2.15)

This error shows up in the modal when trying to force delete a user
immagine

In the browser console doesn't come up anything, the response to the DELETE request is just a status code 500.

Commenting #L192 in the User model the error disappears but it still performs SoftDelete.

I also checked in UF 4.1 and the bug is also there (no error shows up), so it shouldn't be caused by the foreign key added in UF 4.2 .

@lcharette

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2019

This bug is successfully replicated on the hotfix-userIssue branch with a new test. However, apparently this is a bug in Eloquent itself, which was fixed with Laravel version 5.5: laravel/framework#9806 (comment)

We're still on 5.4, which explain why we're still seeing this. There is not much we can do for now I guess. I'll leave this issue open and mark it for UF 4.3 so we can confirm it is indeed fixed when we update our Laravel dependency.

@lcharette lcharette modified the milestones: 4.2.x, 4.3.0 Apr 7, 2019

@claudious96

This comment has been minimized.

Copy link

commented Apr 8, 2019

That issue is not exactly the same bug, it is still a problem but the bug here is being unable to perform hard deletes even without having already soft-deleted the user.

I started digging a little bit more into this problem and I think there are three possible causes, the last 2 are about the error I got:

  • the first one (I'm quite sure about this one) is User.php#L201 (and as a consequence also (User.php#L204) ). Using parent:: the call falls back to the method defined in the class that is extended (Model), to use the method defined in the SoftDeletes trait, should be instead $this->forceDelete(); and $this->delete();. Just for clarity's sake here is a snippet code to clarify this.
  • the second one could be the need to delete any persistences related to the user since each row is referenced to a user_id.
  • the third I think (I'm not sure about this), could be the 'circular foreign keys' between the users table and the activities table. In fact each user's last activity column must be referenced to an activity's id and also each activity's user_id must be referenced to an actual user's id. This means that none of the two (or more) instances can be deleted before the other one. This explains why I could avoid the alert in the modal, commenting #L192.

The first I think could be easily fixed with the edits I wrote above (although there is the possibility of unpredictable behaviour of the code like in the snippet).
The second could be solved just adding a new class mapping in the ServicesProvider and then deleting just like done with the other instances.
About the third, I cannot see a solution that would not involve a new migration. For the future could be also considered to add onDelete('cascade') when defining the foreign keys (see this question in SO ).

I'm not an expert PHP coder so I'm likely missing something. I'd love some kind of feedback, if needed I could also work on a PR.

lcharette added a commit that referenced this issue Apr 8, 2019

@lcharette lcharette modified the milestones: 4.3.0, 4.2.x Apr 8, 2019

@lcharette

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Actually, you're right. I got distracted by another test I did.

Your first point was right on target. $result = parent::forceDelete(); in User.php#L201 should be $result = $this->forceDelete();. No need to use parent:: in that situation, as it should only be used to refer to the same method in the parent class.

That's why changing parent::delete(); into $this->delete(); didn't worked for you, as you where referencing the same method and creating an infinite loop.

Should be fixed in commit 3790e4f, I'll merge into hotfix branch, if you can confirm it work before we release the fix in 4.2.1

@lcharette lcharette referenced this issue Apr 8, 2019

Merged

Fix #951 #962

lcharette added a commit that referenced this issue Apr 9, 2019

@lcharette lcharette closed this Apr 9, 2019

@claudious96

This comment has been minimized.

Copy link

commented Apr 9, 2019

That's why changing parent::delete(); into $this->delete(); didn't worked for you, as you where referencing the same method and creating an infinite loop.

Yes, it makes perfectly sense.
I tested it and now it works only in case the user is created from the admin panel and then deleted (so no activities logged), otherwise the error brakes the whole delete operations.

Since you could not reproduce the error I got in the modal, I guess it depends on the DB server.
Here is my fix for my setup mentioned above, it is backward compatible it disables the foreign key check until the end of the operation and then enables it again.
If it doesn't make it to the hotfix it might be useful for somebody someday.
First I defined the classMapping of the Persistence model inside ServicesProvider.php

$container->extend('classMapper', function ($classMapper, $c) {
           ...
            $classMapper->setClassMapping('persistence', 'UserFrosting\Sprinkle\Account\Database\Models\Persistence');

            return $classMapper;
        });

And this is how my delete method of the User model looks like

public function delete($hardDelete = false)
    {
        /** @var \UserFrosting\Sprinkle\Core\Util\ClassMapper $classMapper */
        $classMapper = static::$ci->classMapper;

        if ($hardDelete) {
            // Remove all role associations
            $this->roles()->detach();

            static::$ci->db->schema()->disableForeignKeyConstraints();
            // Remove all user activities
            $classMapper->staticMethod('activity', 'where', 'user_id', $this->id)->delete();

            // Remove all user tokens
            $classMapper->staticMethod('password_reset', 'where', 'user_id', $this->id)->delete();
            $classMapper->staticMethod('verification', 'where', 'user_id', $this->id)->delete();
            $classMapper->staticMethod('persistence', 'where', 'user_id', $this->id)->delete();

            // Delete the user
            $result = $this->forceDelete();
            static::$ci->db->schema()->enableForeignKeyConstraints();
        } else {
            // Soft delete the user, leaving all associated records alone
            $result = parent::delete();
        }

        return $result;
    }
@lcharette

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Hum, you might be right again. Delete might be missing some cleanup. I wonder if we would need to delete roles or other tables. Delete cascade would be easier indeed.

@claudious96

This comment has been minimized.

Copy link

commented Apr 9, 2019

Well, the cleaning of roles works well with detach() as far as I can see from the tests I tried today.
About cleaning the other tables, I based my thoughts on the migrations that adds the foreign keys. I might have missed something but should be fine like this.

@lcharette

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Hum, can you open a new issue about this, as this one has already been merged?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.