Implemented minutely rate limit with pause #355
Conversation
… of latest refresh token.
|
Any thoughts on this one @stephangroen or @DannyvdSluijs ? |
|
Thank you @meijdenmedia. I cannot merge this, because I don't like framework specific code in the library. It adds a lot of code for people that do not use Laravel. I'm also not a fan of putting Also there is no need to up the PHP requirement to 7.1. Older versions might not be supported by PHP anymore, that doesn't mean those aren't used in production anymore. This changes breaks backward compatibility. I might bump to 7.x on a totally new version of this library so we can use alle the fancy new stuff in that new version :) |
| // todo: implement routes and migrations for authentication with Exact.. | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
It misses EOF (end of file).
| $this->acquireAccessToken(); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
It misses EOF (end of file).
| { | ||
| if ($this->pauseForMinuteLimitEnabled()) { | ||
| if($this->minuteLimitExceeded()) { | ||
| sleep(60); |
There was a problem hiding this comment.
These conditions about coding style are very different.
I think this should be fixed and please check other codes in this PR.
I think other codes should have the same problem.
| { | ||
| if ($this->pauseForMinuteLimitEnabled()) { | ||
| if($this->minuteLimitExceeded()) { | ||
| sleep(60); |
There was a problem hiding this comment.
I also think that using sleep function is not a good idea to do pause work.
I think we can present the message to tell developers need to pause request at this time.
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "~4.8.35" | ||
| }, | ||
| "config": { |
There was a problem hiding this comment.
Should we need to define the config block in composer.json?
I think it's up to developers to build their current development environment.
| "php": ">=5.5.0", | ||
| "guzzlehttp/guzzle": "~6.0" | ||
| "ext-json": "*", | ||
| "php": ">=7.1", |
There was a problem hiding this comment.
It looks like this PR is plant to drop php-5.x and php-7.0 supports.
And just notice that we should drop these PHP versions in .travis.yml setting.
Fixing #255 for Laravel users.
Inspired by SoftTouch-bvba@60bd530
The package keeps track of the most recent
refresh_tokenby saving it in the Laravel cache. This way you can spawn multiple processes at the same time without using an old refresh token. Before hitting the minute limit the script will pause if you allow it too (see readme).Changed the README, updated minimum PHP version to 7.1 since all older versions are not supported anymore. For more info, see: https://www.php.net/supported-versions.php
Tomorrow I will deploy this package in our production environment.
Let's wait a couple of days before merging this.