Dependency on config storage causes circular reference in service container

That's an API change that is problematic, we'd need BC for it.

We could maybe switch to setter injection for that or just not inject it at all, so we only need to access it when actually called?

Well, the class uses syslog() PHP functions, that's not really something that you can unit test anyway and it's possible to mock Drupal:: calls as well, just like you can mock an injected container. Neither is very pretty as technically both result in a dependency on the container an not just their dependency. But, arguably injecting the container is slightly more decoupled, so yeah that works too.

@krystalcode @berdir thanks!

That's an API change that is problematic, we'd need BC for it.

What about creating a wrapper specific instance of ConfigFactoryInterface for syslog that does this container runtime load? This will avoid breaking BC.

https://www.drupal.org/project/filecache/issues/3045245#comment-13407004 also mentions that a cache rebuild is needed. So probably this will require an empty hook_update_N to rebuild the registry.

Well, the class uses syslog() PHP functions, that's not really something that you can unit test anyway

We have the Drupal\syslog_test\Logger\SysLogTest that switches to error_log instead of syslog calls so we can inspect the final result. See #2874069: Make syslog testable

Had to revert to db cache when this issue resurfaced today. It resurfaced when I updated composer packages. I had #2 installed with a composer patch.

Tried patch #5 but that requires refreshing the service_container, if I'm not mistaken. This was cached too and refused to be rebuilt. Couldn't uninstall syslog or enable dblog via drush due to fatal errors. Only way around this, it seems, was changing the db manually. Disabled filecache for now.

Perhaps hardcoding the syslog settings as constants of the SysLog class is acceptable since the settings are not configurable with a settings form anyway. That way they can be overwritten by extending the class. See attached patch.

Do these settings need to be configurations and do they need to be cached?

@basvredeling thanks for the patch.

Tried patch #5 but that requires refreshing the service_container, if I'm not mistaken. This was cached too and refused to be rebuilt. Couldn't uninstall syslog or enable dblog via drush due to fatal errors.

I think we should write documentation about how to restore the system, but a cache rebuilt will be needed anyway, I don't see a way to have a patch that changes services and don't require a cache rebuild.

Do these settings need to be configurations and do they need to be cached?

In D8 we are tied to have them as configurations, we cannot change that. Maybe we can avoid cache them, see my proposal inhttps://www.drupal.org/project/drupal/issues/3103620#comment-13423033

If I do not have filecache enabled, I get the following after using patch in #9
I did a drush cr
I got this:
TypeError: Argument 1 passed to Drupal\syslog\Logger\SysLog::__construct() must implement interface Drupal\Core\Config\ConfigFactoryInterface, instance of Drupal\Core\Logger\LogMessageParser given, called in drupal/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\syslog\Logger\SysLog->__construct() (line 45 of drupal/html/core/modules/syslog/src/Logger/SysLog.php).

if I have it enabled I do not get the above error

Found this issue while working on PHP 8.1. I don't think the PHP 8.1 fix will address this - #3224414: Installing the syslog module uses its configuration before it is written - but if you do want to fix this you need to use a service provider to decouple the service from configuration. An example of something in core already doing this is how \Drupal\language\LanguageServiceProvider sets the language.default_values parameter. For syslog we don't need to container param - we can just set the arguments directly.

Could the above patch be rerolled for 9.2 or above?

Rerolled for 9.5 but still seems there are questions needing answering for #16

Seems to also be an issue with the patch failing logging

TypeError : Drupal\syslog\Logger\SysLog::__construct(): Argument #1 ($config) must be of type array, bool given, called in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262

Is it possible the service is still needed?

For #23

This needed a couple minor fixups: The config array will be FALSE when syslog module is first installed, and format is now available in $this->config['format'] rather than $this->config->get('format').

Marking needs work as the syslog kernel tests are failing; the logger.syslog_test service needs to somehow be configured and registered? Moving to 10.1.x branch as this is still an issue there.

A couple other needs-work notes for anyone working on this:

1. A config event subscriber could be used to invalidate the container whenever the syslog configs are changed (saving the form, drush command, also at install time). As the configs will now be effectively cached with the container.
2. I believe BootstrapConfigStorageFactory::get() only retrieves saved configs, not configs overridden via global $config? syslog could merge the overridden and saved configs (defaulting both to empty array).

Ok I had some more time to bang on this.

