Improve the method for user deletion #408

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@Sama34
Contributor

Sama34 commented Apr 19, 2014

Please be clear with any issues.

Omar Gonzalez
Delete user improvement
Improves the method for use deletion.
@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 Apr 19, 2014

Contributor

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

Contributor

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 1.8 labels May 19, 2014

@Sama34 Sama34 added this to the 1.8 Beta 1 milestone May 19, 2014

@Sama34 Sama34 self-assigned this May 19, 2014

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 22, 2014

Contributor

What you mean by buddy code?

Contributor

PirataNervo commented May 22, 2014

What you mean by buddy code?

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 25, 2014

Contributor

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.

Contributor

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.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 25, 2014

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)

Contributor

PirataNervo commented May 25, 2014

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)

@PaulBender

This comment has been minimized.

Show comment
Hide comment
@PaulBender

PaulBender May 25, 2014

Member

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

Member

PaulBender commented May 25, 2014

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

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 25, 2014

Contributor

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.

Contributor

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.

@PaulBender

This comment has been minimized.

Show comment
Hide comment
@PaulBender

PaulBender May 25, 2014

Member

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

Member

PaulBender commented May 25, 2014

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

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 25, 2014

Contributor

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?

Contributor

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?

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 25, 2014

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.

Contributor

PirataNervo commented May 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 25, 2014

Contributor

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);
Contributor

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);
@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 26, 2014

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!

Contributor

PirataNervo commented May 26, 2014

(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

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 26, 2014

Contributor

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

Contributor

Sama34 commented May 26, 2014

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

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 27, 2014

Contributor

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

Contributor

PirataNervo commented May 27, 2014

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

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
Contributor

PirataNervo commented May 29, 2014

JordanMussi added a commit that referenced this pull request May 30, 2014

Merge pull request #408 from Sama34
Improve the method for user deletion
@JordanMussi

This comment has been minimized.

Show comment
Hide comment
@JordanMussi

JordanMussi May 30, 2014

Contributor

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

Contributor

JordanMussi commented May 30, 2014

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

@JordanMussi JordanMussi added the fixed label May 30, 2014

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 30, 2014

Contributor

Thanks Jordan!

Contributor

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