FileSystem::deleteRecursive() shouldn't log a message when it tries to delete a non-existent directory

Here's a patch.

There are no explicit tests for this currently, we have some for FileSystem but not for ::delete() or ::deleteRecursive().

I think this is a reasonable change to make, if you ask to delete a directory and it's already deleted, then the job is done.

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -336,6 +336,7 @@ public function delete($path) {
   +      dump($path);
   

   Debug code :)

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -367,7 +372,6 @@ public function deleteRecursive($path, callable $callback = NULL) {
   -
   

   Whitespace changes are out of scope.

Tidied up the cruft.

@longwave what do you think about removing the notice altogether, i.e. from FileSystem::delete()?

The last submitted patch, 2: 3374319.patch, failed testing. View results →

@catch I am leaning towards yes, but not 100% convinced just yet. 📌 Remove dependency of "file_system" service on "logger" Needs work makes a similar argument that the filesystem service is low level, and should not be logging or presenting messages directly to users. In the long term it feels like the service should throw exceptions in the case of error, and the caller should decide what to do with them, because it usually depends on why the methods are being called in the first place. But changing all that in a backward-compatible way is non trivial.

FileSystem::deleteRecursive() shouldn't log a message when it tries to delete an empty directory

I think the title is not accurate. The file system does not complain on empty directories. It only complains on non-existing directories.

Fixed title, thanks!

Given we have 📌 Remove dependency of "file_system" service on "logger" Needs work I think the narrow scope here is fine. Even if we added explicit test coverage for ::deleteRecursive() I don't think we'd add test coverage for this issue since it would be asserting against historical behaviour not current behaviour (i.e. we could only assert the absence of a log message, which we'd never do if there hadn't been a log message in the first place), so hoping this can go in as-is.

re: #7 I think if calling code wants to do something special if a directory doesn't exist, then it should check for that itself.

I like the approach in #10, the tests were a bit naff anyway (see 🐛 Keep assets directory when clearing cache Closed: duplicate )

Committed e983ee9 and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks!

Hello all,

Thanks for the contribution.

I've downloaded commit #23140022 as a patch and applied to my core/lib/Drupal/Core/File/FileSystem.php file. After "drush cr", the annoying "The file assets://css was not deleted because it does not exist.
The file assets://js was not deleted because it does not exist. " warning is gone.

But I notice that, it still deletes the js & css directories under the assets directory.

My environment is PHP 8.1, Ubuntu 22.04, nginx and PostgreSQL 15.

What can I do? Should I reactivate the issue?

Best,
Orkut

@Orkut Murat Yılmaz the directory deletion itself isn't a bug as such - it will be recreated when the next aggregate is written and starts of empty when you install a site / enable aggregation. We don't need to delete the directory though, so a follow-up to try to optimize things so we only delete the files would be fine.

Hi, I've tried the patch but the css/js folders are not recreated.

@catch, sorry but the directories are not generated neither after fresh install, nor after enabling aggregation, as @FiNeX mentioned on comment #18.

I've also tried on another environment (docker4drupal) and the result is the same.

I'm changing the issue's status, but I'm not sure that I can do it, since it's a core issue. Please correct me, if I'm wrong.

Hi, after lot of searching I've solved the issue thanks to: https://www.drupal.org/project/drupal/issues/3368769#comment-15118911 🐛 10.1.x breaks Nginx + PHP-FPM with too many redirects even when nginx config is changed Closed: works as designed

After changing my nginx config, as mentioned on @FiNeX's comment #20, nothing has changed.

@Orkut Murat Yılmaz please open a new issue for the problem you're having, it's in the same general area as this issue, but this is just about the unnecessary log message.

The directories are not generated automatically after deletion, they're only generated when an asset URL is requested (i.e. by hitting a page when aggregation is enabled).

You will need to check the HTML of the page, check the URL of the js and css aggregates, check the headers (for example X-Powered-By Drupal means it was served by PHP), and then see if the file exists on disk.

If the CSS/JS aggregate looks correct, but it's not written to disk, then it's possible you're running into something like 🐛 Aggregation URL hashes should be built from normalized list of libraries Fixed , although it would be quite extreme for this to happen to every single aggregate. You should also see if you get the same issue with a clean install of Drupal core vs. with any contrib modules you might have installed.

Lastly, the place that the files get written to disk or not is in system module's AssetControllerBase and the two controllers that extend from it, so if you're comfortable debugging, and everything else seems to be working, you could check where it's getting to in that logic before (not) writing the file.

@catch, thanks for your beautiful & detailed explanation:)

I'll try the things you've written and if I couldn't solve the error, I'd open a new issue.

have a nice one and take care:)

Automatically closed - issue fixed for 2 weeks with no activity.

Sorry, I forgot to mention that, it was all about nginx config. Changing it solved the error.