Problem/Motivation

Steps to see the issue:

  • Turn on Language module on admin/modules page.
  • Add a language on admin/config/regional/language page if it is need.
  • See admin/structure/block page and start to Configure a block or Add custom block.
  • At Visibility/Language/Language selection can be seen: Not specified and Not applicable option, which has no sense here. These languages will never appear as a language of the page, the negotiators will not pick them. So setting a language condition on them is nonsensical.

Original:

Original

Proposed resolution

Change languages from All to only configurable:.

Remaining tasks

  • Writing a patch.

Thanks for the issue Goba!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

janoka’s picture

Issue summary: View changes
janoka’s picture

Issue summary: View changes
janoka’s picture

Gábor Hojtsy’s picture

Issue tags: +Needs tests

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.

Gábor Hojtsy’s picture

Status: Active » Needs review

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.

janoka’s picture

The last submitted patch, 6: 2318429-confusing_language_settings_at_blocks-6-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2318429-confusing_language_settings_at_blocks-6.patch, failed testing.

janoka’s picture

janoka’s picture

Status: Needs work » Needs review

The last submitted patch, 9: 2318429-confusing_language_settings_at_blocks-9-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2318429-confusing_language_settings_at_blocks-9.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -Needs tests

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).

janoka’s picture

Thanks! I will check it on tomorrow.

janoka’s picture

The last submitted patch, 15: 2318429-confusing_language_settings_at_blocks-15-fail.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

Title: Confusing setting at Block > Visibility > Language settings » Language condition / block language visibility includes useless options
Issue summary: View changes
janoka’s picture

janoka’s picture

Status: Needs work » Needs review

The last submitted patch, 19: 2318429-confusing_language_settings_at_blocks-19-fail.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

janoka’s picture

I'm sorry you had to repeat yourself. Let see again.

Gábor Hojtsy’s picture

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.
janoka’s picture

Thanks for your comment! Finally I will learn the correct use.

The last submitted patch, 23: 2318429-confusing_language_settings_at_blocks-23-fail.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, looks great. Thanks for sticking to it.

janoka’s picture

Assigned: janoka » Unassigned

Thank @Gábor Hojtsy for your enduring support!

The last submitted patch, 25: 2318429-confusing_language_settings_at_blocks-25-fail.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2318429-confusing_language_settings_at_blocks-25.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Random fail.

tim.plunkett’s picture

Version: 8.0.0-alpha14 » 8.0.x-dev

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
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.

janoka’s picture

Assigned: Unassigned » janoka
Gábor Hojtsy’s picture

Assigned: janoka » Unassigned
Status: Needs work » Reviewed & tested by the community

  • webchick committed 8391e27 on 8.0.x
    Issue #2318429 by janoka: Fixed Language condition / block language...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks @janoka for sticking to this :)

Status: Fixed » Closed (fixed)

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