Deprecate dependency setters and getters on LanguageNegotiatorInterface

Second line should be indented the way we do on @throws and other tags.

Thanks! Tthe formatting looks good to me now.

But I cannot comment on whether the methods should be deprecated, so I think I'll leave the RTBC to someone else... and this is probably not really just Documentation: deprecating a function is not really a docs issue.

+++ b/core/modules/language/src/LanguageNegotiatorInterface.php
@@ -119,6 +119,9 @@ public function reset();
+   * @deprecated Scheduled for removal in Drupal 9. Use constructor injection
+   *   instead.

Coding standards say: https://www.drupal.org/coding-standards/docs#deprecated

Should say "@deprecated in 8.1.x for removal before Drupal 9.0.x. Inject the current account using the constructor."

Also, we need an issue to add the constructor parameter.

Patch still applies.

Ossum.

I think this makes sense to deprecate in 8.2.x for removal in 9.x, to not pollute the API with setters for things that should never be changed after the object is constructed.

When we do deprecations, we should document what usages exist in core, to ensure they are not "wishful deprecations". For this method, based on a grep, it looks to be:

core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php:      $this->negotiator->setCurrentUser($this->currentUser);
core/modules/language/src/HttpKernel/PathProcessorLanguage.php:    $this->negotiator->setCurrentUser($current_user);
core/modules/language/src/LanguageNegotiator.php:      $instance->setCurrentUser($this->currentUser);
core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php:    $method->setCurrentUser($this->user);
core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php:    $method->setCurrentUser($this->user);

(Other references appear to be a similarly named method on other objects.) So that's two cases where it's being set by other code, the main implementation itself, and its tests.

Looking at those two usages, in LanguageRequestSubscriber:

public function onKernelRequestLanguage(GetResponseEvent $event) {
  if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
    $request = $event->getRequest();
    $this->negotiator->setCurrentUser($this->currentUser);
    $this->negotiator->reset();
    if ($this->languageManager instanceof ConfigurableLanguageManagerInterface) {
      $this->languageManager->setNegotiator($this->negotiator);
      $this->languageManager->setConfigOverrideLanguage($this->languageManager->getCurrentLanguage());
    }
    // After the language manager has initialized, set the default langcode
    // for the string translations.
    $langcode = $this->languageManager->getCurrentLanguage()->getId();
    $this->translation->setDefaultLangcode($langcode);
  }
}

That's not actually part of the constructor for LanguageRequestSubscriber, even though LanguageRequestSubscriber::__construct() receives both the current user and the negotiator. Why is it being set during a particular method call?

Similarly looking at PathProcessorLanguage:

public function __construct(ConfigFactoryInterface $config, ConfigurableLanguageManagerInterface $language_manager, LanguageNegotiatorInterface $negotiator, AccountInterface $current_user, ConfigSubscriber $config_subscriber) {
  $this->config = $config;
  $this->languageManager = $language_manager;
  $this->negotiator = $negotiator;
  $this->negotiator->setCurrentUser($current_user);
  $this->configSubscriber = $config_subscriber;
}

That one is on construction, so we are good there.

I also think the deprecation needs an example or a slight clarification. I assume this means that LanguageNegotiatorInterface implementations should be defined with the current user as a parameter for their constructors. Can we spell that out a bit more clearly, and possibly give/reference an example?

Finally, I tagged #2494695: [meta] Deprecate dependency setters and getters on interfaces for framework manager review, so we should probably wait on that signoff to proceed with this. (Makes sense to me, but I'm not a framework manager.)

Thanks all!

We now have deprecation procedures that involve triggering a silenced E_USER_DEPRECATED notice and removing all usages in core apart from legacy testing. So there's work to do here.

I think the idea of removing unnecessary API and unnecessary setters is correct.