-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Also delete objectstore data on user deletion #15147
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
Conversation
As there is no local data we have to explictly delete the data from the objectstore. Now this might time out if you do it via web. Maybe we should add a warning there if you do not use local storage? Signed-off-by: Roeland Jago Douma <[email protected]>
|
|
||
| // Delete all files | ||
| if (!$userRoot->getStorage()->instanceOfStorage(Local::class)) { | ||
| $userRoot->delete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just do that for all OS and then drop the lines 231 to 236?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should we do this in a background job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that was also my consideration.
So then deleting the user is:
- Disabling the user
- Setting a special flag
- Scheduling backgroundjob
Because we need to fetch the actual user to do the deletion. Else we can't fetch the storage etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is something for master. This can go in like that for the back ports and then we add the background job for master.
|
I'm fine with this for back porting and doing a background job and unified handling of this across all primary storages for master. |
juliusknorr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
|
CI fails on PHPUnit tests |
🏓 |
|
Actually the more I think about it the more it will most likely timeout. If you try to do this from the user management. Let me move this to 18 and see if we can soon come up with a proper backportable way that won't time out and do 💥 |
|
Is this already implemented in nc 17 and 18? As far as i can see, this bug still exists. Take a look at #9690 |
|
For me on NC 20 the issue is still present... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix works for me ....
IIPoliII
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great but only using php-cli without timeout
Fixes #9690
As there is no local data we have to explictly delete the data from the
objectstore.
Now this might time out if you do it via web. Maybe we should add a
warning there if you do not use local storage?
Signed-off-by: Roeland Jago Douma [email protected]