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 writable directory for sqlite default location #3151

Merged
merged 9 commits into from
Jun 24, 2020

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Jun 23, 2020

To avoid removed by incident as @ivantcholakov comment at #3142 (comment)

Checklist:

  • Securely signed commits

@samsonasik samsonasik changed the title use dedicated data directory in root project for sqlite default lcoaiton use dedicated data directory in root project for sqlite default location Jun 23, 2020
@samsonasik
Copy link
Member Author

/cc @michalsn

@michalsn
Copy link
Member

Honestly speaking data folder placed in a root folder looks a bit "random" to me. Also, I don't like the idea of having a "special" folder in the project root just in case someone will use SQLite3 database.

Overall I'm not sure if placing database in the writable folder is really an issue... but if we must move it somewhere else, I would choose app/database folder - it seems more logical and usually, we don't delete anything from there.

Anyway, this change will stay with us for some time, so I can't be the one to decide about this.

@lonnieezell, @MGatner any thoughts?

@MGatner
Copy link
Member

MGatner commented Jun 23, 2020

I think that for permissions handling we should keep everything that the framework needs write access to in the writable folder. I understand the concerns from @ivantcholakov that this groups temp data together with important and sensitive data but I think the configuration and security issues are more important.

Personally I always put my SQLite database at writable/database.db as it is super clear what it is (this is your database) and skips the extra folder. We also have precedence for this from the Playground: https://github.com/codeigniter4projects/playground/blob/develop/.env.example

@samsonasik
Copy link
Member Author

ok, closing it

@samsonasik samsonasik closed this Jun 23, 2020
@michalsn
Copy link
Member

Well... now I'm wondering if placing database at writable/database.db isn't actually a better idea 😅

@MGatner
Copy link
Member

MGatner commented Jun 23, 2020

🤷‍♂️ I think it's open. Since #3142 hasn't been released I think it is safe to change, but I also think your solution is fine. The only additional advantage I see to writable/database.db is that it doesn't require a change to .gitignore and the new data folder, which amount to required updates to existing projects.

@michalsn
Copy link
Member

It's definitely not too late for any changes in this matter.

Another advantage is that when we access writable directory, we immediately see the database file, so it's less likely to be deleted by accident.

@samsonasik samsonasik reopened this Jun 23, 2020
@samsonasik
Copy link
Member Author

Ok, I will update to point to writable dir

@MGatner
Copy link
Member

MGatner commented Jun 23, 2020

Sounds good. Thanks you two! Good progress. It will be nice not having to set that variable in every SQLite test scenario now too :)

@samsonasik samsonasik changed the title use dedicated data directory in root project for sqlite default location use writable directory for sqlite default location Jun 23, 2020
@samsonasik
Copy link
Member Author

@michalsn @MGatner updated

@MGatner
Copy link
Member

MGatner commented Jun 23, 2020

Looking better! Do we know what $this->database will be by default? I think it would be good to add the file to .gitignore (e.g. writable/database.db) but I can't immediately tell if that's the correct name.

@samsonasik
Copy link
Member Author

updated with simplified check database !== 'memory' && strpos($this->database, DIRECTORY_SEPARATOR) === false for adding WRITEPATH prefix

@michalsn
Copy link
Member

michalsn commented Jun 23, 2020

I would try to add writable/*.db and writable/*.sqlite to the .gitignore. $this->database is empty by default, so it can be anything. In fact, we can't be sure if the file extension will be used at all.

@samsonasik
Copy link
Member Author

I've added writable/*.db and writable/*.sqlite to .gitignore

@samsonasik
Copy link
Member Author

I updated tests config to use "database.db" to prove that the patch is working

@samsonasik
Copy link
Member Author

@MGatner @michalsn travis build green 🎉

@MGatner MGatner merged commit 55f947f into codeigniter4:develop Jun 24, 2020
@samsonasik samsonasik deleted the use-dedicated-dir branch June 24, 2020 22:59
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.

3 participants