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

Remove faulty mkdir call #178

Closed
wants to merge 1 commit into from
Closed

Conversation

dwightwatson
Copy link
Contributor

This reverts a change that was made in #172

Quoting my comment from that PR:

This update has broken MaxMindDatabase for me.

[2020-03-09 23:23:40] testing.ERROR: mkdir(): File exists {"exception":"[object] (ErrorException(code: 0): mkdir(): File exists at /Users/Dwight/Sites/my-app/vendor/torann/geoip/src/Services/MaxMindDatabase.php:29)
This is with the default configuration. Out of the box the path is storage/app/geoip.mmdb, your call to str_replace turns this into storage/app and then tries to create that directory, which already exists.

This PR should be reverted, and made again if necessary with a proper explanation of the error that is being seen and relevant tests to ensure it isn't breaking existing functionality.

The change means the latest release has started breaking test suites. I don't believe the original PR describes well enough what it was trying to fix, so rather than attempting to resolve that issue I suggest we revert it and ship a patch release.

@jessarcher
Copy link
Contributor

jessarcher commented Mar 10, 2020

I've just run into this as well. It seems to break clean installs as well as tests - basically any time the geoip.mmdb file doesn't exist, but the parent directory does.

My temporary workaround has been to add another subdirectory to the database path in the configuration so that mkdir has something to create.

@Torann
Copy link
Owner

Torann commented Mar 10, 2020

Fixed with release 1.2.1

@Torann Torann closed this Mar 10, 2020
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