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

Add feature to optionally gzip dumped .sql #424

Merged
merged 7 commits into from
Apr 26, 2017
Merged

Conversation

pdewit
Copy link
Contributor

@pdewit pdewit commented Apr 21, 2017

According to issue #372, add an option to gzip the SQL files before zipping them. Very useful in my use case since .sql.gz files are a lot smaller while still able to be loaded into my SQL client, unlike .zip files.

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

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

Could you also add tests for class that does the gzipping?

/*
* Gzip the dumped SQL files for extra compression
*/
'gzipSql' => false,
Copy link
Member

Choose a reason for hiding this comment

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

use snake case here

* @param $temporaryFilePath
* @return string
*/
protected function gzipSqlFile($dbDumper, $temporaryFilePath)
Copy link
Member

Choose a reason for hiding this comment

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

Refactor this to it's own class that is responsible for gzipping

@pdewit
Copy link
Contributor Author

pdewit commented Apr 21, 2017

Thanks for the feedback! I hope it's better now.

gzwrite($gzipOut, fread($gzipIn, 1024 * 512));
}
fclose($gzipIn);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I should have also mentioned that I really dislike else statements. Could you refactor this code to avoid them entirely?

@pdewit
Copy link
Contributor Author

pdewit commented Apr 22, 2017

Yeah I can see that, the error handling also didn't work correctly that way so I added a test for that and fixed it. Hope it's better now 😃

@freekmurze
Copy link
Member

Cool, thanks

Holding of on this for a while, because we might use gzip for compressing the other files as well. When I have some time on my hands I'll think this through.

@afraca
Copy link

afraca commented Apr 24, 2017

Hey @pdewit , thanks for doing this. As we only use the database backup feature, it's fine as it is, but I understand the plan from @freekmurze .

(I know if I really want it, I should do it myself. Thanks for all your hard work guys)
If you happen to approach this yourselves, could you make sure the generated database backup file can be imported by PMA / Adminer? They support gzipped files.

@pdewit
Copy link
Contributor Author

pdewit commented Apr 24, 2017

@afraca you can use my fork in the meantime, I'm not sure if it's going to be kept up to date but I will use it in production myself. https://github.com/pdewit/laravel-backup/tree/gzip-sql

@freekmurze freekmurze merged commit a40da08 into spatie:master Apr 26, 2017
@freekmurze
Copy link
Member

I've decided to merge this in so you can already make use of this.

In the next major version (no ETA on that yet, probably early 2018) I plan to make this kind of compression a first class citizen in the package.

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