Remove dependency of "file_system" service on "logger"

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Inside of Drupal\Core\File\FileSystem class, instance of logger is used only once in the chmod() method to log error when chmod() PHP function returns FALSE.

The issues is quite old. At the moment, file system makes a heavy use of the logger.

Please review.

NW for MR feedback above.

I think given that we have code like

if (is_dir($directory) && !is_writable($directory) && !$file_system->chmod($directory)) {

we should go for solution 2 as the less disruptive for now. And leave it up to the calling code to log if necessary. In the above example, logging is actually not appropriate as this is used to inform the user.

Hiding patches as we have an MR now.

I've gone for a halfway house for #40 and added $throw to \Drupal\Core\File\FileSystem::chmod() so the caller is in control.

I don't think there are any use-cases for $throw in core but I've ensured there is test coverage.

Updated issue summary with latest changes.

I updated the change record to more completely describe the impacts of logging being removed. Now I realize it needs a bit more work as I see Alexpott added an additional new change record, so they are partially redundant.

Why do we need the $throw argument, the caller can detect failure via the return value already?

@longwave yeah I went that way originally but then got cold feet when deleting the exception.. but I agree - let's remove $throw.

If we think it's worth cleaning up, perhaps in a followup we can add deprecations in all the cases we currently return FALSE and notify users that we will start throwing exceptions instead in the next major.

@longwave I don't think we want that though - in the case of chod there are times that throwing an exception would lead to way more complex calling code.

@alexpott I'd say it should still be mentioned in the change record that logging is removed from those other methods, not just chmod. But you removed my mention of those. This is relevant if the calling code caught the exception and assumed an error was already logged by the file_system service.

@mfb I've listed the public method that no longer log in the CR and detailed that it is the callers responsibility to log if any method results in FALSE or throws an exception.

Fixed deprecation, also fixed a nit in the docblock.

📌 Inject services into LoggerChannelFactory RTBC is somewhat related, there is special code in LoggerChannelFactory to handle the case where the injected dependencies are not present (during the installer), once this dependency is gone I think that can also be removed.

Tagging with Needs followup for #52 📌 Remove dependency of "file_system" service on "logger" Needs work

Code changes make sense and are primarily removing logging/loggers and altering comments about logging/loggers.
Checked CR and that mentions all methods involved in not being logged any more.
Tests are green.

For me this is RTBC.

The followup isn't really needed until both issues land, just they overlap a bit, so we can probably fix it directly in whichever lands second - removing the tag.

I'm glad somebody is an optimist and is sure we'll remember ;)

Well it's only a bit of dead code if we don't. And 📌 Fix install container definition of StreamWrapperManager Needs review is making me think that this might not even get used...

I applied this 📌 Remove dependency of "file_system" service on "logger" Needs work I get this:
Circular reference detected for service "workspaces.manager", path: "scheduler.manager -> dat
e.formatter -> workspaces.manager -> logger.channel.workspaces -> logger.factory -> logger.syslog".

#58: You might have c/p a wrong issue link (it is to this very issue) in there?

@SocialNicheGuru we're going to need some more information about how to reproduce what you are seeing. I have installed:

1. Standard profile
2. Enabled scheduler, workspaces, filecache
3. Configured filecache
4. Enabled syslog
5. Boom things are broken
6. Applied this MR and cleared the cached - things are fixed

I can create the datetime.formatter and workspaces.manager services just fine after this. But my datetime.formatter does not depend on workspaces.manager - how is that happening on your install?

This MR is definitely resolving a bug.

One question on the MR.

@catch's feedback has been addressed. This looks good to go now. Thanks @kim.pepper.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

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

By the way, I noticed that at least one contrib file system decorator (s3fs) still has a dependency on logger. And logging seems even more useful/necessary for s3fs than the default local file_system (all kinds of API errors, network errors, etc.). So I still see some utility in the alternative work-around of a lazy config service that loggers could depend on ( 🐛 Dependency on config storage causes circular reference in service container Needs review )