Language condition / block language visibility includes useless options

I think for blocks this is definitely required. There is no sense of picking those two languages, they will never appear as page negotiated languages, so it is pointless to display them there. I'm not 100% sure about the intentions of the condition API, whether these options would be useful in other cases, but likely not. So I think this looks fine.

Needs tests.

A larger scope version of this is #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) BTW. I think we should fix this one, that went nowhere for years.

All right, the reason this is failing, is because you did not modify the buildConfigurationForm() method on the language condition, only the summary method. So the form will have the elements still.

1. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,41 @@
   + * Definition of Drupal\language\src\Tests\LanguageBlockSettingsVisibility
   

   The current correct form of this would be "Contains \Drupal..."

2. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,41 @@
   +  // Modules to enable for testing.
   ...
   +    // Create user.
   ...
   +    // Log in for testing.
   ...
   +    // Add a language (Hungarian) to test.
   ...
   +    // Get a block config page.
   ...
   +    // Verify that 'Not specified' option do not appears.
   ...
   +    // Verify 'Not applicable' option do not appears.
   

   I don't think any of the comments add anything to the code, all can be removed IMHO.

3. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,41 @@
   +  protected $webUser;
   ...
   +    $this->webUser = $this->drupalCreateUser(array('administer languages', 'access administration pages', 'administer blocks'));
   ...
   +    $this->drupalLogin($this->webUser);
   

   If you log the user in directly in setUp() which is totally fine, then there is no reason for the property. Less code :)

4. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,41 @@
   +    $this->assertNoFieldByXPath('//input[@id="edit-settings-visibility-language-langcodes-und"]', NULL, 'The \'Not specified\' option appears at block config, language settings section.');
   ...
   +    $this->assertNoFieldByXpath('//input[@id="edit-settings-visibility-language-langcodes-zxx"]', NULL, 'The \'Not applicable\' option appears at block config, language settings section.');
   

   The string feedback should define what happens when it *passes*. The current text says what happens when it fails. That is incorrect. When the feedback displays them as failures, it says the statement presented was not true, while in your case that is the opposite.

5. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,41 @@
   +} ¶
   

   Remove extra whitespace from the end of line. (Keep extra line ending on the end of file).

Getting there!

1. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,29 @@
   + * Definition of Drupal\language\src\Tests\LanguageBlockSettingsVisibility
   

   See my review on this line above at #13/1.

2. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,29 @@
   +class LanguageBlockSettingsVisibility extends WebTestBase {
   

   The name should end in "Test" so LanguageBlockVisibilityTest altogether.

3. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,29 @@
   +    $this->assertNoFieldByXPath('//input[@id="edit-settings-visibility-language-langcodes-und"]', NULL, 'The \'Not specified\' option does not appear at block config, language settings section.');
   +    $this->assertNoFieldByXpath('//input[@id="edit-settings-visibility-language-langcodes-zxx"]', NULL, 'The \'Not applicable\' option does not appear at block config, language settings section.');
   

   Let's add one positive test for Hungarian to make sure the xpath works. This way even if the page is a 404 or 403, the test would still pass.

4. +++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibility.php
   @@ -0,0 +1,29 @@
   \ No newline at end of file
   

   Add newline at end of file.

Also if you post interdiffs, it is way easier to review what is going in your updates. See https://www.drupal.org/documentation/git/interdiff for guidance.

+++ b/core/modules/language/src/Tests/LanguageBlockSettingsVisibilityTest.php
@@ -0,0 +1,31 @@
+ * Definition of \Drupal\language\src\Tests\LanguageBlockSettingsVisibility

Everything looks good, except I need to ask the third time again to change this to "Contains \Drupal..." and the name of the class is now not correct in this comment.

No its not "Contains of" but just Contains :) Also I actually forgot to say there needs to be a dot at the end. This sounds like absolute pedantry, but commiters would not welcome an RTBC from me with known issues like that :/

$ git grep "Contains \\Drupal"
core/lib/Drupal/Component/Annotation/AnnotationInterface.php: * Contains Drupal\Component\Annotation\AnnotationInterface.
core/lib/Drupal/Component/Annotation/Plugin.php: * Contains Drupal\Component\Annotation\Plugin.
core/lib/Drupal/Component/Annotation/PluginID.php: * Contains Drupal\Component\Annotation\PluginID.
core/lib/Drupal/Component/Plugin/DerivativeInspectionInterface.php: * Contains Drupal\Component\Plugin\DerivativeInspectionIn
core/lib/Drupal/Component/Utility/DiffArray.php: * Contains Drupal\Component\Utility\DiffArray.
core/lib/Drupal/Component/Utility/NestedArray.php: * Contains Drupal\Component\Utility\NestedArray.
core/lib/Drupal/Component/Utility/Variable.php: * Contains Drupal\Component\Utility\Export.
core/lib/Drupal/Core/Access/AccessCheckInterface.php: * Contains Drupal\Core\Access\AccessCheckInterface.

Yay, looks great. Thanks for sticking to it.

Random fail.

public function summary() {
    $language_list = language_list(LanguageInterface::STATE_ALL);

in

Drupal\language\Plugin\Condition\Language</code could do with the same change.

Can we get a followup to inject the language manager properly and remove the usage of <code>language_list()

That followup could also introduce a proper test of Drupal\language\Plugin\Condition\Language because we don't really have one.

I wish that this wasn't introducing a new webtest but I can;t see a way around that as we're lacking coverage of this condition anyway.

Opened #2328463: Language condition plugin needs LanguageManager injected, unit tests added.

Seems like the follow-up will address Alex's concerns.

Committed and pushed to 8.x. Thanks!

Thanks @janoka for sticking to this :)