Bugfix check if driver is local is changed in laravel v9

AndreiTelteu commented 2 years ago

WHY

BEFORE - What was wrong? What was happening before this PR?

Issue #100 - Call to undefined method League\Flysystem\Filesystem::getAdapter()

In laravel v9, league/flysystem package has beed upgraded. As a result $disk->getDriver()->getAdapter() does not exist. We can use $disk->getAdapter() instead. https://github.com/Laravel-Backpack/BackupManager/blob/4c1878c4edfd37c0d2aa0adbe81add9dcb56d274/src/app/Http/Controllers/BackupController.php#L26 However, this will either break ($adapter instanceof Local) either lose backwards compatibility with older laravel versions. In laravel < 9 - $disk->getAdapter() is instance of League\Flysystem\Adapter\Local In laravel >= 9.* - $disk->getAdapter() is instance of League\Flysystem\Local\LocalFilesystemAdapter

Also the download operation is broken because $disk->getAdapter()->getPathPrefix() does not exist anymore. https://github.com/thephpleague/flysystem/blob/3.x/src/Local/LocalFilesystemAdapter.php

AFTER - What is happening after this PR?

An easier solution is to just check the driver name. config("filesystems.disks.$disk_name.driver") == 'local'

For the download operation i used $disk->download($file_name); documented here: https://laravel.com/docs/9.x/filesystem#downloading-files

HOW

How did you achieve that, in technical terms?

config("filesystems.disks.$disk_name.driver") == 'local'

if (method_exists($disk->getAdapter(), 'getPathPrefix')) {
    // to keep backwards compatibility
    $storage_path = $disk->getAdapter()->getPathPrefix();
    return response()->download($storage_path.$file_name);
} else {
    return $disk->download($file_name);
}

Is it a breaking change or non-breaking change?

No

How can we test the before & after?

Install laravel v9 with backpack v5 and this backup manager, then just navigate to /admin/backup

A future update/improvement is to show the download link with any disk driver, and use Storage::url or Storage::temporaryUrl

welcome[bot] commented 2 years ago

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly. Please keep in mind that:

if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you! -- Justin Case The Backpack Robot

tabacitu commented 2 years ago

Ouch! Thank you for the PR @AndreiTelteu ! And sorry for leaving this unattended for so long. @pxpm has merged your PR into #102 , which will merged in a few days and tagged as a new version.

Thanks again for the PR, and for your patience! 🙏

AndreiTelteu commented 2 years ago

No worries. I'm glad to help. 😁

tabacitu commented 2 years ago

Just realised we can't wait to launch v4 to merge this. We'd still have a v3 with a HUUGE bug in it - doesn't work with Laravel 9, even though it's installable like that.

So I've merged your PR to the v3 branch @AndreiTelteu and released v3.0.6 with your PR.

Your PR will look closed (cause it points to master but it's actually merged). Thanks a lot! Sorry it took so long to see the light 😀

Laravel-Backpack / BackupManager