Dependency on config storage causes circular reference in service container

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Think since we changing the syslog service it will need a change record.

Wonder what the immediate impact would be for existing sites using the old service?

Can this get an issue summary update explaining the proposed solution in the patch, api changes, etc.

Updated the issue summary and started a draft change record @ https://www.drupal.org/node/3343843 →

The Needs Review Queue Bot → tested this issue. It 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.

Fixup some documentation issues

It was pointed out to me that services also need to be deprecated before removed

See config.storage.staging in 9.5x for an example. Will need simple test coverage too.

@smustgrave Can you elaborate what you mean by deprecated - do you mean trigger a deprecation notice if a ConfigFactoryInterface is passed in to the SysLog service? And could you propose what additional tests would be helpful? The patch already overhauls the existing tests so they work.

-  logger.syslog:
-    class: Drupal\syslog\Logger\SysLog
-    arguments: ['@config.factory', '@logger.log_message_parser']

This service is being removed from what I can tell and it may be in use.

No, the service is not removed, it's just removed from the .yml file and instead registered by SyslogServiceProvider::register()

Ah thanks @mfb!

I would like to review this as well. A bit concerned about the backward compatibility.

@dagmar wonder if you've had a chance to take a look?

@smustgrave This is what I have in mind. Something similar to what I mentioned in #8.

Using this approach, the signature of the Logger is not affected.

There is one test that is failing for me locally, I'm not sure how to fix it. Maybe @mfb can figure out why.

1) Drupal\Tests\syslog\Functional\SyslogTest::testSettings
Symfony\Component\DependencyInjection\Exception\RuntimeException: Unable to dump a service container if a parameter is an object without _serviceId.

/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:436
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:316
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:230
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:134
/var/www/html/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php:75
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:935
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:810
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:600
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:236
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:465
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:545
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:364
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

The last submitted patch, 42: 3103620-42.patch, failed testing. View results →

That error is saying that you'd need to turn the defaultSyslogConfig() argument into an actual service - it wants to represent that class as a service ID when dumping the container.

@dagmar were you interested in adding a new service, or should we just get more reviews on patch #34?

This is a reroll of #34

Thanks @mfb. I think the service is a good idea. But I won't be able to work on this for some time, so #47 for now is a good option as well.

Per #48 (leaning on the submaintainer).

I think this will require a change record as it is changing the constructor signature.

@dagmar there is already a draft change record for this issue; feel free to tweak it. Perhaps it should mention that for backwards compatibility purposes, the SysLog service still accepts config as ConfigFactoryInterface.

