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

Make php artisan api-key:delete actually delete #127

Merged
merged 1 commit into from
May 24, 2016

Conversation

danhunsaker
Copy link
Contributor

The API key passed to the command is found, and its existence is checked, but it is never actually revoked. Since the model uses SoftDeletes, we can use $apiKey->delete() to revoke the key. Note that this means the command doesn't actually support deleting the key unless the model has been replaced in the app config by a model class that doesn't support soft deletes. We could probably add a flag to specify that the command should use ->forceDelete() instead, but I haven't opted to do so at this point, erring instead on the side of changing as little as possible.

This PR adds the ->delete() line so the artisan command will actually revoke the key.

The API key passed to the command is found, and its existence is checked, but it is never actually revoked.  Since the model uses SoftDeletes, we can use `$apiKey->delete()` to revoke the key.  Note that this means the command doesn't actually support *deleting* the key unless the model has been replaced in the app config by a model class that doesn't support soft deletes.  We could probably add a flag to specify that the command should use `->forceDelete()` instead, but I haven't opted to do so at this point, erring instead on the side of changing as little as possible.

This PR adds the `->delete()` line so the artisan command will actually revoke the key.
@chrisbjr chrisbjr merged commit 6773176 into chrisbjr:master May 24, 2016
@chrisbjr
Copy link
Owner

Thanks for this @danhunsaker!

@danhunsaker danhunsaker deleted the bugfix/artisan-delete branch May 24, 2016 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants