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

ADD caching support using WP transient API as exposed from WP-CLI #7

Merged
merged 2 commits into from
Nov 30, 2016

Conversation

emaV
Copy link

@emaV emaV commented Nov 27, 2016

I guess this is the simplest solution even though adding a caching layer to guzzle client maybe would be a more sound solution as the WP-CLI caching is "site" based.

@markri
Copy link
Owner

markri commented Nov 28, 2016

I like the idea of caching. A developer of wpvulndb.com told me that a limit of 30 calls every 30 seconds is acceptable.

However, to keep things simple I would suggest to disable caching by default, this way we have more control (e.g. during development this is cumbersome when one needs to clear the cache to check for new values when updates are done). So adding an option like:

--caching | protects from lockouts on wpvulndb.com (max 30 calls/30 seconds) by caching results

would be better I think, unless I'm missing something here. Are there any ideas to suggest otherwise?

@emaV
Copy link
Author

emaV commented Nov 28, 2016

Well, no problem in adding a switch. What is indeed cached is the call to wpvulndb.com (for eight hours and I don't know how much often is updated) not the version checking.

@emaV
Copy link
Author

emaV commented Nov 28, 2016

BTW in the pipeline (zipangotes/wp-sec@2b9d090) there is the fix for the behat JSON testing.

@dogsbody
Copy link

Would this caching be able to be shared between users. We already do this with the download cache in wp-cli by setting the WP_CLI_CACHE_DIR to a shared folder. It would be great if we could do the same with this and result in a lot less calls to wpvulndb. Thank you for this and all your hard work with it.

@emaV
Copy link
Author

emaV commented Nov 28, 2016

Uhm, it would be possible by implementing the caching in the guzzle stack.
I don't know exactly how the wp-cli download cache is working. I asked in slack (https://wordpress.slack.com/archives/cli/p1480249573000275) but so far no answers.

@emaV
Copy link
Author

emaV commented Nov 28, 2016

In the wp-cli API there is WP_CLI\Utils\http_request but I don't know if it implements a caching.

But indeed I found this on CoreUpgrader.php an implementation of the cache system related to the WP_CLI_CACHE_DIR setting.

So it could be something like:

$cache = WP_CLI::get_cache();
$cache_key = "wp-sec/core";
$cache_file = $cache->has( $cache_key, $ttl );
if ($cache_file) {
  $json = $cache->read($cache_key);
}
else {
  ...
  $json = json_decode($res->getBody(), true);
  $cache->write($cache_key, $json);
}

Any thoughts @dogsbody ?

@emaV
Copy link
Author

emaV commented Nov 28, 2016

BTW using WP_CLI\Utils\http_request would remove the dependency on Guzzle. What you think @markri?

@markri
Copy link
Owner

markri commented Nov 28, 2016

Agreed to loose the Guzzle dependency and to use the http_request() by WP_CLI. Advantages I see:

  • Less overhead by Guzzle; which is a bit of too much weight for this plugin. I just chose it because of habitual patterns. As it seems; WP_CLI uses rmccue/Requests for the actual request, which is a better match for the purpose it is used for.
  • More simplified install profile, no dependencies :-)
  • Better BC compatibility with PHP 5.3

@emaV
Copy link
Author

emaV commented Nov 28, 2016

This should work: zipangotes/wp-sec@c2c4989

@markri markri merged commit 3f66a80 into markri:master Nov 30, 2016
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

3 participants