Skip to content

Add caching + defer the service provider #13

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

Merged
merged 6 commits into from
Apr 26, 2017

Conversation

jkudish
Copy link
Contributor

@jkudish jkudish commented Apr 25, 2017

This builds upon #12 to further improve the performance of this service provider:

  • Moves the reading of the file to inside the validator's definition. That way the file isn't read if the validator isn't used during a request
  • Defer's the service provider's loading until it's needed
  • Adds caching using rememberForever and an md5 hash of the file as the key, so that the file doesn't need to be re-read and re-computed every time.

Let me know what you think 👍

jkudish added 2 commits April 25, 2017 16:15
cache the password list for future re-use so it doesn’t need to be re-computed every time
Validator::extend('dumbpwd', function ($attribute, $value, $parameters, $validator) {
$path = realpath(__DIR__ . '/../resources/config/passwordlist.txt');
$cache_key = md5_file($path);
$data = Cache::rememberForever('dumbpwd_list_' . $cache_key, function () use ($path) {
Copy link
Owner

@unicodeveloper unicodeveloper Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the Cache Facade being imported? or would you rather use the cache helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. i had added it in the project I tested this in, but not the fresh clone of the repo. fix incoming!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching it

@unicodeveloper
Copy link
Owner

@jkudish Thanks for the PR. This is beautiful. Merging right away!!

@unicodeveloper unicodeveloper merged commit 8476a06 into unicodeveloper:master Apr 26, 2017
@jkudish
Copy link
Contributor Author

jkudish commented Apr 26, 2017 via email

@unicodeveloper
Copy link
Owner

@jkudish What's your twitter handle? I want to praise you there 😄

@jkudish
Copy link
Contributor Author

jkudish commented Apr 26, 2017

I think you found it! 👍

Thanks for the speedy release

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.

2 participants