Cache flushing showing wrong output message when unsuccessful due to permission issue

[damms005]

• Laravel Version: 7.9.2
• PHP Version: 7.4.7
• Database Driver & Version: mysql Ver 8.0.20 for Linux on x86_64

Description:

This issue was previously reported but @taylorotwell closed it because it was not "very critical" and also not "worth the time to fix at the moment".

However, it's now almost 7 years since then. I believe that the year 2013 to present year 2020 is a considerably long period of time, and hopefully, it should now matter that if the command cache:clear or optimize:clear fails, it should not falsely display Application cache cleared!, when indeed a permission issue prevented the application cache stored in storage/framework/cache/data from being cleared.

Also, while investigating this, I found that the issue lies in the implementation of deleteDirectory() in Filesystem.php, which does not countenance the success/failure of calling delete() on a file.

This means that, in the future, this bug can also potentially affect any implementation that depends on deleteDirectory() in Filesystem.php, not only the core Laravel cache flushing.

If a PR is welcomed to this effect, I can submit one, since it only requires correctly consuming delete().

Steps To Reproduce:

1. Create cached application data
2. Run cache:clear or optimize:clear from an environment that does not have write permission to files stored in storage/framework/cache/data
3. Output wrongly shows Application cache cleared!, when, in reality, it was not cleared

[driesvints]

This is a file permission issue and unrelated to Laravel.

[damms005]

@driesvints Since it is a permission issue, will it be wrong if Laravel correctly responds to such permission issue and even optionally notify users of same instead of wrongly (silently) notifying user that action was successful?

[Eduruiz]

I'm deploying my first laravel application and lost several hours because of this bug.
Maybe for more advanced developers and for those who are more comfortable with back-end issues this is an obvious one, but for me it was a really hard to get, I just understand what was happening with the help of another developer who has already suffered with the same situation.

[driesvints]

https://laravel.com/docs/7.x/installation#configuration

[mallardduck]

@driesvints I think what's being asked of this issue isn't how/why the issue happens, but if Laravel's artisan command can be more verbose when that issue happens.

My assumption is that it should be possible to do so - though may not be as simple/easy to pull off as one would expect it might be.

---

For instance the cache:clear command already does this when the cache flush() returns false for results. However - what complicates 'solving' this - is that the File based cache relies on what the Filesystem tells it.

So as @damms005 pointed out the root issue lies in Filesystem::deleteDirectory() - since the method silently fails if there are issues with removal, then the cache clearing process cannot report a partial failure to flush (file based) caches.

All that said, IMHO I don't think solving it there is viable.

Since Filesystem is such a fundamental component and changing deleteDirectory that much would surely break tests and be a BC issue.

Solving the issue in the FileStore class could be an option since we could be 'defensive' about the results of deleteDirectory. Yet it would more FS calls, since it's a reactive solution. So not completely ideal.

So it would look like:

        foreach ($this->files->directories($this->directory) as $directory) {
            $deleteResponse = $this->files->deleteDirectory($directory);
            if (! $deleteResponse || $this->files->hasDirectory($deleteResponse)) {
                return false;
            }
        }

Instead of:

        foreach ($this->files->directories($this->directory) as $directory) {
            if (! $this->files->deleteDirectory($directory)) {
                return false;
            }
        }

[driesvints]

Thanks for the detailed reply @mallardduck. You can PR that for Taylor to have a look at 👍

[mallardduck]

@driesvints if you think that's a worthwhile solution to discuss I can make that happen! TBH I was not sure how much I liked it yet, it seems there could be a more elegant solution. Yet if this PR helps get that conversation started lets go for it.