-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: add CLI command to manage users #833
Conversation
0d08c19
to
8ead772
Compare
6bb4aba
to
da466af
Compare
$action = $params[0] ?? null; | ||
|
||
if ($action === null || ! in_array($action, $this->validActions, true)) { | ||
$this->write( |
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.
If not valid, display a form to select valid items.
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 PR is too big. If you want to add more features, you should send another PR.
I am more concerned about the current features working properly than usability.
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.
I am more concerned about the current features working properly than usability.
Ok I get it, I haven't exactly checked out this PR yet. I will try to continue with this in mind.
src/Commands/User.php
Outdated
$this->removegroup($group, $username, $email); | ||
break; | ||
} | ||
} catch (RuntimeException $e) { |
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.
Throw exception! If not valid, display a form to select valid items.
|
||
class User extends BaseCommand | ||
{ | ||
private static ?InputOutput $io = null; |
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.
$io
I think it is short, and it is not understandable.
https://phpmd.org/rules/naming.html#shortvariable
src/Commands/User.php
Outdated
if ($password !== $passwordConfirm) { | ||
$this->write("The passwords don't match", 'red'); | ||
|
||
throw new RuntimeException("The passwords don't match"); |
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.
Not good. Do not remove the user from the process. Display new form to re-enter data.
src/Commands/User.php
Outdated
if ($user === null) { | ||
$this->write("User doesn't exist", 'red'); | ||
|
||
throw new RuntimeException("User doesn't exist"); |
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.
In all cases, I disagree with throw.
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.
The exception will be caught in run()
method.
It is the replacement of exit()
.
@robertogerola @cijagani Try this, if you can. |
CLI::getOption() cannot use in testing now.
Co-authored-by: Pooya Parsa <[email protected]>
The validation rules may be an array or string.
RuntimeException is too wide.
8eef957
to
fade2c1
Compare
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.
I haven't had a chance to review, and I probably won't be able to for a few days.
It seems that people are waiting for this PR. You can merge.
Okay. The test code has been improved, so there should be no major bugs. |
Description
Supersedes #579
Closes #566
Add command
shield:user
to manage users:TODO
exit()
Checklist: