search

stefanzweifel / laravel-backup-restore

A package to restore database backups made with spatie/laravel-backup.
https://stefanzweifel.dev/posts/2023/06/15/introducing-laravel-backup-restore
MIT License
165 stars 15 forks source link

Better unpacked dump detection #62

Closed alies-dev closed 9 months ago

alies-dev commented 9 months ago

There are few ways to solve the issue. This PR uses PendingRestore::getAvailableDbDumps to check existence of dumps; another way - add the same file extension filter to PendingRestore::hasNoDbDumpsDirectory. I prefer the current approach to make codebase smaller and do not repeat the logic. But I'm ok to fix PendingRestore::hasNoDbDumpsDirectory if you think it's better.

BTW, it was the only usage of PendingRestore::hasNoDbDumpsDirectory. I haven't removed this method because it was public and not marked as @internal (which means it will be BC break).

PS: I discovered this issue debugging my CI check (probably caused by the DB connection misconfiguration), the message was really confusing:

image

"dumps" is plural because it should be plural for zero :)

PPS: How about listing all extracted files in the NoDatabaseDumpsFound message? This will improve debugging experience a lot. I can create a PR.

stefanzweifel commented 9 months ago

PS: I discovered this issue debugging my CI check (probably caused by the DB connection misconfiguration), the message was really confusing: "dumps" is plural because it should be plural for zero :)

From your comment I'm not sure if this is just a comment for your specific project, or if you see this as an issue for this package.

The "Importing database dumps …" message is only shown when dumps are available to import.

Or is this all related to the other PR of yours (#61) and the message was shown to you because you use bz2 for your dumps?

alies-dev commented 9 months ago

Hey @stefanzweifel

From your comment I'm not sure if this is just a comment for your specific project, or if you see this as an issue for this package.

sorry for not being clear.

The "Importing database dumps …" message is only shown when dumps are available to import.

not exactly. Firstly, both my PRs fixed this issue, but from different angles :)

Previous code called PendingRestore::hasNoDbDumpsDirectory that returned false: there are .bz2 in the dir. Then str()->plural($dumps) called where $dumps is an empty array (or Collection—I don't remember). Why empty - because PendingRestore::hasNoDbDumpsDirectory checks for ANY files, PendingRestore::getAvailableDbDumps takes into account file ext (and .bz2 was missing in the list).