1. Resolves the issues I raised in #26 and #27. Added container rebuild to the kernel tests, as I'm not sure how else to simulate the container being invalidated and rebuilt in between requests? Used global $config to merge any config overrides - is there a cleaner way to do this?
2. Adds a BC shim so logger.syslog doesn't throw an error after applying the patch, with a cached container: If config factory is passed in, it's used to build the required config array. Not necessary and could be removed, but seems nice for a logger to not blow things up if it can avoid it?
3. Adds a post_update hook so the container will be rebuilt.
4. Adds syslog_test to the syslog functional test, so we can verify that config event successfully triggered container rebuild and logging is working. Previously the functional test didn't actually functionally test logging.

Code style adjustment.

Well that seems wrong, as it results in caching the config overrides with the container, rather than allowing them to override the config at runtime. So here, I apply config overrides in the constructor.

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

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 is totally aligned with what I suggested in #4. 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 further discussion 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: #2838474: Remove dependency of "file_system" service on "logger"

So, if #2838474: Remove dependency of "file_system" service on "logger" 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 #2838474: Remove dependency of "file_system" service on "logger" does not address the issue - please let us know. Thanks.

I applied this #2838474: Remove dependency of "file_system" service on "logger" 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 also have filecache enabled in addition to workspace and scheduler if that helps.

After I applied the MR from this issue, it solved the problem.

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: #3437739: SchedulerManager should take an EventDispatcherInterface object, not ContainerAwareEventDispatcher

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.

I have done a drupal update from 10.1.8 to 10.4.1 and when i am enabling syslog i am getting below error

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "domain.route_provider", path: "options_request_listener -> domain.route_provider -> cache_tags.invalidator -> plugin.manager.block -> logger.channel.default -> logger.factory -> logger.syslog -> router -> router.no_access_checks". in Drupal\Component\DependencyInjection\Container->get() (line 149 of core/lib/Drupal/Component/DependencyInjection/Container.php).

When i tried patch #69 - https://www.drupal.org/project/drupal/issues/3103620#comment-15369587 composer gives error patch not apply but error gone. Not sure what is happening. Do we have any patch to resolve the issue.

service "domain.route_provider"

@rahulkhandelwal1990 It seems it is related to the domain module.

I was also facing the same error mentioned by @rahulkhandelwal1990 in 10.3 version and this patch fixed the issue. On upgrading to 10.4.1 the patch could not be applied. But the original issue seems not occurring in 10.4.1. Hope that is fine.

@dagmar, i am using "drupal/domain": "^2.0@beta" in my project. Not sure this is causing issue.

@jomsy, i have done upgrade to 10.4.1 but when i enable syslog module i am getting issue.

There's another issue at play here as well. With the syslog module enabled, calls to drupal_flush_all_caches resolves the asset.js.collection_optimizer service, which resolves about 5 other services until it eventually gets to creating a Syslog object. This object causes a cache_config rebuild before drupal_flush_all_caches is ready to start rebuilding.

Specifically, the drupal_static and to a lesser extent the twig caches will be stale / corrupt when this rebuild happens. The state of Drupal is kind of in a weird half-flushed state where all of the bins are empty, but drupal_static isn't.

I still think we should at least move the ->get() call out of the constructor and preferably remove the config entirely and just use settings. Anyone who needs syslog should be capable of customizing it through $settings. A bit tricky with BC of course.

That said, the backtrace in #88 doesn't seem related to config, I have no idea what the router has to do with syslog, maybe there are Url objects involved.

Removing the ability for loggers to depend on config would be a big trade-off for the contrib loggers that I maintain - there would no longer be a nice, friendly UI for admins to tweak logging without having to tinker with the settings file or yaml files. It certainly would help resolve circular dependency hell, though...

Another dup of this: #3129225: Circular reference between filecache and syslog

I'm running into such circular refs twice a year and every time it's like here a "fat constructor" with a config-get-call. I'm with @berdir that the proper fix is to move the call out of the constructor, and in all the cases i had that solved it.

Maintaining BC works w/ a __get facade. And we even don't have to deprecate it, just replace it w/ a PHP8.5 property getter hook in D12.

Ok, since this issue is progressing slowly, I added a proposed resolution D) to the issue summary, to fix the SysLog constructor. It's definitely correct to make that fix in the short term, but AFAIK, it won't help with edge cases where a ServiceCircularReferenceException is thrown at the time the logger service is being instantiated (due to the config dependency). Lazy services would help with many of those edge cases. Or establishing policy that loggers should not depend on configuration would be the nuclear option, with some usability impacts for site admins (and BC issues).

I think we should deprecate the bc layer for the config property - can add a trigger_error() in the __get() call. Properties of concrete classes aren't covered by the bc policy at all, doesn't hurt to have the bc especially since it's written already. Should we also look at making the config factory a service closure here?

