In #1236680: Move path language settings from Locale to Path module, there is a patch that includes the following code. Gábor Hojtsy suggests that creating a "language_select" element type would be a good idea to avoid repetition of similar code.

  // A hidden value unless locale module is enabled.
  if (module_exists('locale')) {
    $form['language'] = array(
      '#type' => 'select',
      '#title' => t('Language'),
      '#options' => array(LANGUAGE_NONE => t('All languages')) + locale_language_list('name'),
      '#default_value' => $path['language'],
      '#weight' => -10,
      '#description' => t('A path alias set for a specific language will always be used when displaying this page in that language, and takes precedence over path aliases set for <em>All languages</em>.'),
    );
  }
  else {
    $form['language'] = array(
      '#type' => 'value',
      '#value' => $path['language']
    );
  }

I've included a patch that adds this. I'm totally lost as to how to properly organize this, so advice would be greatly appreciated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

I think form_process_password_confirm() would be a good pattern to follow. Basically for a language selector I'd allow #title, #description, #required and #default_value to be overridden. Would we need anything else? And then the _process function would either create the select field or a value field in place of the element. At least that would be my idea.

One more thing to support that is that IMHO hook_element_info() is cached, so if you turn locale on/off, the element might not update proper. So I'd better do it on the fly in the process function. Also, it would be vital to convert the path module code to this as a test (the existing simpletests will verify if it worked :). I think here you can assume the other patch is not yet committed, so the code is still in locale module just to speed up your work.

Thanks for working on this!

Gábor Hojtsy’s picture

Title: Create language_select element type » New language_select element type for form API

Retitle.

sun’s picture

Status: Needs review » Needs work
+++ b/includes/form.inc
@@ -2533,6 +2533,19 @@ function form_process_select($element) {
+  if (module_exists('locale')) {
+    $element['#options'] = array(LANGUAGE_NONE => t('All languages')) + locale_language_list('name');

1) I don't think we need a dependency on Locale module to generate this #options list.

2) LANGUAGE_NONE should be #empty_value

3) - Any - should be #empty_option

+++ b/modules/system/system.module
@@ -452,6 +452,22 @@ function system_element_info() {
+  if (module_exists('locale')) {
+    $types['language_select'] = array(
...
+    );
+  }
+  else {
+    $types['language_select'] = array(
+      '#input' => TRUE,
+      '#title' => t('Language'),
+    );
+  }

We cannot conditionally/dynamically define form/render elements in system.module.

Or at least, that's a direction I cannot support in any way.

That said, I'm quite OK with the element name, since one of our ultimate goals is to introduce a (required) language.module in core for D8.

12 days to next Drupal core point release.

Gábor Hojtsy’s picture

@sun: hm, well, for now if locale module is not enabled there is no language list, so that is the case, where it falls back on a 'value' #type, that is what path module does and node module does to keep the existing value at least. Once/if we have language module, and if its required and we basically consider that just there all the time, we'll have this element have it all the time. At that point language module could define this element and it might never be #value type.

The motivation for doing this now is that @catch and you were not too happy with the conditionals on locale() in path and node module patches. So we discussed this option on IRC and now exploring the implementation to unify this and move the ugly part out of the modules :)

Gábor Hojtsy’s picture

More feedback from IRC:

