Fix MaxMind database fetching #164
Conversation
|
This is likely to still fail because there is no licence key on the testing server - so it's a matter of getting a licence key for testing, stubbing out the download or removing the test completely. |
There was a problem hiding this comment.
@dwightwatson this looks great, thanks for cleaning this up! Just searching for the extension makes sense as far as I can see. I guess there's no guarantees either way around their naming conventions so this is probably less likely to break. We could potentially check to make sure there aren't more than one .mmdb files in the tar but otherwise I don't see any issues with it.
I'm not super familiar with this laravel-geoip project though so will let a maintainer take a look as well.
| if (pathinfo($file, PATHINFO_EXTENSION) === 'mmdb') { | ||
| return $file; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could potentially explicitly return null at the end here instead of an implicit void in case the file wasn't matched but up to you. I just usually like having a function either always return something or always not return something for readability, but that may also just be me so feel free to ignore.
There was a problem hiding this comment.
Not opposed to it, it's nice that it's more explicit. I've made the change.
There was a problem hiding this comment.
Better yet - I've moved the exception into the method. Means I don't have to do the is_null check further up.
|
@Torann are you able to review this PR? |
|
Going over it today |
|
This looks great!!! I'm thinking since people have to update their config file, which is out of our hands, I'm going to mark this a version 1.1 which will then require people to go over the upgrade doc. @dwightwatson if you wouldn't mind could you update upgrade.md on Lyften.com with the best way for people to go about upgrading from v1.0 to v1.1 How does this sound? |
|
Makes sense, no worries - just made a quick PR that goes over the change: Torann/lyften.com#17 |
|
Ok, making the release version now and the docs are being deployed now. Thanks! |
This is a follow-up to #163 that is intended to fix #162.
It expands on the work by @tylermann - with a couple of differences.
Instead of adding the database filename to the config it searches for the first file with the extension
.mmdband assumes this is the correct one. Seems like a little simpler to me though less configurable - open to feedback here.It would be great if we could leverage Laravel's filesystem adapter as it would clean up much of the directory creation/removal code, but that's probably part of a larger discussion.