-
Notifications
You must be signed in to change notification settings - Fork 18
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
expire login links #1 #37
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! Few things to polish up and then it should be fine.
* | ||
* @return array | ||
*/ | ||
function one_time_login_generate_tokens( $user, $count, $delay_delete ) { | ||
function one_time_login_generate_tokens( $user, $count, $delay_delete, $expiry ) { |
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.
function one_time_login_generate_tokens( $user, $count, $delay_delete, $expiry ) { | |
function one_time_login_generate_tokens( $user, $count, $delay_delete, $expiry = 0 ) { |
Should we default to 0
so we don't break existing calls to this function?
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.
as $delay_delete, default value is set on calling function. I can change this but I think this way keeps the current standard.
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 think it'd be best to make this change.
* Bug fix: Pass `$assoc_args` into the command to ensure the `--porcelain` flag actually works. | ||
|
||
= 0.1.0 (April 28th, 2016) = | ||
* Initial release. |
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 wouldn't expect this entire file to change... can you make sure only your changes are applied?
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'm not sure why github marked all file as changed. On my vscode nor online diff tool this is not happening. In any case I will do a new commit.
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.
Seems that in
https://github.com/danielbachhuber/one-time-login/blob/master/.editorconfig
txt files are set to crlf but readme.txt and readme.md are using lf. What do you want to be done? If keep lf, seems good to change .editorconfig to associate end_of_line accordingly.
Same for trim_trailing_whitespace: some whitespaces was introduces on last commit before me. Let me know what do you want to be done.
About web api I will digg a bit more to have a better documentation.
:)
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.
Hm, I'm not sure but this will need to be sorted in any case.
one-time-login.php
Outdated
foreach ( $login_urls as $login_url ) { | ||
WP_CLI::log( $login_url ); | ||
} | ||
} | ||
|
||
if ( class_exists( 'WP_CLI' ) ) { | ||
WP_CLI::add_command( 'user one-time-login', 'one_time_login_wp_cli_command' ); | ||
$args = ['synopsis' => '<user> [--count=<count>] [--delay-delete] [--expiry=<minutes>]']; |
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.
A better approach would be to update the PHPdoc to include the synopsis. Can you make that change? Here's an overview to the standard: https://make.wordpress.org/cli/handbook/guides/commands-cookbook/#annotating-with-phpdoc
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.
Sure. Digging a bit more found that PHPdocblock need a description on each param do be parsed correctly on php-cli, like this:
- [--expiry=]
- : Delete existing token after $expiry minutes from creation, even if not used (default: 0 - not expiry)
I will do it.
Close looking on rest part, the readme.md has correct syntax to make curl calls. But lacks need of a nonce header, or authorization plugin, or from WP 5.6 the use of application password. |
fix #1
Sorry for delay. Here is the PR for expiring links.