[12:43am] sun: GaborHojtsy: euhm, but "the" language list is defined in includes/standard.inc ?
[12:43am] GaborHojtsy: sun: well, that is not the list of languages you can choose from in path module or node module though
[12:44am] GaborHojtsy: sun: http://drupal.org/node/1260860 is where your languages are defined
[12:44am] Druplicon: http://drupal.org/node/1260860 => Rework language list admin user interface => Drupal core, language system, normal, needs work, 23 comments, 5 IRC mentions
[12:44am] sun: GaborHojtsy: but yes, this cries for a decision whether this element pertains to Locale/Language + active configuration only, or whether it's going to use an (optional) "map-reduce" kind of processing -- i.e., have all, and if you want to, limit to only available
[12:45am] GaborHojtsy: sun: then we'd be back with the conditional locale() logic in the path/node modules, which you wanted to avoid with catch
[12:45am] sun: GaborHojtsy: just a #process
[12:46am] GaborHojtsy: sun: ?
[12:47am] sun: GaborHojtsy: have a default #process (similar to the patch) that injects all possible #options, and - if you opt-in via #only_enabled => TRUE - have a second #process that limits #options to configured/enabled languages only
[12:48am] sun: ...or bake that logic into the the first/only #process, I don't care
[12:48am] GaborHojtsy: sun: yeah, maybe…. now that you say this, we should also drop all items from standard.inc for which l.d.o does not have stuff, so we at least don't have 3 different language lists then in Drupal
[12:48am] GaborHojtsy: sun: I think one process is fine
[12:48am] sun: but obviously, the default #process is able to work without Locale module, only the second can work with Locale module
[12:49am] GaborHojtsy: sun: would be great to figure out how to generalize things like (a) I want an "All languages option" (b) I want a "No language" or "Language neutral" option instead
[12:50am] GaborHojtsy: sun: the more use cases we want to cover, the more complex this gets, and we might lose the code reuse benefits IMHO
[12:50am] sun: GaborHojtsy: that's application-level logic already, but yes, also agreed on that
[12:50am] GaborHojtsy: sun: e.g. take in #options as is and add the list of languages to that?
[12:50am] GaborHojtsy: sun: so people can specify their All, No, Neutral, etc. in #options first
[12:51am] sun: GaborHojtsy: mmm, nope, overloading/multi-purposing properties is bad
[12:51am] sun: GaborHojtsy: but yes, we could have one or more custom #properties that decide on what kind of #options are going to be loaded
[12:52am] sun: GaborHojtsy: overriding the default #empty_option => '- None -' with #empty_option => 'Language neutral' should not be an issue here though
[12:53am] sun: that's 1 line to heaven
[12:53am] GaborHojtsy: sun: hm, that sounds good

tarmstrong’s picture

This needs more work, but here's a work-in-progress. Is this along the lines you were thinking?

Gábor Hojtsy’s picture

Well, now if you put this to path module like that, you'll see this is *totally* different to how it works now. It would get you a list of languages in any case, even if locale is not enabled, so you could (attempt to) save paths in all kinds of funky languages that are otherwise not available on your site. That is why I did not suggest/want to have the overall language list tainting this element :) @sun: since this was your proposal, how would you make this work for that scenario and be simple at that, which we do assume for DX.

tarmstrong’s picture

@Gábor Hojtsy: The path module example could be written:

$form['language'] = array(
  '#type' => 'language_select',
  '#only_enabled' => TRUE,
);

If locale is enabled, I get English/French.

If locale is disabled, I get a value element -- not the whole language list.

