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

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

The last submitted patch, 24: 3134349-24-testonly.patch, failed testing. View results →

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($type) 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 much better.

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

Automatically closed - issue fixed for 2 weeks with no activity.