search

aaemnnosttv / wp-cli-login-command

Log in to WordPress with secure passwordless magic links.
https://aaemnnost.tv/wp-cli-commands/login/
MIT License
294 stars 47 forks source link

Feature link timeout #12

Closed niladam closed 7 years ago

niladam commented 7 years ago

Added possibility to change the link timeout duration using environment variable. Defaults to 15 minutes as initially intended.

Setting the WP_CLI_LOGIN_TIMEOUT_DURATION as an environment variable will change the duration of the link to that specified duration. Fallbacks to the initial 15 minutes for the timeout.

NOTE: This PR is a duplicate of #11 because i didn't actually read the contributing docs :)

aaemnnosttv commented 7 years ago

Thanks for putting this together! Not a bad start, but there are a few things I'd like to see before I can consider it for a merge.

  1. Test coverage - This shouldn't be that difficult since we can use a very short timeout for the tests.
  2. Command option - I like the idea of using environment variables for configuration, however in this case I think it should only be used to change the default.
    Ideally I see this available in the create command as an option the user can pass, like --expires=15.

Apart from that, theres a few things I would change like the case of the get_env_timeout method (PSR-2 please).

I realize using a command flag for the value would make it a bit more complicated than a global env var like you are here, but I think that makes the most sense as far as implementation goes.

Those are my initial thoughts, let me know what you think.

niladam commented 7 years ago

Well, i've never done test coverage before, and i'm afraid to tackle this right now. I just thought i could give a helping hand to a project i think will save me some time in the future. As for the command option, TBH -- i don't think that would be all that complicated to add and i can clearly understand your opinion about it. However, for 99% of the people using this, the 15 minutes timeout is good enough. I'm a part of the rest. Hence the improvement.

Also, what do you mean by get_env_timeout ?

Thanks.

aaemnnosttv commented 7 years ago

I hear you regarding the testing part. It can be intimidating if you've never done it before.

Regarding the get_env_timeout method I was making a reference to the method naming which is not compatible with the coding standard of this project, (PSR-2) which requires camelCase method names. There's also some stuff in the docblock which looks like it was copied from another method ;)

Please see the instructions for creating a pull request here

Unfortunately, I can't merge changes into master that aren't ready for release because wp package defaults to installing via dev-master.

Your effort is greatly appreciated though, so if you'd prefer to pass on finishing up the PR, I will be sure to give you a mention for your contribution on the next release with this feature 👍

aaemnnosttv commented 7 years ago

This feature is now available in 1.2.0 which was just released. Please see the readme for details.

Thanks again for your contribution.