Inject page_cache_kill_switch service to LanguageNegotiationBrowser

Thanks for working on this!

I think you need to implement ContainerFactoryPluginInterface to inject the killswitch service from the container when the plugin is instantiated - we don't have autowiring yet, unfortunately.

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -25,6 +26,23 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase {
+  public function __construct(KillSwitch $killSwitch) {
+    $this->killSwitch = $killSwitch;

While it looks awkward Drupal uses snake_case for all variables, except member variables which use camelCase, so this should be:

public function __construct(KillSwitch $kill_switch) {
  $this->killSwitch = $kill_switch;

Looks great now!

Looks good to me as well, however this is now a 9.1.x issue (that might still be considered for backporting).

Spoke with @xjm about this: to be able to safely backport this to 8.9, we need to add a BC layer for the constructor param and a deprecation error stating that the param is optional in 9.0 and required in 10.0. See FileTranslation::__construct() for an example. The unit test should cover also the deprecation, so we could add the unit test class here and the actual test coverage in #3130104: Add unit test coverage for LanguageNegotiationBrowser.

It could be done backward compatible way like #3126127-11: Fix LanguageNegotiationMethodInterface::getLangcode() result docblock

As negotiator is plugin then setter injection instead of constructor one makes the change future-proof.
Tldr kill switch will change in session related issues so better not introduce constructor argument which could be deprecated

Good point, that's probably the reason why DI wasn't used in the first place...

Great! Btw I find this setter useless as you can access class's property directly.

CR maybe required... Because plugin now implements the new interface

Thanks!

1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
   @@ -25,6 +28,29 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase {
   +  private $pageCacheKillSwitch;
   

   Usage of private visibility is controversial and whether it should be considered ok is still being decided, so let's make this protected to avoid discussions down the line.

2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
   @@ -25,6 +28,29 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase {
   +  public function setPageCacheKillSwitch(KillSwitch $page_cache_kill_switch) {
   

   I agree with Andy: given that we can access private/protected members from a static method, the setter is not needed. It would be if this were a service exploiting setter injection via the container.

It looks RTBC for me

Looks good to me as well. I'm not sure we'd normally need a CR for such a change, given that plugins are not public API, but since we already have one, can we update it?

Committed b152620 and pushed to 9.1.x and friends. Thanks!

Any links to where I can read some more on this controversy? I am coming from a place in my mind where I would favor private over protected.

I cannot find the issue I had in mind, asked in Slack, I will post it here once I get a reply.

I think related exactly about it

Maybe CR needs better wording, like "service explicitly depends on policy"

Because it's plugin it should not be created directly - there's plugin manager

But implementation of interface should be mentioned

@stefanos.petrakis:

The oldest issue I could find discussing private visibility is #2081945: [Policy, no patch] Reintroduce 'private' visibility to object-oriented code., a related discussion then took place at #3019332: Use final to define classes that are NOT extension points.

This kind of injection - neither a setter nor a constructor one - does it go by a specific name?

Not that I know of. If it were me, I'd probably call it "static factory method injection" ;)

Looks it's all a factory) https://refactoring.guru/design-patterns/factory-method

Looks great and could be published!

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -25,6 +28,22 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase {
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    $instance = new static();

Just found that it missing unittest coverage #3130104: Add unit test coverage for LanguageNegotiationBrowser

This plugin does not use plugin arguments ($configuration, $plugin_id, $plugin_definition) which are not passed to constructor now, the pointed issue should catch and solve it