So this is the same as the logic in this patch (http://drupal.org/files/issues/decouple-path-and-locale_0.patch), isn't it?

tarmstrong’s picture

Oops, duplicated my previous comment. Sorry!

Gábor Hojtsy’s picture

All right, let's convert the path module element at least in the patch, that should help us see if tests are passing OK, which would help validate the solution then :) Looking the same for node module would also be good.

tarmstrong’s picture

Updated patch with the path.module code using the new element.

tarmstrong’s picture

Status: Needs work » Needs review

Changing the status for testbot.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Well, without #1236680: Move path language settings from Locale to Path module being committed, this would need to change the locale module code seems like, or we better integrate the two issues. This patch in itself will not be good since it leaves the locale module override intact (does not really have an effect in itself).

catch’s picture

Subscribing.

tarmstrong’s picture

This patch takes #13 into consideration, but I'm not happy with it. Sure, the tests pass, but I don't think the options I have, or the resulting behaviour is very clear. Any ideas on how to improve it would be welcome :)

Gábor Hojtsy’s picture

You are applying this to the locale addition form, which is interesting, and brings up good use cases for this (ie. what to do if you want the predefined list and if locale is enabled). For the path module itself, getting rid of the form alter in locale module would be vital in the patch.

tarmstrong’s picture

Finally got the tests passing! I left the node_form alter in because it does more complicated logic that we wouldn't want to do in our #process hook.

Gábor Hojtsy’s picture

#1236680: Move path language settings from Locale to Path module got committed, so let's get this going with that in mind! Getting closer! Thanks for all you work.

catch’s picture

+  if (!$only_enabled) {
+    if (module_exists('locale') && $use_predefined_languages) {
+      $options = _locale_prepare_predefined_list();
+      $element['#default_value'] = key($options);
+    }
+    else {
+      include_once DRUPAL_ROOT . '/includes/standard.inc';
+      $standard_languages = standard_language_list();
+      $get_first = function($value) {
+        return $value[0];
+      };
+      $options = array_map($get_first, $standard_languages);
+      $element['#empty_value'] = LANGUAGE_NONE;
+      $element['#empty_option'] = t('Language neutral');
+    }

I don't understand why this code is being added to form.inc. Can't locale module extend this element? What's the standard install profile doing here?

Gábor Hojtsy’s picture

@catch: standard_* is not (necessarily) the standard profile. Look into #1231402: Drupal does not use ISO language codes, iso.inc is misleading to debate that further. On extending it from locale, I'm not sure what's possible there, we basically need a select field or a value field in form API, which are not really extensions of each other, are they? Also, the built-in language list does not require locale module at all (and @sun requested above to have the built-in language list there too). So it would be 2/3rds in form.inc and 1/3rd in locale module? How do you envision that extension?

catch’s picture

Oh I missed the iso.inc/standard.inc change, that makes a bit more sense!

So for the locale module bit. What about #options_callback and then locale module can provide one, and when it uses the element set that value on the element?

Gábor Hojtsy’s picture

@catch: #options_callback is not yet a known property, right (I've grepped but did not find it). How would that work? Why its called an options callback if it would change the value of the field in cases? Maybe I'm misunderstanding this, but its not too clear to me.

catch’s picture

No it's not a known property and I don't claim that's a particularly good idea in itself, but I think we can find something better than a hard coded switch on an optional core module for this, we need less coupling in core, not more.

tarmstrong’s picture

I'm new here, so this may be a silly idea, but would a 'hook_form_language_select_alter' make sense?

Gábor Hojtsy’s picture

@catch, @tarmstrong: well, we can use http://api.drupal.org/api/drupal/modules--system--system.api.php/functio... to alter the element if locale module is enabled and use a different #process callback. That callback would need to repeat some of the form.inc code, but at least we can get the form.inc code rid of locale.module (this part soon to be language.module) stuff. What about that?

That is, if we want to have it a form element still with one-off properties like #use_predefined_languages. I think the main problem is that we want to have a centralized element, but there are many options that might be provided. Such as some places we want to have the full predefined language list (in locale module for example), then depending on configuration, we don't want that in node and path alias language assignment (we might want the default language + language independent if locale is not enabled or even just language independent), then when languages are enabled, we want that language list at places, but still keep the predefined language list at other places. Maybe we can unify these in an #options_type, where it can be 'predefined' or 'enabled', since that seem to be the two unique options list types for now. If its enabled, then locale module is used if available or en empty element is provided if not. If its predefined, then the whole dance is about trying to produce a langauge list even if locale's helper function is not there. We can move that helper function elsewhere to be available at all times or avoid using it, once we standardize on this form element (since we'll not need it, it is just for forms).

Makes sense?

catch’s picture

Using hook_element_info_alter() sounds good to me, and some kind of switch on the options callback/#options_type seems fine also.

tarmstrong’s picture

Sounds good to me. I'll try to get a patch up early next week.

Gábor Hojtsy’s picture

@tarmstrong: perfect! Thanks for your continued work here :)

fastangel’s picture

Hi,

I have begun to work to make the patch with the hook_element_info_alter. However I tried applying the patch in #17 but fails :(. Any ideas?. If not try to adapt again.

Gábor Hojtsy’s picture

@fastangel: yes, the patch was rolled before all core files were moved to a core/* directory. You can just copy the patch to the core directory under your checkout and try to apply it there. You should have much better success :)

fastangel’s picture

I tried. But it still fails (Especially with locale.admin.inc). I try to fix it and send a new patch.

fastangel’s picture

Status: Needs work » Needs review
FileSize
6.46 KB

Hi,

Attached new patch with hook_element_info_alter and adaptation of patch #17 to code last checkout.

Status: Needs review » Needs work

The last submitted patch, language_select_element_type-1280996-18.patch, failed testing.

Gábor Hojtsy’s picture

Looks like the predefined_langcode field does not really work as intended, it does not have a 'custom' field. That should be used for empty_option and empty_value there AFAIS.

fastangel’s picture

Hi,

Attached new patch. This patch add empty_option and empty_value. Only one problem. The value custom appears first. Therefore the description of custom field is incorrect. I change this description?

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Well, it would need to be the last item for usability. (Marking needs review for testbot).

Status: Needs review » Needs work

The last submitted patch, language_select_element_type-1280996-19.patch, failed testing.

sun’s picture

+++ b/core/includes/form.inc
@@ -2553,6 +2553,38 @@ function form_process_select($element) {
+function form_process_language_select($element) {
...
+    $element['#theme'] = 'select';
+    $element['#theme_wrappers'] = array('form_element');
+  }
+  else {
+    $element['#theme_wrappers'] = NULL;
+    $element['#process'] = array('form_process_language_select');

+++ b/core/modules/locale/locale.module
@@ -607,9 +607,49 @@ function locale_modules_disabled($modules) {
+function form_process_language_select_locale($element) {
...
+    $element['#theme'] = 'select';
+    $element['#theme_wrappers'] = array('form_element');
+  }
+  else {
...
+    $element['#theme'] = 'select';
+    $element['#theme_wrappers'] = array('form_element');

I do not understand why these #process functions contain so much duplicated code, and why most of the properties aren't defined as defaults in hook_element_info() instead.

Also, it doesn't really make sense to set or adjust #process within a #process callback ;)

+++ b/core/includes/form.inc
@@ -2553,6 +2553,38 @@ function form_process_select($element) {
+  $only_enabled = isset($element['#only_enabled']) ? $element['#only_enabled'] : FALSE;

+++ b/core/modules/locale/locale.admin.inc
@@ -193,13 +193,15 @@ function locale_language_overview_form_submit($form, &$form_state) {
+    '#only_enabled' => FALSE,

#only_enabled defaults to FALSE on the element type definition. Hence, this isset() is superfluous. It is valid to expect and rely on default element type properties to exist. Code that unsets them is broken code.

+++ b/core/includes/form.inc
@@ -2553,6 +2553,38 @@ function form_process_select($element) {
+  $use_predefined_languages = isset($element['#use_predefined_languages']) ? $element['#use_predefined_languages'] : FALSE;

Either I'm blind or $use_predefined_languages isn't used anywhere here.

+++ b/core/includes/form.inc
@@ -2553,6 +2553,38 @@ function form_process_select($element) {
+    $standard_languages = standard_language_list();
+    $get_first = function($value) {
+      return $value[0];
+    };
+    $options = array_map($get_first, $standard_languages);

Sorry, but shouldn't that $get_first just simply be an argument to standard_language_list()? We've one use-case for it here, so most likely there are going to be 1,000 others.

+++ b/core/includes/form.inc
@@ -2553,6 +2553,38 @@ function form_process_select($element) {
+    $element['#empty_option'] = t('Language neutral');

#empty_option should always have a hyphen prefix and suffix for consistency; i.e., "- Language neutral -"

+++ b/core/modules/locale/locale.module
@@ -607,9 +607,49 @@ function locale_modules_disabled($modules) {
+  foreach ($type['language_select']['#process'] as $pfunction) {
+    if ($pfunction != 'form_process_language_select') {
+      $process[] = $pfunction;
+    }
+    else {
+      $process[] = 'form_process_language_select_locale';
+    }
+  }
+  $type['language_select']['#process'] = $process;

Any reason why Locale's #process doesn't simply act on top of the default #process in kind of a post-processor/map-reduce fashion? I'd imagine that might simplify things a lot?

Whatever we decide on, there are native array_*() helper functions in PHP that perform this operation much faster and less verbose. See array_search() and array_splice() & co.

20 days to next Drupal core point release.

fastangel’s picture

Hi,

Following the advice of @sun. I attached new patch.

fastangel’s picture

Sorry. I attached new patch.

fastangel’s picture

Status: Needs work » Needs review

Change the status to needs review.

Status: Needs review » Needs work

The last submitted patch, language_select_element_type-1280996-21.patch, failed testing.

fastangel’s picture

There are many tests that fail. If the patch code is correct I would have to modify some tests to work properly now. Do I get to work on it?

Greetings.

tarmstrong’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Thanks for the patch, fastangel. I have the locale tests passing on my machine now. Let's see what testbot does with it.

Status: Needs review » Needs work

The last submitted patch, language-select-1280996-44.patch, failed testing.

loganfsmyth’s picture

I'm not going to do a huge review since there is clearly still work to be done on this, but I had a few comments. Some of which are more to start discussion than anything.

* Does it make sense for the only_predefined option to automatically strip out any language that exists in the db? Maybe it would be nicer to have an #exclude_languages option or something.
* Would it make more sense to have two different elements for predefined vs standard lists? Or at least maybe a consistent property to decide what languages show up? The only_enabled and only_predefined seems like an overly confusing way to go. As it is only_predefined is more like "show_predefined" and only_enabled is "filter_enabled".
* Is using empty_option and empty_value a good idea? That will make it difficult to set the language_select #required, since it will automatically select LANGUAGE_NONE.
* Would it be useful to introduce usage of optgroups for this? This may require some other work, since there is logic in form.inc hard-coded to look for #type 'select'.
* Probably shouldn't be directly assigning #theme_wrappers since that will stop earlier #process callbacks from being able to add stuff.

tarmstrong’s picture

This feature is a little trickier than I originally realized. Here are four administrative pages (I'm certainly missing some) that all have language selects. No two pages let the user select from the same language list.

  • node/add/page
    • Enabled languages and language-neutral
    • If translation is enabled: enabled languages, disabled languages, and language-neutral. Enabled and disabled languages are in separate optgroups.
  • admin/config/regional/language/add
    • All languages that haven't been installed already. There is an additional option 'Custom language...' that allows the user to create their own language.
  • admin/config/search/path/add
    • Enabled languages, language-all
  • admin/config/regional/translate
    • Enabled languages and language-neutral
    • The system language option reads "System Language (English)"
  • user/1/edit
    • enabled languages only
    • displayed as radio buttons if count($options) > 5

This is clearly an ambitious abstraction. One way to do it might be a #languages option, that would be used like this:

$form['language'] = array(
  '#title' => t('New Language'),
  '#type' => 'language_select',
  '#languages' => array('predefined', '-enabled'),
);

Or like this:

$form['language'] = array(
  '#title' => t('New Language'),
  '#type' => 'language_select',
  '#languages' => array('none', 'enabled'),
);

This still makes the translation case a bit awkward. Maybe this would work?

$form['language'] = array(
  '#title' => t('New Language'),
  '#type' => 'language_select',
  '#languages' => array('none', 'enabled'),
  '#language_group' => TRUE,
);

I personally think the abstraction is still useful enough, but it is hairier than I expected.

Gábor Hojtsy’s picture

Issue tags: -i18n, -d8dx +language-base

Tagging for base language system.

dawehner’s picture

Assigned: tarmstrong » dawehner

Assign to myself to move this to language.module and trying to get the tests running.

dawehner’s picture

Status: Needs work » Needs review

Just moved the code to language.module as it makes more sense there.

I know the tests will not work, though maybe other people could have a look at.

dawehner’s picture

FileSize
7.81 KB

Okay this time with a patch.

Status: Needs review » Needs work

The last submitted patch, 1280996.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
7.74 KB

fixed the bugs, I hope ;-)

dawehner’s picture

FileSize
7.9 KB

Thanks for the help!
The problem of the test error is that translation.module alters the node form to apply some logic in there.

The question is whether the language_select should support this use-case or translation.module should handle it like it is now.

Status: Needs review » Needs work

The last submitted patch, 1280996.patch, failed testing.

TravisCarden’s picture

Re-tagging.

Gábor Hojtsy’s picture

Issue tags: +sprint

Need this or some variant of this to introduce language selectors on all entity forms in core easily. Currently you cannot assign language to taxonomy vocabularies, terms or files. You can assign to users with a custom widget, you can assign to nodes with a one-off widget and comments get language assigned automatically. We might want to have a UI there eventually. This issue is just about the underlying element for form API if we can make it work. If not, we can just introduce helper functions maybe for this to move on.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
5.93 KB

Because of the latest changes in the 8.x branch, the patch in #54 has to be very much changed. I started now with a new version which I attach here, that exposes a language select form element (that was actually also implemented alredy in the patch). Also added some tests for it.
Todos:
- use this language select form element in the core entities forms.
- add some more tests.

Gábor Hojtsy’s picture

We discussed in person that moving the language module dependence to the element somehow would be a good way to move this repeated ugly code to one place. Basically alter the element from language module that was previously defined by form.inc for a value field.

vasi1186’s picture

A new patch that puts the language_select form element in the Form API. Also, replaces the langcode form element in the NodeFormController with the new element. Ready for a first round of code review.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.incundefined
@@ -2645,6 +2645,36 @@ function form_process_select($element) {
 /**
+ * Processes a language select list form element.
+ *
+ * @param array $element
+ *   The form element to process.
+ *
+ * @return array $element
+ *   The processed form element.
+ */
+function form_process_language_select($element) {
+  // If the language module is not enabled, use the 'value' form element.
+  if (!module_exists('language')) {
+    $element['#type'] = 'value';
+    $element['#value'] = isset($element['#default_value']) ? $element['#default_value'] : '';
+    // Make sure the element will not be rendered on the page.
+    unset($element['#theme'], $element['#theme_wrappers']);
+  }
+  else {
+    // Don't set the options if another module(translation for example) already
+    // set the options.
+    if (!isset($element['#options'])) {
+      $element['#options'] = array();
+      foreach (language_list($element['#languages']) as $langcode => $language) {
+        $element['#options'][$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
+      }
+    }
+  }
+  return $element;
+}

Instead of embedding the module_exists() in form.inc, I'd define this element initially as a value type, then have an elements_alter hook in language.module to alter the process hooks and use a different process hook from language module in that case (which uses a select).

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -97,28 +97,12 @@ class NodeFormController extends EntityFormController {
-    if (module_exists('language')) {
-      $languages = language_list(LANGUAGE_ALL);
-      $language_options = array();
-      foreach ($languages as $langcode => $language) {
-        // Make locked languages appear special in the list.
-        $language_options[$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
-      }
-
-      $form['langcode'] = array(
-        '#type' => 'select',
-        '#title' => t('Language'),
-        '#default_value' => $node->langcode,
-        '#options' => $language_options,
-        '#access' => !variable_get('node_type_language_hidden_' . $node->type, TRUE),
-      );
-    }
-    else {
-      $form['langcode'] = array(
-        '#type' => 'value',
-        '#value' => $node->langcode,
-      );
-    }
+    $form['langcode'] = array(
+      '#type' => 'language_select',
+      '#default_value' => $node->langcode,
+      '#languages' => LANGUAGE_ALL,
+      '#access' => !variable_get('node_type_language_hidden_' . $node->type, TRUE),
+    );

This is just pure beauty! :)

Have you found the user module code convertible?

vasi1186’s picture

Attached a patch that:
- avoids the module_exists() call.
- uses the new form element also for the user edit form.

vasi1186’s picture

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

Status: Needs review » Needs work

This is looking very, very very nice. Only three minorish things :)

+++ b/core/includes/form.incundefined
@@ -2645,6 +2645,24 @@ function form_process_select($element) {
+ * Processes a language select list form element.
+ *

Name this language select form element maybe? Not list :) To differentiate somehow from the actual select list.

+++ b/core/modules/language/language.moduleundefined
@@ -158,6 +158,42 @@ function language_theme() {
+    $type['language_select']['#languages'] = LANGUAGE_CONFIGURABLE;

This seems to be the only configuration this element supports outside of the standard #options. Is form API docs in core to document this, or will we need this documented in contrib?

+++ b/core/modules/language/language.moduleundefined
@@ -205,6 +241,10 @@ function language_save($language) {
+function language_update_language_count() {
+  variable_set('language_count', db_query('SELECT COUNT(langcode) FROM {language} WHERE locked = 0')->fetchField());
+}

Let's add some phpdoc to this.

Gábor Hojtsy’s picture

Opened #1739876: Document new language_select field type for docs, since the docs for form elements are in the documentation contrib project.

Gábor Hojtsy’s picture

Ok, so we discussed introducing this on taxonomy terms and vocabularies is pretty simple now given it is only a few lines of code to add the UI now (and the schema support is all already there).

vasi1186’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
26.32 KB

As discussed with Gábor, I added the UI support for editing the language of Vocabularies and Taxonomy Terms. Also, added some tests and solved the issues from 64.

Gábor Hojtsy’s picture

Looks superb to me. It resolved all of my concerns, its simple, it removes complex code replacing with standard form processing code. What else can a core developer ask for? (Green testbot feedback notwithstanding).

Gábor Hojtsy’s picture

#1739928: Generalize language configuration on content types to apply to terms and other entities is a followup to make term language configurable the same way as node types are. That is out of scope for this patch, we just introduce the easily reusable form API solution here to this common problem.

plach’s picture

Status: Needs review » Needs work

This patch is lovely! I really like the approach, it's really clean and stragihtforward.
Just a couple of remarks, hopefully they won't delay commit too much ;)

+++ b/core/includes/form.inc
@@ -2645,6 +2645,24 @@ function form_process_select($element) {
+  $element['#value'] = isset($element['#default_value']) ? $element['#default_value'] : '';

I guess the default should be LANGUAGE_NOT_SPECIFIED.

+++ b/core/modules/language/language.install
@@ -102,3 +102,22 @@ function language_schema() {
+  variable_set('language_count', 0);

Sorry if this has been already explained above but why 0 an not 1? The comment is not very clear on this point. IIRC language_multilingual() used to check if language count was > 1.

+++ b/core/modules/language/language.module
@@ -158,6 +158,42 @@ function language_theme() {
+    $type['language_select']['#process'] = array('language_process_language_select', 'form_process_select', 'ajax_process_form');

Are the process callbacks here needed for the patch to work or are we hardcoding unrelated callbacks? In both cases I'd say we don't want override the #process array but add to it instead.

+++ b/core/modules/language/language.module
@@ -158,6 +158,42 @@ function language_theme() {
+    $type['language_select']['#theme_wrappers'] = array('form_element');

Same as above, we might override previous alterations here.

+++ b/core/modules/language/language.module
@@ -206,6 +242,17 @@ function language_save($language) {
+function language_update_language_count() {

This looks a bit verbose: maybe we could rename it to language_update_count()?

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -203,45 +203,26 @@ abstract class AccountFormController extends EntityFormController {
+      '#type' => language_multilingual()?'fieldset':'container',

Missing whitespaces between the string values and the operators.

plach’s picture

I guess sooner or later we should move the language selector to the base entity form controller. #1739928: Generalize language configuration on content types to apply to terms and other entities could be an enabler for this.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
25.78 KB

Attached a patch that should solve the issues from #70.
I actually removed the "form_process_language_select" from form.inc, because all what it did was to set a default value, so I just added this in the element_info() hook.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks superb :)

webchick’s picture

Title: New language_select element type for form API » Change notice: New language_select element type for form API
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Wow, what a great little patch! It cleans up a bunch of specialized code for a nice API and then applies it consistently across entities. :)

We should make a change notice for this, so people supplying entity forms know to call it.

catch’s picture

Status: Active » Needs work

Couple of minor code style things slipped in, can probably just follow-up here.

+++ b/core/modules/language/language.installundefined
@@ -102,3 +102,22 @@ function language_schema() {
+  // Update the language count, if the module was disabeld before, the

typo for disabled.

+++ b/core/modules/language/language.moduleundefined
@@ -158,6 +158,48 @@ function language_theme() {
+  // Don't set the options if another module(translation for example) already

Missing space after module.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
628 bytes

Attached a patch that solves only the #75 issue, I've seen that the rest of the patch has been already commited to core.
The "disabeld" type was somehow solved before the patch was applied.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, just fixed a comment.

webchick’s picture

Status: Reviewed & tested by the community » Active

Oops. Committed and pushed to 8.x. Thanks! And yes, I fixed that typo on commit. Sorry, I didn't mention it here cos it was very minor.

Back to active for the change notice.

vasi1186’s picture

Status: Active » Needs review

Created the change notice: http://drupal.org/node/1749954

corvus_ch’s picture

Status: Needs review » Fixed

Change notice looks good.

plach’s picture

Title: Change notice: New language_select element type for form API » New language_select element type for form API
corvus_ch’s picture

Priority: Critical » Normal
pp’s picture

Status: Fixed » Needs work

All the documentation is clear, but I can't know what does #language constants mean.

eg.:

1.
Is it true?
LANGUAGE_CONFIGURABLE | LANGUAGE_LOCKED === LANGUAGE_ALL

2.
LANGUAGE_CONFIGURABLE and LANGUAGE_LOCKED is disjunct?

(after reading documentation I read the bootstrap.inc and I get the answer this questions, but it isn't user friendly I mean :))

I suggest clarify this.

vasi1186’s picture

Whouldn't be better to just put like a reference to the bootstrap.inc (especially to the language_list() function) because basically the what a user specifies in the #language parameter it will be used directly to call the language_list(). So, if the language_list() internal implementation somehow changes and it will behave differently than it behaves now, then we should the change the documentation for this field, which is very possible to forget :). So, I really think that a reference to language_list() would be pretty good.

pp’s picture

I agree whit you, a reference to language_list() is enough, and it is a perfect resolution. (I am understanding now, what #language property is)

I suggest the following:

All the element options that a select field can use, can be also used for this new form element. In addition, there is a new option, "#languages", that is actually a set of flags to specify what languages should appear as options. This option used by the language_list() function. The following language constants can be used: LANGUAGE_CONFIGURABLE, LANGUAGE_LOCKED, LANGUAGE_ALL.

pp’s picture

Status: Needs work » Fixed

oh, Drupal handbook is a wiki. I made a modification.

Gábor Hojtsy’s picture

Need to document this in the documentation project as well. #1739876: Document new language_select field type (not in core).

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Changelog action happening in #1777870: Catch up changelog.txt with recent languages changes, removing off sprint. Thanks all!

xjm’s picture

Issue tags: -Needs change record