\Drupal\language\LanguageNegotiator does not handle PluginNotFoundException and break the site completely

Replaced the deprecated code.

drupal_set_message() with
$this->messenger->addError();

Correction in the previous patch. Missed the modification in the service file to add messenger argument.

+++ b/core/modules/language/language.services.yml
@@ -4,7 +4,7 @@ services:
+    arguments: ['@language_manager', '@plugin.manager.language_negotiation_method', '@config.factory', '@settings', '@request_stack', '@messenger']

Patch looks good to me since it is handling the language-negotiator plugin exception by adding messenger service to language service. Seems very appropriate way.
+1 RTBC

I think it's safe to state that a missing plugin should not collapse an entire system. If that's the case, I don't understand why it taken so long for this simple patch to be adopted into the codebase.

Consider this comment acknowledgement that the problem still exists and a vote to adopt this patch.

This is great! This is the sort of thing that makes us look as stable as a stable D8/D9 site is when properly working :)

Is there any use case where having the site completely crash with the uncaught exception might be the preferred route (ie: some chain of events that would follow that should never have occurred), or is this not our burden to consider?

+++ b/core/modules/language/src/LanguageNegotiator.php
@@ -329,13 +351,11 @@ public function updateConfiguration(array $types) {
-
         // If the language type is locked we can just store its default language
         // negotiation settings if it has some, since it is not configurable.
         if ($has_default_settings) {
           $method_weights = [];
           // Default settings are in $info['fixed'].
-
           foreach ($info['fixed'] as $weight => $method_id) {

These whitespace changes are unrelated.

+++ b/core/modules/language/src/LanguageNegotiator.php
@@ -128,7 +140,17 @@ public function initializeType($type) {
+            $this->messenger->addError(t('Error finding language negotiator plugin %method-id: @message', [
+              '%method-id' => $method_id,
+              '@message' => $e->getMessage(),
+            ]), 'error');

@@ -329,13 +351,11 @@ public function updateConfiguration(array $types) {
-

I'm not sure about outputting a message, which is a UI operation, in a service like this which is fairly deep. A watchdog error should be sufficient?

#16 1st point fixes, reverted unrelated whitespace changes.

Re-roll for 9.5.x

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.

As a bug this will need a test case.

Test only patch.

#21 and #24 combined, please review

New parameter should default to null with a trigger_error that it will be required in D11. Would search the repo for exact working. Then a simple unit or kernel test that when you pass null to the constructor you get that trigger_error response.

Also with a new parameter it will need a change record to announce this constructor now takes a new parameter.

Thanks

1. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -130,7 +142,17 @@ public function initializeType($type) {
   +            // If a plugin is not found, simply log the error so user may handle it.
   

   Comment is too long, should be wrapped.

   Also, we're not 'just logging the error' since we're also showing an error message.

2. +++ b/core/modules/language/tests/src/Functional/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,45 @@
   +    $this->assertSession()->statusCodeEquals(200);
   

   If we're only testing the status, could this be done in a kernel test?

Please review, covers #28 and #29

1. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -70,6 +72,13 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   +   * Messenger Service.
   

   We use

      * The messenger.
   

   everywhere in Drupal core. Change to that.

2. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -70,6 +72,13 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   +  protected $messenger;
   

   Nitpick: Put $messenger after $requestStack (before $currentUser and $negotiatedLanguages). I wouldn't be so nitpicky if I didn't have more feedback.

3. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -83,13 +92,20 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   +   *   Messenger Service.
   

   Same here. Use "The messenger"

4. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -83,13 +92,20 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $messenger argument is deprecated in drupal:10.1.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3280207', E_USER_DEPRECATED);
   

   The linked change record is unrelated. We need to create a draft one for the changes in this issue and link it here.

5. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -130,7 +146,17 @@ public function initializeType($type) {
   +            watchdog_exception('language', $e);
   

   We can use \Drupal::logger() instead?

6. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,40 @@
   +  public function testLanguageNegotiatorNoPlugin() {
   

   This test has no assertions.

   I suggest:

   + We assert we don't have the exception we had until now.
   + We assert the message is shown.
   + We assert the error is logged.

I also tempt to agree with @joachim at #16. A visitor could see this error which is scary and he can't do anything about it. We might want to just log it.

Addressing the suggestions of #31 fixing these issues, Only point #4 is left as i not clear with it completely. Apart all issues being addresses in attached Patch with interdiffs. Leaving issue still in NW.

Was mentioned in #32 that #31.4 has not been addressed.

Removed the messenger, just logs the error. No change record needed any more, because interface does not change.

#35 looks close!

1. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,52 @@
   +use ColinODell\PsrTestLogger\TestLogger;
   

   TIL :-)

2. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,52 @@
   +    // Assert we don't have the exception we had until now.
   +    $this->assertNotInstanceOf('\Drupal\Component\Plugin\Exception\PluginNotFoundException', $switchLinks);
   

   I doubt this is the right assertion, as the exception would be thrown, not returned.
   Can we upload a test-only patch to verify?

I deleted the assertNotInstanceOf

+++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
@@ -0,0 +1,50 @@
+    $this->assertFalse($logger->hasErrorThatContains('Valid plugin IDs for Drupal\language\LanguageNegotiationMethodManager are'));

I'd prefer to assert by "The "Drupal\Tests\language\Kernel\LanguageNegotiatorPluginTest" plugin does not exist." but don't want to nitpick. Is RTBC for me.

Thanks for your work on this!

+++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
@@ -0,0 +1,50 @@
+    $this->assertFalse($logger->hasErrorThatContains('Valid plugin IDs for Drupal\language\LanguageNegotiationMethodManager are'));

is assert false right here?

The comment says 'assert the error is logged'

but then we assertFalse?

My bad, overlooked:

1. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,50 @@
   +    $this->container->get('logger.factory')
   +      ->get('package_manager')
   +      ->addLogger($logger);
   

   This needs to be language, not package_manager.

   Also, even changing that didn't get it to work.

   But

       $logger_factory = $this->createMock(LoggerChannelFactory::class);
       $logger_factory->expects($this->once())
         ->method('get')
         ->with('language')
         ->willReturn($logger);
       $this->container->set('logger.factory', $logger_factory);
   
   

   worked for me to fix the test.

2. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -0,0 +1,50 @@
   +    $this->assertFalse($logger->hasErrorThatContains('Valid plugin IDs for Drupal\language\LanguageNegotiationMethodManager are'));
   

   As @larowlan pointed out, this needs to be assertTrue.

   I'd change the string to 'The "Drupal\Tests\language\Kernel\LanguageNegotiatorPluginTest" plugin does not exist.' while we are there.

+++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
@@ -0,0 +1,50 @@
+    $languageNegotiator->initializeType(LanguageInterface::TYPE_URL);
+    $switchLinks = $languageManager->getLanguageSwitchLinks(LanguageInterface::TYPE_INTERFACE, new Url('<current>'));

Let's also change the test to be explicit about the exception:

    try {
      $languageNegotiator->initializeType(LanguageInterface::TYPE_URL);
      $switchLinks = $languageManager->getLanguageSwitchLinks(LanguageInterface::TYPE_INTERFACE, new Url('<current>'));
    }
    catch(PluginNotFoundException $exception) {
      $this->fail('Plugin not found exception unhandled.');
    }

Includes issues of #39-#42

1. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -13,6 +16,7 @@
   +  use ProphecyTrait;
   
   @@ -24,27 +28,32 @@
   +    $languageNegotiator->setCurrentUser($this->getProphet()->prophesize('Drupal\Core\Session\AccountInterface')->reveal());
   

   We can call prophesize directly as you were doing. No need to include the trait.

2. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorPluginTest.php
   @@ -24,27 +28,32 @@
   +      $switchLinks = $languageManager->getLanguageSwitchLinks(LanguageInterface::TYPE_INTERFACE, new Url('<current>'));
   

   We don't even need this line now that I see it, as it would fail on the initializeType method and we are not asserting on the switchLinks

address #44

try to fixed CCF #45

Looks great now, all feedback was addressed. Thanks!

Updating issue credits

removed these out of scope whitespace changes on commit

diff --git a/core/modules/language/src/LanguageNegotiator.php b/core/modules/language/src/LanguageNegotiator.php
index 72bf83cea9e..0007dab69ed 100644
--- a/core/modules/language/src/LanguageNegotiator.php
+++ b/core/modules/language/src/LanguageNegotiator.php
@@ -340,13 +340,11 @@ public function updateConfiguration(array $types) {
         // settings are always considered non-configurable. In turn if default
         // settings are missing, the language type is always considered
         // configurable.
-
         // If the language type is locked we can just store its default language
         // negotiation settings if it has some, since it is not configurable.
         if ($has_default_settings) {
           $method_weights = [];
           // Default settings are in $info['fixed'].
-
           foreach ($info['fixed'] as $weight => $method_id) {
             if (isset($method_definitions[$method_id])) {
               $method_weights[$method_id] = $weight;

Committed to 10.1.x and backported to 10.0.x

Will backport to 9.5.x if test run passes

Reverted from 10.0.x as TestLogger does not exist there.

You could possibly use BufferingLogger instead

Reverted from 10.1.x too, if we use BufferingLogger instead, we should have the same test in all three branches for consistency.

Took https://git.drupalcode.org/project/drupal/-/commit/b279c9946176bfc7a7dd5....
Changed TestLogger for BufferingLogger. Took as a reference \Drupal\Tests\field\Kernel\EntityReference\EntityReferenceSettingsTest that uses that class, but kept the changes to the minimum.
As I just did what @larowlan said, I think this is small enough to RTBC it myself.

I've applied and run locally the test for 9.5.x, 10.0.x, 10.1.x, all green.

One thing that I'm not sure how we operate with. There was a change record required at some point, and the draft was created. But it's not true anymore. Do we keep it as draft? Delete it? It can be confusing if we leave it as is.

Committed to 10.1.x and backported to 10.0.x and 9.5.x

Deleted the change record