Added the service closure. Another possibility, I guess, is to actually use the config property via __get() as a shorthand way to call ($this->configFactory)()->get('syslog.settings') :) But I didn't do that yet, just added a deprecation

I think there is some chance of loggers being serialized in various contrib and custom code (if not in core). See e.g. the dblog logger, which uses DependencySerializationTrait. So, service closures would only be usable here if/when they can be serialized.

Wow @mfb, this was a quick implementation.

Reviewed !14205 and it's exactly like I imagined in !97. Current tests are green.
I'd RTBC this, but it has the Needs-Tests tag.
Also yes, this must wait for the closure-serialization issue. Added it as related.
I see the point for the lazy service, but imho the service closure does the same w/ less overhead.

Added a functional test - but for now, it's going to be failing because I added a test that the logger can be serialized.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rerolled MR !14205, back to needs review, still waiting on #3544994: Make service closures serializable in classes using DependencySerializationTrait

Shouldn't this be postponed on that issue?

@smustgrave it needs a review. If you like proposed resolution D then it's blocked. If not then we circle back to proposed resolution A.

Hello all,

This comment is one of the results from making S.D.O. issue #184413 public. I believe that Resolution A is the only safe path forward.

There are certain conditions in Drupal Core that can allow loggers to be used before the kernel fully boots, which can be used in conjunction with other enabling criteria to lead to certain bad things. The logger service is used in the errors.inc file, which has the potential to fire very, very early, leading to things like #3207813: ModuleHandler skips all hook implementations when invoked before the module files have been loaded. While the syslog service in and of itself isn't the only actor involved, it's a link in the middle of a chain needed to put the system in a corrupted state.

To simulate such an early kernel failure, simply visit https://www.example.com?ajax_page_state=not_an_array, in which a HTTP middleware service allows an exception to unwind to the custom exception handler that's registered by the kernel. This failure occurs as early as in the \Drupal\Core\DrupalKernel::preHandle method.

Similarly, I think that any any all services that have the potential to be used in this early bootstrap phase should be similarly audited and have policies created about them needing to function in extreme situations without any complex dependencies.

> Or establishing policy that loggers should not depend on configuration would be the nuclear option, with some usability impacts for site admins (and BC issues).

I'm not sure who needs to make that decision, but I'm a clear +1 on that Option A, to drop the dependency on config.

It's been a long time since I've reviewed merge requests doing that, but my proposal specifically would be:

Similar to #3571593: Deprecate translation.path config in favor of settings, compare the config in a post update, if it matches the default, delete it. deprecate the config schema.

For runtime BC, I'm unsure. There are very few subclasses in contrib, some are test modules:

https://search.tresbien.tech/search?q=%22extends%20SysLog%22%20-r%3Adrup...

There's a constructor override there, but no access to ->config from what I see.

Looking at our BC policy (https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...), it has room for exceptions: security hardening as well as "BC ... has negative consequences. ". Will need confirmation from a core committed, but IMHO, this could qualify for that. This can bring down production sites with enough bad luck.

Not sure how far that exception could go? the property? completely ignoring config going forward?

I'm not exactly sure why the service provider idea (option B) is striked through, can't find a comment that explains why this isn't feasible? If we make it container parameters then we could keep a static service definition and the service provider would only be temporary BC that replaces the parameter on the flag if the config exists?

dblog doesn't have any configuration, I think the crossover of people using syslog who also can't edit settings.php is probably quite low.

If we used container parameters, we could do the (horrible but effective) trick of setting container parameters based on config - same as is done for multilingual and one or two other places, that might not require actually removing the configuration UI then. Would get rid of the runtime dependency which is where everything goes wrong.

#53 did that, but that approach was then abandoned based on the following comments, possibly because config overrides wouldn't work for that. That's why I suggested doing regular container parameters with config just for BC, and then I suppose also deprecate/remove the UI for it.

I should also note that the reproducible steps to get caches in a corrupt state is seemingly only a drupal 10 and earlier problem.

That's not to say there aren't other problems that can arise because of this, but it's just something to keep in mind when triaging this issue. If the solution is something that will only become generally available in Drupal 11+, the ROI may simply not exist in T minus 9 months.

I'm still a big +1 for keeping low level services as "dumb" as possible though. It makes a lot of sense from a hardening perspective.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

I crossed out proposed resolutions based on what reviewers preferred at the time, to indicate what was currently the "chosen" resolution. Reviewers can still review all the proposed resolutions. Apparently, at this point, proposed resolution D) should be crossed out as well, and the chosen resolution is now A) so I'm adding the "needs issue summary update" tag.