Blocks seems to be another case where, besides having translatable blocks, we may like to have the option of defining different blocks -may be related or not- per each language.

So the first solution that I thought of was adding a language field to the block table -already added the db field, maybe too quick...- and selecting blocks for display depending on language, like what we did for path aliases. The problem with this option is that it won't allow to show a module provided block for two languages but not for others.

I.e. We have Spanish, French, English. Then we want to show the 'latest comments' block (provided by comment.module) for Spanish and French but not for English. This is not possible right now if we simply add a language field to the 'block' table, as this block should be linked to two languages instead of only one.

So I am thinking: Would it be a good option to have a different table 'block_language' so you can enable a single block for as many languages as you want, like for roles?
May be some more work, bit more complex, some performance impact -blocks table is queried for each page request and we'd have an additional join here-... But on the other side it would be a more 'complete' solution.

Please comment.

CommentFileSizeAuthor
#100 move-block-test-to-block-module.patch14.17 KBGábor Hojtsy
#93 move-block-test-to-block-module.patch14.18 KBGábor Hojtsy
#90 135464-language_visibility-c90.patch20.51 KBGábor Hojtsy
#87 135464-language_visibility-c87.patch19.96 KBGábor Hojtsy
#86 135464-language_visibility-c86.patch20.01 KBGábor Hojtsy
#84 135464-language_visibility-c84.patch20.3 KBGábor Hojtsy
#82 135464-language_visibility-c82.patch20.54 KBGábor Hojtsy
#81 BlockLanguage.png91.31 KBGábor Hojtsy
#81 interdiff.txt9.08 KBGábor Hojtsy
#81 135464-language_visibility-c81.patch20.6 KBGábor Hojtsy
#71 135464-language_visibility-c71.patch18.23 KBArtusamak
#69 135464-language_visibility-c69.patch18.4 KBArtusamak
#64 135464-language_visibility-c64.patch15.99 KBArtusamak
#61 135464-language_visibility-c61.patch10.58 KBArtusamak
#55 135464-language_visibility-c55.patch12.52 KBArtusamak
#52 135464-language_visibility-c52.patch12.47 KBArtusamak
#50 135464-language_visibility-c50.patch7.78 KBYesCT
#48 135464-language_visibility-c48.patch7.78 KBYesCT
#48 diff_of_c46_c48.txt197 bytesYesCT
#46 135464-language_visibility-c46.patch7.79 KBYesCT
#46 diff_of_c45_c46.txt1.02 KBYesCT
#45 135464-language_visibility-c45.patch7.79 KBYesCT
#44 block role visibility.png68.57 KBYesCT
#43 135464-language_visibility-c41.patch7.82 KBsaltednut
#39 135464-language_visibility-c36.patch7.28 KBsaltednut
#37 135464-language_visibility-c35.patch10.11 KBsaltednut
#36 135464-language_visibility-c33.patch43.38 KBsaltednut
#34 135464-dashboard-blocks-showing.png22.06 KBsaltednut
#33 135464-language_visibility-c32.patch41.47 KBsaltednut
#30 135464-language_visibility-c30.patch13 KBArtusamak
#27 135464-language_visibility-default.PNG18.48 KBArtusamak
#27 135464-language_visibility-set.PNG18.65 KBArtusamak
#25 135464-language_visibility.PNG20.08 KBArtusamak
#24 135464-language_visibility-c24.patch6.85 KBArtusamak
#21 135464-language_visibility-c21.patch5.61 KBArtusamak
#12 locale_hook_block_load.patch1.43 KBz.stolar
#8 block_intl_2.patch5.47 KBz.stolar
#7 intl_block_1.patch2.79 KBz.stolar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Roberto Gerola’s picture

