Remove dependency of "file_system" service on "logger"

I vote for solution 1 as the cleaner OO based one.

+1 for solution 1

Wouldn't it be better not to throw but to just have the function return an error value instead ? (like traditional C/UNIX I/O).

Here's an initial go at solution 1.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -357,3 +357,4 @@ public function jsonSerialize() {
+w

Seems to be breaking the patch :D

Oops!

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -63,10 +57,12 @@ class FileSystem implements FileSystemInterface {
       * @param \Psr\Log\LoggerInterface $logger
       *   The file logger channel.
   ...
   -  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger) {
   +  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger = NULL) {
   

   php doc needs to point Optional for logger

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -752,9 +713,6 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0)
   -    else {
   -      $this->logger->error('@dir can not be opened', ['@dir' => $dir]);
   -    }
   

   the only regression place

Thanks @andypost.

1. Fixed
2. Not sure what do here? I don't think it makes sense to throw an exception.

This patch should fix a few test fails related to the manual registration of the FileSystem service in install.core.inc.

Fix test fails in FileSystemDeprecationTest.

It blocks further fixes for file system and config storage

Had a chat about this with @larowlan and couldn't find anything about a bc compatibility policy for logging. Given we are throwing exceptions, I think this means we are covered for the most part.

The only outlier is:

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -752,9 +713,6 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0)
-    else {
-      $this->logger->error('@dir can not be opened', ['@dir' => $dir]);
-    }

...which currently logs an error when scanning a directory tree and gets to a directory it can't open.

A new exception should work in that case as well.

Re-roll of #19

Forgot to attach the file!

Re: #22

A new exception should work in that case as well.

I don't think it will, because that will halt the scan. Currently it just logs a warning and continues.

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? We're already inconsistent where some methods throw exceptions and others return FALSE, but this doesn't make it any better.

@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.

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

Tagging with Needs followup for #52

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 #3416697: Remove install container definitions of FileSystem and StreamWrapperManager is making me think that this might not even get used...

I am saying that I thought that this issue would help me solve circular recurrence. But it did not.

I applied this #2838474: Remove dependency of "file_system" service on "logger" 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".

I have filecache, workspace, and scheduler enabled.

this issue solved this problem for me #3103620: Dependency on config storage causes circular reference in service container

This comment, https://www.drupal.org/project/drupal/issues/3103620#comment-15408346, redirected to this issue for a fix. I do not think it is a fx.

#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!

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 (#3103620: Dependency on config storage causes circular reference in service container)