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

Improve the method for user deletion #408

Closed
wants to merge 1 commit into from
Closed

Improve the method for user deletion #408

wants to merge 1 commit into from

Conversation

Sama34
Copy link
Contributor

@Sama34 Sama34 commented Apr 19, 2014

Please be clear with any issues.

Improves the method for use deletion.
@Sama34
Copy link
Contributor Author

Sama34 commented Apr 19, 2014

GitHub for Windows shows me a CONFLICT with file ./inc/modules/user/users.php regarding some buddy code in that file...

@Sama34 Sama34 added this to the 1.8 Beta 1 milestone May 19, 2014
@Sama34 Sama34 self-assigned this May 19, 2014
@DiogoParrinha
Copy link
Contributor

What you mean by buddy code?

@Sama34
Copy link
Contributor Author

Sama34 commented May 25, 2014

I'm not sure @PirataNervo, that is the error message GitHub for Windows shows me. We will need to manually merge this (which I don't know how-to) or create a new PR for this.

@DiogoParrinha
Copy link
Contributor

You should probably just fix the users.php file then. Pick a fresh and do the edits manually there again - it should be fixed after that. (you'll likely need to create a new PR)

@Starpaul20
Copy link
Member

Is there any reason the datahandler has a multiple uid input?

@Sama34
Copy link
Contributor Author

Sama34 commented May 25, 2014

Some parts delete multiple users at the same run. Should it only accept one UID and be run for every user being deleted? That could be resource intensive.

I will have a look at the conflict.

@Starpaul20
Copy link
Member

I think it might be easier, especially for plugin authors. The original function also only ran one-per-user.

@Sama34
Copy link
Contributor Author

Sama34 commented May 25, 2014

The origianl function for deleting one use did, however there some places within the 1.6 code that deletes multiple users at once (prune users, for example).

I can see your way being more friendly to the data-handler and I agree with you, but I did it this way to avoid possible extensive use of resources or similar.

@PirataNervo, @stefan-st any comments?

@DiogoParrinha
Copy link
Contributor

Check if the input is an array. If it's not an array, assume it's one uid and put it into an array. If it's an array, then you assume you have multiple uid's.

@Sama34
Copy link
Contributor Author

Sama34 commented May 25, 2014

I think that is what it already does:

$this->delete_uids = array_map('intval', (array)$delete_uids);

The following are valid uses of the new method:

$data_handler->delete_user( integer $uid);
$data_handler->delete_user( array $uids);
$data_handler->delete_user( integer $uid, boolean $prune_content);
$data_handler->delete_user( array $uids, boolean $prune_content);

@DiogoParrinha
Copy link
Contributor

(I had not checked to be honest, just provided my input based on the comments)
The current implementation seems more than good for me!

@Sama34
Copy link
Contributor Author

Sama34 commented May 26, 2014

I assumed you didn't checked, didn't intend to be rude! 👅

@DiogoParrinha
Copy link
Contributor

Just fix the conflict problem and we're good to go :P

@DiogoParrinha
Copy link
Contributor

@Sama34

JordanMussi added a commit that referenced this pull request May 30, 2014
Improve the method for user deletion
@JordanMussi
Copy link
Contributor

Fixed conflict and merged (since @Sama34 had deleted their branch). 13c4d44

@Sama34
Copy link
Contributor Author

Sama34 commented May 30, 2014

Thanks Jordan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x s:resolved Status: Resolved. Solution implemented or scheduled t:enhancement Type: Enhancement. Contains minor improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants