Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
Proposed resolution
Change languages from All to only configurable:.
Remaining tasks
- Writing a patch.
Thanks for the issue Goba!
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff-2318429-23-25.txt | 575 bytes | janoka |
#25 | 2318429-confusing_language_settings_at_blocks-25.patch | 2.65 KB | janoka |
#25 | 2318429-confusing_language_settings_at_blocks-25-fail.patch | 1.87 KB | janoka |
#23 | interdiff-2318429-19-23.txt | 575 bytes | janoka |
#23 | 2318429-confusing_language_settings_at_blocks-23.patch | 2.65 KB | janoka |
Comments
Comment #1
janoka CreditAttribution: janoka commentedComment #2
janoka CreditAttribution: janoka commentedComment #3
janoka CreditAttribution: janoka commentedPlease review.
Comment #4
Gábor HojtsyI 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.
Comment #5
Gábor HojtsyA 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.
Comment #6
janoka CreditAttribution: janoka commentedPlease check my test.
Comment #9
janoka CreditAttribution: janoka commentedPlease check my test again.
Comment #10
janoka CreditAttribution: janoka commentedComment #13
Gábor HojtsyAll 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.The current correct form of this would be "Contains \Drupal..."
I don't think any of the comments add anything to the code, all can be removed IMHO.
If you log the user in directly in setUp() which is totally fine, then there is no reason for the property. Less code :)
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.
Remove extra whitespace from the end of line. (Keep extra line ending on the end of file).
Comment #14
janoka CreditAttribution: janoka commentedThanks! I will check it on tomorrow.
Comment #15
janoka CreditAttribution: janoka commentedPlease test.
Comment #17
Gábor HojtsyGetting there!
See my review on this line above at #13/1.
The name should end in "Test" so LanguageBlockVisibilityTest altogether.
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.
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.
Comment #18
Gábor HojtsyComment #19
janoka CreditAttribution: janoka commentedThanks for your review! I hope it will be better.
Comment #20
janoka CreditAttribution: janoka commentedComment #22
Gábor HojtsyEverything 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.
Comment #23
janoka CreditAttribution: janoka commentedI'm sorry you had to repeat yourself. Let see again.
Comment #24
Gábor HojtsyNo 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 :/
Comment #25
janoka CreditAttribution: janoka commentedThanks for your comment! Finally I will learn the correct use.
Comment #27
Gábor HojtsyYay, looks great. Thanks for sticking to it.
Comment #28
janoka CreditAttribution: janoka commentedThank @Gábor Hojtsy for your enduring support!
Comment #32
Gábor HojtsyRandom fail.
Comment #33
tim.plunkettComment #35
alexpottin
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.
Comment #36
janoka CreditAttribution: janoka commentedComment #37
Gábor HojtsyOpened #2328463: Language condition plugin needs LanguageManager injected, unit tests added.
Comment #39
webchickSeems like the follow-up will address Alex's concerns.
Committed and pushed to 8.x. Thanks!
Comment #40
Gábor HojtsyThanks @janoka for sticking to this :)