1. +++ b/core/modules/syslog/src/Logger/SysLog.php
   @@ -39,29 +39,57 @@ class SysLog implements LoggerInterface {
       */
   -  public function __construct(ConfigFactoryInterface $config_factory, LogMessageParserInterface $parser) {
   

   I think this should be array|ConfigFactoryInterface

2. +++ b/core/modules/syslog/src/Logger/SysLog.php
   @@ -39,29 +39,57 @@ class SysLog implements LoggerInterface {
   +    if ($config instanceof ConfigFactoryInterface) {
   +      $config = $config->get('syslog.settings')->get();
   +    }
   

   I think we should issue a deprecation here.

3. I also ponder whether we should move the config to a container parameter to completely remove this from configuration? Is there any value to this being configuration - we don't provide a UI.

Addresses #52

I also ponder whether we should move the config to a container parameter to completely remove this from configuration? Is there any value to this being configuration - we don't provide a UI.

But there is a configuration UI, see syslog_form_system_logging_settings_alter()

Appears #52 has been addressed.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Setting back to RTBC as patch should still be passing (it just got retested today)

This issue has been on my radar for a while.
I leave it at RTBC, but just because I don't know how to better solve the problem.

What I would like to point out is that this superficially uses config. But without using the config manager it is not really using it as it should.
The patch adds a way to get config overrides from settings.php. But this won't work with other kinds of config override. One would not expect this to be translated for sure, but other config overrides exist in contrib (for example the key → module)
I don't know if instead of injecting the dependency one could use \Drupal::service instead? Or the very least we should probably document that this is an exceptional case and that one should not follow this example because other config overrides do not take effect. I think it is probably fine here but I am a bit worried that this pattern would be copied from.

It might be possible for the service to become ContainerAware and then only retrieve the config when it needs it, to avoid the loop when initialising services? However Symfony is trying to deprecate ContainerAware and force everyone to use dependency injection.

I think a valid long term solution might be to move syslog config to container parameters. Syslog is a fairly advanced use case and I think there is little use for the UI; I would think most users who are using syslog would also be comfortable with editing a services.yml to configure it.

Or is it possible to move the config dependent parts into a separate lazy service, which could only be instantiated when the config is needed? Unsure if we will still run into the same issue with the container though.

I could imagine a hypothetical corner case, e.g. if file_system needed to log something in its constructor (which doesn't actually happen afaik). filecache requires file_system, but syslog lazily-loaded config requires filecache, so you have a circular reference again. But, maybe lazy services are a good enough solution for this particular issue.

My take on lazy services was they are somewhat semantically equivalent to container-aware services, as either way the container is injected into the service, allowing you to get other services as needed.

Here's a lazy config factory, which can be used as a drop-in replacement for the standard config factory.

fixed typo in #61

If a different solution is being taken could the IS be updated?

Updated the proposed resolution section to list all the proposed resolutions we have so far.

@dagmar as sub-maintainer which solution would you prefer?

I think solution C proposed in #61 🐛 Dependency on config storage causes circular reference in service container Needs review is totally aligned with what I suggested in #4 🐛 Dependency on config storage causes circular reference in service container Needs review . Thanks! @mfb

I updated the change record to match the new chosen resolution.

+++ b/core/modules/syslog/src/Logger/SysLog.php
@@ -70,6 +77,10 @@ protected function openConnection() {
 
+    if (!isset($this->config)) {
+      $this->config = $this->configFactory->get('syslog.settings');
+    }
+

Why is this only necessary in the ::log() method and not the ::openConnection() method? Should there be a new protected ::geConfig() method that handles initializing the property?

Or, what do you think about getting rid of the config property? The config can be retrieved from the factory when needed.

Another change here: I renamed the service from "lazy.config.factory" to "config.factory.lazy" - I don't know what the naming policy is for services, but I was thinking if a standard prefix is important, then it should probably start with "config"

Converted to MR as testing the patch was hitting no space left on device

While we're changing a property on Drupal\syslog\Logger\SysLog, perhaps we should also "upgrade" this class to use constructor property promotion? Just pushed a commit doing so.

Small stuff.

@smustgrave should we create a followup issue to add type declarations to ConfigFactoryInterface and ConfigFactory, at which time they could be added to LazyConfigFactory too?

Could you open that follow up

@smustgrave Sure

By the way, where the coding standards say

Parameter and return typehints should be included for all new functions and methods, including new child implementations of methods for existing classes and interfaces.

does this mean LazyConfigFactory should go ahead and add type declarations where its parent ConfigFactoryInterface only has docblock annotations? It's not 100% clear to me what that sentence is saying.

That's correct, even though the parent doesn't have it new child functions can have return types.

In that case then I guess this doesn't need followup, we can add return types

Thanks for working on that!

Changes look good to me so it's a +1 RTBC for me. per a new approach for #needs-review-queue-initiative going to leave in review for additional reviews for a few days.

Been about a week, still believe changes are fine so going to mark now.

Assigning this to me to review.

I think that given the complex nature of the bug it'd be great to add a test case for the bug that proves the solution fixes it.

I'm also a bit suspicious of using a lazy service. In this can the service is doubly lazy - as config.factory.lazy is lazy and so is the proxy that wraps it - that's a lot of laziness.

I've discussed this issue with @catch and @longwave and we've not really come up with a conclusion beyond a general feeling that configurable logging services are a problem and that we should consider moving these settings to container parameters as indicated by @longwave in #58.

After discussion further with @alexpott and @catch we think this issue should not be fixed as-is.

Cache backends should not rely on loggers, as they are too low level for that. If something goes wrong in a cache backend, you should throw an exception and fail hard instead of trying to log something.

Memcache should already no longer suffer from this problem as #2887558: Circular reference detected for service "cache.backend.memcache" → was marked fixed long ago.

Filecache does still suffer on this problem, but for a slightly different reason: it relies on the file system service, which injects a logger. But, for similar reasoning to cache backends, we already think this should not happen: 📌 Remove dependency of "file_system" service on "logger" Needs work

So, if 📌 Remove dependency of "file_system" service on "logger" Needs work is fixed then Filecache should no longer suffer from this issue when syslog is in use. Marking needs review to get feedback from other contributors on this proposal first, then if this is agreed we can update #3045245: Circular reference detected between filecache and syslog → with the new preferred solution.

After discussions with @catch and @longwave decided to postpone - if one of the contributors here feels that doing 📌 Remove dependency of "file_system" service on "logger" Needs work does not address the issue - please let us know. Thanks.

I applied this 📌 Remove dependency of "file_system" service on "logger" Needs work and not the patch from this issue. 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 applied the MR from this issue and it solved the issue.

I tried to reproduce the circular dependency described in #86 on the 10.3.x branch, and what I got was

Circular reference detected for service "scheduler.manager", path: "scheduler.manager"

which is apparently caused by the need to log a TypeError: 🐛 SchedulerManager should take an EventDispatcherInterface object, not ContainerAwareEventDispatcher Fixed

Once I manually patched this bug in scheduler module, the ServiceCircularReferenceException was also resolved. But, this may be a different circular dependency than the one @SocialNicheGuru described.