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

Use base_path() for default data_path #69

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Use base_path() for default data_path #69

merged 3 commits into from
Feb 8, 2023

Conversation

Orclyx
Copy link
Contributor

@Orclyx Orclyx commented Jan 25, 2023

This wraps the default value used for data_path with base_path(), which provides similar handling to what is being done in getVendorPath() already.

The relative data_path was causing an issue where when running a Laravel worker process, the relative path seemed to be relative to somewhere other than the base path and the worker didn't have permission to write there. This caused a fatal error similar to the one described in #61

@mrtimbrook
Copy link
Collaborator

Good catch! Thanks for adding this.

I was just testing it and getting an error when including the data files as it adds the base_path again to each file on line 56 (https://github.com/area17/blast/blob/1.x/src/DataStore.php#L56).

If you could remove the base_path here as part of this PR we should be good to merge. Thanks!

@Orclyx
Copy link
Contributor Author

Orclyx commented Jan 30, 2023

@mrtimbrook Ah have changed, thanks.

It occurred to me that this isn't as close to how getVendorPath() works as maybe it ought to be: if you set a relative vendor_path in the blast.php config file then getVendorPath will apply base_path(), but it won't if vendor_path is absolute (good). If you set a relative base_path in config that probably won't work any more which seems bad. Happy to iterate on this further to solve if you'd like.

@mrtimbrook
Copy link
Collaborator

mrtimbrook commented Jan 30, 2023

I think you're right and it makes sense to follow the same pattern as the getVendorPath() method.

@Orclyx
Copy link
Contributor Author

Orclyx commented Jan 30, 2023

Have reworked and added tests for the different scenarios:

  • custom relative path
  • custom absolute path
  • default path

@mrtimbrook
Copy link
Collaborator

Sorry for the delayed reply. This all looks great (the tests are 🔥 )! Thanks for your work.

@mrtimbrook mrtimbrook merged commit f4766aa into area17:1.x Feb 8, 2023
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