Hi Jose`.

> was adding a language field to the block table -already added the db field, maybe
> too quick...- and selecting blocks for display depending on language

Yes, I have implemented block support in localizer i this way, adding also
"Any" as optional language.
If a block has "Any" as language it will showed in every language.

Probably your proposal to have a list of languages for every block is more
flexible. For my point of view could be very useful also for nodes.

In every case, by the users feedback, it seems that many times you need to translate
only the block title and not the body.

Gábor Hojtsy’s picture

Project: Internationalization improvements for Drupal 6.x-dev » Drupal core
Version: » 7.x-dev
Component: Code » language system

Moving to Drupal 7 queue. We don't have a separate code repository now, so no reason to have this separated as if someone is working on it.

Jose Reyero’s picture

Issue tags: +i18n sprint
z.stolar’s picture

Assigned: Unassigned » z.stolar

Assigning to myself for a while. I'll see how far I can get with that...

z.stolar’s picture

Is there any rule-of-thumb as to whether to use another column in block table with a serialized array of languages, or a whole another table?

Jose Reyero’s picture

@z.stolar

Yes, there is, as you may be joining the language condition in you'll need another table, otherwise it won't work for queries. Also serializing has its own problems so it's always the last resource.

z.stolar’s picture

Assigned: z.stolar » Unassigned
FileSize
2.79 KB

The attached patch is the beginning of the work. It contains the additions to the block configuration form.
It counts on an additional column in block table, where it serializes the language codes, but that's probably not the best practice, as it will be harder to alter queries later on (for example: to decide whether to display a block or not.)

Next steps:

- Add a table and change the patch
- Add the necessary code to decide whether to display a block
- Add languages to the blocks admin table (admin/build/block)

z.stolar’s picture

Status: Active » Needs review
FileSize
5.47 KB

Corrections in this patch:

- Added a table block_language, currently with no indexes.
- Values are correctly saved and retrieved in block configuration form

I'd appreciate some quick review, just to tell if I'm in the correct direction. Also, suggestions for indexes are welcome, though I think it's better to leave this to the end.

catch’s picture

I'm not sure about this approach - there's already #79571: Allow blocks to appear in multiple regions which would allow for blocks to have multiple 'instances' - so for the use case of showing something in Spanish and French, but not English, that ought to get us in that direction (although I'm not sure if it's enough by itself).

So I'd lean towards adding a language column to the blocks table here to cover the most basic use case, then getting more substantial refactoring of the blocks system in which is being worked on elsewhere - which should allow for the cross-language requirements here to be covered in a more generic way.

The question is though, if those more substantial refactoring patches don't make it in, is an 'all' and single-language setting going to be enough in the meantime? If it's not, we should at least make sure that the join only happens with locale installed - and also benchmark it.

catch’s picture

I'm fairly sure a SELECT on the block table with a WHERE on block_language is going to be hard or impossible to index properly. But that structure seems like the best approach for actually storing the block/language combinations, so maybe we just need to shift responsibility instead.

So the idea would be instead of {block_language}, we have a {locales_block} table - with the same schema, just owned by locale.module. Then a locale_block_visibility() callback is run just after the initial list of blocks is loaded, and does a separate single query per page on {locales_block}, which should be quicker than an un-indexed join. This would allow for blocks to be filtered out quickly before they get rendered, and means the only change to block module would need to be the hook addition - with everything else just copied over to locale.

Having said all that, I reckon we can do it in a separate patch - and let this go in pretty much as is - then shift visibility over to a refactored system at a later date - and having the block_language table like this would let that happen pretty cleanly.

Here's the spin-off issue: #370172: refactor block visibility

z.stolar’s picture

Cool. I'll use the patch at #370172: refactor block visibility#8 and add the necessary locale.module code.

z.stolar’s picture

Complete the patch at #8 in #370172: refactor block visibility, plus patch #8 in this issue with a patch to locale.module, to implement hook_block_load.
Blocks are now filtered out on each page, if they're not enabled for the current language, or ALL languages.

Please review.

catch’s picture

Status: Needs review » Needs work

The naming of the hook in the other issue is under dispute, but that doesn't make a lot of difference here. With the locale_block_visibility() you can just foreach ($result as $record) now that we're using dbtng/pdo - no need for db_fetch_array() which is on it's way out. I tried to draft a possibility for how this hook might look in http://drupal.org/files/issues/hook_block_visibility_0.patch - but that's untested and I think your approach is more readable.

Pasqualle’s picture

subscribe

sun’s picture

With translatable fields in core, this patch (resp. the approach taken) is in competition with #430886: Make all blocks fieldable entities

plach’s picture

Version: 7.x-dev » 8.x-dev

I am afraid this will have to wait for D8 :(

Gábor Hojtsy’s picture

Title: Multilingual support for Blocks » Add language options to block visibility
Issue tags: +D8MI

@sun: reading up the discussions here, it looks like this issue is more about adding language specific visibility to blocks, not making them translatable per say. That is if block visibility survives the Drupal 8 context stuff. Retitling and tagging for that.

sourcesoft’s picture

It's better this way than using ugly en/* in block visibility settings.

Artusamak’s picture

Assigned: Unassigned » Artusamak

Will try to have a shot at it.

Gábor Hojtsy’s picture

Yay! Yay!

Artusamak’s picture

Assigned: Artusamak » Unassigned
Status: Needs work » Needs review
FileSize
5.61 KB

Alright, here is the patch.

I created a new table dedicated for the visibility per language called block_language.
This table has the same structure as block_node_type and the same behavior. It adds a new visibility fieldset dedicated to the language.
The available languages are those enabled. If no language is selected, the block will always show.
The visibility alteration is done in the hook_block_list_alter and the documentation of this hook was actually implementing the block visibility per language (easy shot there).

I added this code in the language module. Not sure if it should go there or in locale. If i did wrong i'll update that.

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c21.patch, failed testing.

Gábor Hojtsy’s picture

Looked at the code. Looks generally great, just "minor" comments.

+++ b/core/modules/block/block.jsundefined
@@ -32,6 +32,17 @@ Drupal.behaviors.blockSettingsSummary = {
+    $('fieldset#edit-language', context).drupalSetSummary(function (context) {

+++ b/core/modules/language/language.installundefined
@@ -71,6 +71,33 @@ function language_schema() {
+      'language' => array(
+        'type' => 'varchar',
+        'length' => 32,
+        'not null' => TRUE,
+        'description' => "The machine-readable name of this language from {language}.langcode.",
...
+    'primary key' => array('module', 'delta', 'language'),
...
+      'language' => array('language'),

+++ b/core/modules/language/language.moduleundefined
@@ -211,3 +211,95 @@ function language_css_alter(&$css) {
+  $default_language_options = db_query("SELECT language FROM {block_language} WHERE module = :module AND delta = :delta", array(

All these places and more should use langcode, not language. We use langcode for language codes in D8.

+++ b/core/modules/language/language.moduleundefined
@@ -211,3 +211,95 @@ function language_css_alter(&$css) {
+    '#description' => t('Show this block only if the current language is of the given language(s). If you select no types, there will be no language-specific limitation.'),

Type? Looks like a copy-paste mistake.

+++ b/core/modules/language/language.moduleundefined
@@ -211,3 +211,95 @@ function language_css_alter(&$css) {
+    // Any module using this alter should inspect the data before changing it,
+    // to ensure it is what they expect.
+    if (!isset($block->theme) || !isset($block->status) || $block->theme != $theme_key || $block->status != 1) {
+      // This block was added by a contrib module, leave it in the list.
+      continue;

This looks very odd but I guess it is a standard code snippet that we need to live with.

Artusamak’s picture

Status: Needs work » Needs review
FileSize
6.85 KB

Comments are integrated. Question though: I didn't replace the "language" string in the strings displayed in the user interface, should it be replaced there too by "langcode" or is "langcode" a term that we don't want to expose to the end user because it's not meaningfull?

I fixed the test by creating the table in the pre-imported database of the language upgrade path test. Should i write new assertions? If yes is the following test scenario ok?
* Configure a block visibility
* Assert that the block is displayed in the configured language
* Assert that the block is not displayed in the configured language

Thanks for reviewing.

Artusamak’s picture

Here is the asked for screenshot:
Language visibility settings screenshot

nonsie’s picture

One more minor - add a period at the end of
Sets up display criteria for blocks based on language.

Artusamak’s picture

Following Gabor recommandation. Attaching 2 screenshots, the "default" state of the form and the "set" state.
Reduced the width of the screenshot too.
Language visibility settings screenshot default
Language visibility settings screenshot set

Artusamak’s picture

Assigned: Unassigned » Artusamak
Status: Needs review » Needs work

Gabor gave feedback in live.
I'm working on adding the hook_update_N() that is missing and the tests to detect that the table has been created and that the visibility setting is working.

Kristen Pol’s picture

I applied the patch and then had to start with a fresh database because there wasn't an update function yet... (I see that is being worked on ;)

The only issue I found was that when I created a block from scratch then the Languages fieldset/tab didn't show up. I had to save and edit the block for it to show up.

Artusamak’s picture

Upgrade path added.
Block creation visibility added.
Updagre path tests in progress.
Feature tests in progress.

dergachev’s picture

I decided to see how block.module handles the records in {block_role}, which are similar.

When a custom block is deleted, entries in block_langcode should be removed. See block_custom_block_delete_submit().
Same thing should happen when a language is deleted.

We should also consider implementing hook_module_uninstalled(), just as the block module does:


/**
 * Implements hook_modules_uninstalled().
 *
 * Cleans up {block} and {block_role} tables from modules' blocks.
 */
function block_modules_uninstalled($modules) {
  db_delete('block')
    ->condition('module', $modules, 'IN')
    ->execute();
  db_delete('block_role')
    ->condition('module', $modules, 'IN')
    ->execute();
}
dergachev’s picture

No time to roll a patch, but the following are also necessary:


/**
 * Implements hook_menu_delete().
 */
function language_menu_delete($menu) {
  db_delete('block_langcode')
    ->condition('module', 'menu')
    ->condition('delta', $menu['menu_name'])
    ->execute();
}
diff --git a/core/modules/language/language.module b/core/modules/language/language.module
index 20f7c62..d79c0a0 100644
--- a/core/modules/language/language.module
+++ b/core/modules/language/language.module
@@ -176,6 +176,11 @@ function language_delete($langcode) {
       variable_set('language_count', variable_get('language_count', 1) - 1);
     }

+    // Remove the orphan entries from the block_langcode table
+    db_delete('block_langcode')
+      ->condition('langcode', $langcode)
+      ->execute();
+
     drupal_static_reset('language_list');

     $t_args = array('%language' => $language->name, '%langcode' => $language->langcode);
@@ -211,3 +216,137 @@ function language_css_alter(&$css) {
     }
   }
 }
saltednut’s picture

Status: Needs work » Needs review
FileSize
41.47 KB

Currently we are not removing orphan entries from node... discussed with Gabor the orphan cleanup was deemed unnecessary.

Patch attached for #31 - block_langcode cleaned up when modules are uninstalled.

saltednut’s picture

Blocks are showing/hiding around the site in normal pages and regions.

However, on the dashboard, a disabled block is still appearing. The content of the disabled block is 'empty' but the title of the block is still rendering.

This behavior seems inappropriate. See attached screenshot:
Disabled dashboard blocks showing.

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c32.patch, failed testing.

saltednut’s picture

Status: Needs work » Needs review
FileSize
43.38 KB

Forgot to remove some local configuration from 135464-language_visibility-c32.patch.

This should be corrected in the attached patch. Also made adjustments to dashboard.module to accomodate for blocks determined disabled (set empty) for langcodes visibility.

saltednut’s picture

ignore last patch - contained unrelated code for this issue. (apologies!)

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c35.patch, failed testing.

saltednut’s picture

Making changes to dashboard.module seems a bridge to far for this issue. I suggest moving the issues with dashboard not supporting 'block_langcode' table to a different thread.

@evolvingweb: Attached is the patch accommodating comment #31 - clean up {block_langcode} for modules uninstalled.

saltednut’s picture

Assigned: Artusamak » saltednut
Status: Needs work » Needs review
dergachev’s picture

Just spoke with Brant and we agree that the dashboard behaviour he was concerned about in comment #34 is actually "by design", and it handles the language visibility setting fine. The dashboard module purposely renders "empty" blocks to the user, probably so they can administer. There's another issue [http://drupal.org/node/1009872] that's currently tracking this.

dergachev’s picture

Sun's feedback:

Description is too long, too verbose.
Hide the tab if there's only 1 language. (in form_alter, check language_multilingual)
Form alter the "add new block" form too (present in #30)
rename table from block_langcode to block_language
write test for language_multilingual

saltednut’s picture

This patch is based on some of the work from patch c30 minus the tests. There is also a test from c24 that is not included.

Due to talks with Gabor and sun it was decided to call this table block_language instead of block_langcode.

Table renamed to block_language
Upgrade path added (unchanged from c30)
Block creation visibility added (implemented using less code thanks to sun & evolvingweb)

YesCT’s picture

FileSize
68.57 KB

Edit: was looking at the screen shots in comment 27.

I was looking for a similar pattern of selecting no check boxes means selecting all and thought we could reuse the same language.

I suggest something like what is used in the vertical tabs roles visibility settings page.

Show this block only for the selected role(s). If you select no roles, the block will be visible to all users.

I'll make a new patch with those words.

YesCT’s picture

same patch as in comment 43 but changes

    '#description' => t('Show this block only if the current language is of the given language(s). If you select no language, there will be no language-specific limitation.'),

to

+    '#description' => t('Show this block only for the selected language(s). If you select no languages, the block will be visible to all users.'),

other than that, the ui looks good to me. it might be interesting to see what it looks like with a lot of languages.

YesCT’s picture

I took out spaces trailing at the end of lines, changed tabs to spaces and also reformatted indent pattern on some db_delete to match some other db_delete's I found elsewhere in core.

  db_delete('block_language')
    ->condition('module', $modules, 'IN')
    ->execute();
Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/language.installundefined
@@ -101,3 +128,39 @@ function language_update_8000() {
+/**
+ * Create the {block_language} table to store the langcode visibility settings
+ * per block.
+ */
+function language_update_8001() {

phpdoc should be shorter and on one line.

YesCT’s picture

Status: Needs work » Needs review
FileSize
197 bytes
7.78 KB

phpdoc is one line and changed Create to Creates per http://drupal.org/node/1354#functions

line is now:

 * Creates the {block_language} table for langcode visibility settings per block.

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c48.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.78 KB

I think this one applies now.

saltednut’s picture

1. At this point we need tests written for a) upgrade path and b) add/edit block screens. Let me know if I am missing anything, Gabor.

2. A discussion was brought up by sun at Denver about the wording. The redundancy between the Title and Description "Show this block for specific languages" and "Show this block only if..." only adds to verboseness and does not add to usability. This is a problem inherited from the wording on the Roles tab. We should consider punting that format and coming up with new wording that is clear and concise.

Artusamak’s picture

Assigned: saltednut » Unassigned
FileSize
12.47 KB

Oh some activity happened here since friday :)
I finally managed to fix the simpletest, you'll find in the attached patch a test for the upgrade and a test for the visibility.

Question: In the test i'm generating a custom block, for the block body when i'm using a random string it happens to have invalid markup, causing the test to fail. Should i create the body of the block with a filter or do you have any other suggestion to implement that?

About the verboseness, why not dropping the first part of the sentence?

Show this block only for the selected language(s). If you select no languages, the block will be visible to all users.

Would become.

If you select no languages, the block will be visible to all users.

With the title it should be enough.

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c52.patch, failed testing.

tstoeckler’s picture

One minor thing: It seems weird that all the form alters are in language.module, but we add JS to block.js and not language.js. I'm not JS-savy enough to judge whether it is actually possible to extend vertical tabs like that from the outside, but I thought I'd mention it.

Artusamak’s picture

Status: Needs work » Needs review
FileSize
12.52 KB

Rewritting the body generation.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

I tested the patch in #55 and it works as expected. Specifically, I tested as follows:

1) added Burmese
2) added new block
3) restrict block to Burmese
4) restrict block to Burmese and English
5) restrict block to English
6) removed all language restrictions

and all scenarios worked. I also quickly scanned the code and nothing jumped out at me as needing to be changed.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/language.install
@@ -101,3 +128,37 @@ function language_update_8000() {
+    'primary key' => array('module', 'delta', 'langcode'),
+    'indexes' => array(
+      'langcode' => array('langcode'),
+    ),

1) 'langcode' never seems to be used as query condition or sort. Thus, 'langcode' unnecessarily increases the size of the primary key.

2) If we want to make sure that no duplicate rows exist at a database level, then we could add a unique index across the three columns. However, I'm not sure whether that is really needed.

3) We could leave the additional index for 'langcode', in order to speed up delete queries for the case when a language is deleted.

Apparently, this patch does not seem to contain code that deletes all langcode records when the corresponding language is deleted from the system. (There's only code for deleting corresponding module records when modules are uninstalled.)

4) These adjustments need to be done in the update function and in the schema function.

+++ b/core/modules/language/language.module
@@ -211,3 +222,115 @@ function language_css_alter(&$css) {
+  if ($default_langcode_options === FALSE) {
+    $default_langcode_options = array();
+  }

I looked at the ->fetchCol() implementation once again, and apparently, it always returns an array. Thus, this entire condition is superfluous.

+++ b/core/modules/language/language.module
@@ -211,3 +222,115 @@ function language_css_alter(&$css) {
+    $langcodes_options[$language->langcode] = t($language->name);

@gabor: Is the t() valid here?

+++ b/core/modules/language/language.module
@@ -211,3 +222,115 @@ function language_css_alter(&$css) {
+    '#title' => t('Show block for specific languages'),
...
+    '#description' => t('Show this block only for the selected language(s). If you select no languages, the block will be visible to all users.'),

The description still duplicates the title/label. Let's merge the first sentence into the label:

"Show this block only for specific languages"

...and remove the first sentence from the description.

Regarding the second sentence, I'm not fully up to speed with regard to whether we're favoring direct speech ("you") or not. That said, the last part is definitely wrong ("users" vs. "languages").

I'd shorten the description to:

"Select none to show this block in all languages."

+++ b/core/modules/language/language.module
@@ -211,3 +222,115 @@ function language_css_alter(&$css) {
+function language_block_list_alter(&$blocks) {
+  global $language_interface, $theme_key;
...
+    if (!isset($block_langcodes[$block->module][$block->delta][$language_interface->langcode])) {

I'm not sure whether $language_interface is the correct language type/scope to filter by. Blocks contain content, so I'd rather expect them to be filtered by the current $language_content type/scope.

+++ b/core/modules/language/language.test
@@ -179,3 +179,80 @@ class LanguageListTest extends DrupalWebTestCase {
+class LanguageBlockVisibilityTest extends DrupalWebTestCase {

The tests do not seem to assert whether {block_language} records are properly deleted when

1) the providing module of a block is uninstalled

2) a language is deleted

+++ b/core/modules/language/language.test
@@ -179,3 +179,80 @@ class LanguageListTest extends DrupalWebTestCase {
+      'name' => 'Language block visibility configuration',

"configuration" can be removed from the label/name.

+++ b/core/modules/language/language.test
@@ -179,3 +179,80 @@ class LanguageListTest extends DrupalWebTestCase {
+  function setUp() {
+    parent::setUp('language', 'locale', 'block');
+  }

Do we actually need Locale module for this test?

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.test
@@ -150,4 +150,29 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+  public function testLanguageBlockVisibilityUpgrade() {

I'm not sure whether and why this upgrade test is needed. D7 did not contain a language visibility condition, and the module update does not attempt to migrate any data either (because there is nothing to upgrade), so this upgrade test does not really test anything other than bare essential block module functionality.

So unless I'm missing something big, this upgrade test can be removed.

Artusamak’s picture

Assigned: Unassigned » Artusamak
Status: Needs work » Needs review

OK, feedbacks taken into account, thanks for that.
I'll will discuss the t() implementation with Gabor tomorrow during the D8MI meeting and post the updated patch afterward.

Artusamak’s picture

Status: Needs review » Needs work

Still need work.

Gábor Hojtsy’s picture

@sun:

- We discussed deletion behavior in great detail at the sprint. When a language is removed, your nodes in that language or your users in that language are not removed. Neither they are made neutralized. This is likely legacy behavior. We might or might not want to remove the information about relation to removed languages. Sames goes for language relation of blocks. This is thankfully not the main block data itself (unlike nodes or users), so it is much more lightweight to remove them. If you believe we should, we can put it in there. It is going to be inconsistent with how we keep language information other places when a language is removed, but that whole thing is inconsistent with the rest of Drupal.

- t($language->name) should not be good anymore. CMI translation will need to be used when needed, once available.

- On the description for visibility, I believe YesCT unified that with the roles descriptions (see http://drupal.org/node/135464#comment-5780188), so I think as long as we use the same pattern here, it should be a common approach; if we deviate from that, it will look odd/nonstandard IMHO.

- Otherwise agreed.

Artusamak’s picture

Assigned: Artusamak » Unassigned
Status: Needs work » Needs review
FileSize
10.58 KB

New patch applying feedbacks described above.

I removed the t() and added a TODO to note that it needs work related to CMI.
About the description, i kept the one from YesCT but remove the first part used in the field title. It makes more sense to avoid this redundancy. Is it ok?

@Sun we need locale to be able to have the drupalGet() assertion working properly with the language option. (Otherwise the language is not changing).

Ready for new feedback.

Kristen Pol’s picture

I applied the patch without errors. Then, I tested the same scenarios as in #56 with no obvious errors (from the UI). But, this time I looked at the block_languages table while I was doing the configuration and found that the block_languages data wasn't removed when:

  1. deleting the block
  2. deleting the language

It is not clear based on #60 whether this functionality must be in there (since legacy language data persists for deleted nodes). Perhaps @sun needs to comment on that?

If the block_language data doesn't need to be deleted when deleting the block or language, then this is RTBC in terms of functionality. Though, someone might want to check through the code.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Right, looks like based on reviews, we want to remove language vs. block info after all when blocks are removed or languages are removed.

Artusamak’s picture

Status: Needs work » Needs review
FileSize
15.99 KB

Alright, i added the code to clean the db entries when a language is deleted and when a custom block is deleted.

Kristen Pol’s picture

I repeated the tests from #62 and everything worked as expected. Everything that worked previously still worked and now the appropriate records from the block_language table are removed when the block or language is deleted.

I won't mark RTBC because the code needs to be reviewed by a core person... I did scan the code quickly and noticed quite a few lines exceed 80 characters.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I've been thinking about this problem of the JS being in block module and the rest being in language module. We have seen similar problems before with node module, comment module, etc. In Drupal 8, we moved language handling entirely to the respective modules instead of locale/language module handling it for them. So with that in mind it seems logical to put this functionality entirely in block module. Related issues for supporting this direction:

#1236680: Move path language settings from Locale to Path module
#540294: Move node language settings from Locale to Node module
#1415764: Untangle comment module and locale module

etc. It is likely going to be much less code but more intertwined with the existing block module code. That is in line with how low level / built in we consider language support should be in Drupal 8.

YesCT’s picture

I would suggest keeping wording similar to roles settings page. Or, if we think a new pattern is needed, to change it in both (and other) places. If we come up with better wording pattern here, I suggest opening an other issue to see if it can improve the roles settings page too.

Artusamak’s picture

Assigned: Unassigned » Artusamak

OK will work on it.

Artusamak’s picture

Assigned: Artusamak » Unassigned
Status: Needs work » Needs review
FileSize
18.4 KB

The attached patch is moved the feature implementation into the block module. I let the tests in the language module, i believe it makes more sense to have them there because it's tight to the language module.
Let me know if i have to change that.
The patch is also redefining the description on the same pattern as roles use it.

I hope it will make it through this time! :)

Status: Needs review » Needs work

The last submitted patch, 135464-language_visibility-c69.patch, failed testing.

Artusamak’s picture

Status: Needs work » Needs review
FileSize
18.23 KB

Reformatting the patch, also rebasing with an up to date 8.x.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I think this looks pretty good. Just two notes:

+++ b/core/modules/block/block.moduleundefined
@@ -776,7 +776,7 @@ function _block_load_blocks() {
 function block_block_list_alter(&$blocks) {
-  global $user, $theme_key;
+  global $user, $language_content, $theme_key;
 

@@ -854,6 +861,19 @@ function block_block_list_alter(&$blocks) {
+    // This block should not be displayed with the active language, remove
+    // from the list.
+    if (!isset($block_langcodes[$block->module][$block->delta][$language_content->langcode])) {
+      unset($blocks[$key]);
+      continue;
     }

I think this could be topic of debate, but I'd consider blocks to be interface and should therefore use the interface language.

+++ b/core/modules/block/block.moduleundefined
@@ -1021,7 +1041,8 @@ function block_admin_paths() {
+ * Cleans up {block}, {block_role} and {block_language} tables
+ * from modules' blocks.
  */

+++ b/core/modules/language/language.testundefined
@@ -179,3 +179,192 @@ class LanguageListTest extends DrupalWebTestCase {
+   * Tests if the visibility settings are clean from the database if the
+   * language is removed.
...
+   * Tests if the visibility settings are clean from the database
+   * if the block is deleted.

All functions/methods should have one line summaries. Good twitter skills come in handy here, except you are mostly not allowed to abbreviate or make grammar mistakes :)

@Kristen: want to take this for a test drive too? :)

sun’s picture

heh, quoting myself from #57:

I'm not sure whether $language_interface is the correct language type/scope to filter by. Blocks contain content, so I'd rather expect them to be filtered by the current $language_content type/scope.

In the end, I guess that you'd potentially want to be able to filter by one of both (or even both) in advanced use-cases. The new block plugins might even allow for that. But until then, we need to decide what's more appropriate for this simple implementation.

For built-in Drupal core functionality, I don't think it actually matters, since AFAIK, there's only one language negotiation configuration, which is applied to the interface and content language in one shot, so they are the same in most cases.

@plach: Could you make the final call on this?

Gábor Hojtsy’s picture

@sun: the blocks for account login or registration or menus is translated by the interface language, so that is why I assumed that would be more applicable. This is easily changed either way, so I can agree with content language if that is what people prefer.

plach’s picture

In the end, I guess that you'd potentially want to be able to filter by one of both (or even both) in advanced use-cases.

I agree. Not sure about the UX implications, but what about adding an element allowing to choose among all the configurable language types? This way if we make content language configurable, which is likely to happen at some point in D8, we are ready to support this use case.

andypost’s picture

I have another thoughts:
1) each block is unique by it's application usage so there's should be difference in config related to content or interface language.
2) this functionality makes sense only on multilingual context so having this code in block.module is wrong (#10 explains deeper) also WSCCI introduced context seems more suitable for this purpose
In #66 Gabor explains that we should have this code in block but having another table once most of sites are single language is nonsense

Suppose this issue should have more focus on UX implications first

Gábor Hojtsy’s picture

@andypost: yes, this code only makes sense in multilingual context. Node, user and path module also has lots of code which is only applicable there. The concept for Drupal 8 is that just like contrib modules, core modules would need to implement this on their own instead of implementing most of it on their own and then some injected from the outside, like it was for node, user and path in Drupal 7. Since they need to do their data handling, list filtering, etc. for themselves, it makes sense to have this code with them.

Related: we have a host of issues at #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) where the current plan is to have multiples of some core tables for translation purposes.

Kristen Pol’s picture

I'm just looking at this (was Spring break last week :)... do you still need testing for #71 or should I wait on any final tweaks based on the last few comments?

Gábor Hojtsy’s picture

I think if we want to introduce the language type selector, it should be a dropdown or radio button group above the language list I assume. Then how would we store that data I'm not 100% sure. On each record of the block language table? I assume the UI should not support combined filtering by multiple languages at once... I feel this could easily complicate this too much (ie. delay this getting in for longer, while we still have a great opportunity ATM to get it in).

Artusamak’s picture

Assigned: Unassigned » Artusamak

Just to keep a trace of what is discussed quoting the D8MI IRC meeting.

Artusamak: looks like each block_language row would have a language type column too and you'd have one select box or radio group on the block selection if you have multiple configurable language types to select from them
Artusamak: there should always be at least one configurable type in which case that should be used and no selection UI presented
Artusamak: http://api.drupal.org/api/drupal/core%21includes%21language.inc/function...

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
20.6 KB
9.08 KB
91.31 KB

Adding language type support:

1. New type column in block_language.
2. Found out that you could not tie a block to multiple languages at once, because the primary key on the table did not allow for that. Expanded primary key.
3. While the DB (and the block visibility logic that I've updated) supports multiple type-language combos, the UI is built to allow for one language type selection for all the languages for a block. Much simpler UI.
4. The type selector is hidden if there is only one language type configurable (which is the only thing that can happen in core at this stage btw, there is no core way to make more types configurable so far).

So this ties the block to the only configurable language type if there is one and offers a selection if there are multiples. UI did not change for the core case. If there are more language types (again, does not happen in core at all now), then you get this UI:

BlockLanguage.png

Interdiff attached for your reviewing pleasure. @all: please review!

Gábor Hojtsy’s picture

Left a little bitty test code in that patch :) This line removed now:

      $configurable_language_types = language_types_get_configurable();
-    $configurable_language_types[] = LANGUAGE_TYPE_CONTENT;

:) This was used to create the screenshot btw :)

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.admin.inc
@@ -400,6 +400,78 @@ function block_admin_configure($form, &$form_state, $module, $delta) {
+    foreach($existing_language_settings as $setting) {

Missing space after foreach.

+++ b/core/modules/block/block.admin.inc
@@ -400,6 +400,78 @@ function block_admin_configure($form, &$form_state, $module, $delta) {
+    $default_language_type = db_query("SELECT type FROM {block_language} WHERE module = :module AND delta = :delta", array(
+      ':module' => $form['module']['#value'],
+      ':delta' => $form['delta']['#value'],
+    ))->fetchField();

This additional query looks bogus and like obsolete debugging code; the result overwrites $default_language_type.

+++ b/core/modules/block/block.admin.inc
@@ -400,6 +400,78 @@ function block_admin_configure($form, &$form_state, $module, $delta) {
+      // Only one configurable langugae type, save with that.

Typo in language.

+++ b/core/modules/block/block.module
@@ -854,7 +861,23 @@ function block_block_list_alter(&$blocks) {
+    // Language visibility settings.
+    // No language setting for this block, leave it in the list.
+    if (!isset($block_langcodes[$block->module][$block->delta])) {
+      continue;
+    }
+    foreach ($block_langcodes[$block->module][$block->delta] as $language_type => $langcodes) {
+      if (isset($langcodes[$GLOBALS[$language_type]->langcode])) {
+        // Found a language type - langcode combination in the configuration
+        // that is applicable to the current request.
+        continue 2;
+      }
     }
+    // Had language configuration but none matched.
+    unset($blocks[$key]);

It's not really ideal that this code block cannot be freely moved within the context/visibility filtering code, because it skips to the next item on positive match (so any additional filtering code behind it would not run).

However, since most of this code will likely be refactored for the new D8 block system either way, I guess we can leave it that way.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
20.3 KB

Fixed the typos and leftover debugging code. I agree the logic not possibly freely moved around is probably very temporary. Any other concerns?

sun’s picture

Status: Needs review » Needs work

Sorry, forgot one issue. RTBC after fixing this:

+++ b/core/modules/block/block.admin.inc
@@ -400,6 +400,72 @@ function block_admin_configure($form, &$form_state, $module, $delta) {
+    if (count($language_type_options) > 1) {
+      // More than one configurable language type.
+      $form['visibility']['language']['language_type'] = array(
+        '#type' => 'radios',
+        '#title' => t('Language type'),
+        '#options' => $language_type_options,
+        '#default_value' => $default_language_type,
+      );
+    }
+    else {
+      // Only one configurable language type, save with that.
+      $form['visibility']['language']['language_type'] = array(
+        '#type' => 'value',
+        '#value' => $configurable_language_types[0],
+      );
+    }

The proper way to do this is:

  '#access' => count($language_type_options) > 1,

on the otherwise always identical #type radios form element. That makes sure that other modules do not have to use complex conditional code when trying to enhance or alter the form.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
20.01 KB

I definitely had that at one point but found that it would complicate the submission workflow. Let's see, just changed the form.

Gábor Hojtsy’s picture

Ok, I must have had some other issue when working on it back then :) Also fixed the two phpdoc issues I've noted in #72 above, so I don't have any more outstanding issues to fix I believe :) Good? :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Great feature!

This patch needs a quick re-roll.

Also, do we want to update CHANGELOG.txt?

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
20.51 KB

Rerolled. Added short changelog entry.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

catch’s picture

Status: Fixed » Needs work

Why is all the code added to block module, but the tests added to language module?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.18 KB

You are right. Moved the tests to block module. Changed group to 'Block' to appear there and removed locale module from list of modules enabled for the test. There is no use/need of locale module there, its about UI translation only now (except those pesky date formats). Passed for me, so should be directly RTBC if passes. It is just a logical copy-paste.

Title: Add language options to block visibility » pink beats by dr dre
Version: 8.x-dev » 7.9
Status: Needs review » Needs work

The last submitted patch, move-block-test-to-block-module.patch, failed testing.

Gábor Hojtsy’s picture

Title: pink beats by dr dre » Add language options to block visibility
Version: 7.9 » 8.x-dev

Restoring spam changes.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Trivial.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/block.test
@@ -844,3 +844,189 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+class LanguageBlockVisibilityTest extends DrupalWebTestCase {

Within Block module, this test case needs to be in the Block namespace; e.g.:

BlockLanguageTestCase

(Yes, we're not fully consistent with that and there are a couple of bad test case names throughout core, but those need to be fixed and don't serve as examples ;))

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.17 KB

Did exactly that. :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -D8MI, -language-config

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