With the registry, there is no need for this old beast anymore.

Files: 
CommentFileSizeAuthor
#21 332725-locale_inc_callback_d7.patch4.71 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 17,888 pass(es). View
#19 332725-locale_inc_callback_d7.patch4.7 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 17,905 pass(es). View
#18 332725-locale_inc_callback_d7.patch4.68 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 17,899 pass(es). View
#16 332725-locale_inc_callback_d7.patch1.86 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 17,731 pass(es), 3 fail(s), and 0 exception(es). View
#9 locale_inc_callback.patch5.73 KBGábor Hojtsy
Unable to apply patch locale_inc_callback.patch View
#4 332725-rip-locale-inc-callback.patch2.51 KBDamien Tournoud
Unable to apply patch 332725-rip-locale-inc-callback_0.patch View
#3 issue-332725.patch2.38 KBlilou
Failed: Failed to install HEAD. View
#1 332725-rip-locale-inc-callback.patch2.53 KBDamien Tournoud
Failed: 7020 passes, 58 fails, 11 exceptions View

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
2.53 KB
Failed: 7020 passes, 58 fails, 11 exceptions View

And here's the patch.

lilou’s picture

Status: Needs review » Needs work

again 1 :

+          if (drupal_function_exists('_locale_parse_js_file')) {
+            locale_inc_callback('_locale_parse_js_file', $filepath);
+            watchdog('locale', 'Parsed JavaScript file %file.', array('%file' => $filepath));
+            $parsed[] = $filepath;
+            $new_files = TRUE;
+          }
lilou’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
Failed: Failed to install HEAD. View
Damien Tournoud’s picture

FileSize
2.51 KB
Unable to apply patch 332725-rip-locale-inc-callback_0.patch View

Well spotted lilou, but we need to keep the function call!

Here is a reroll. Ran the test suite, 100% pass.

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

A closely related issue is #187398: Re-split locale module which should be followed up, fixed and committed to Drupal 7 as well. Locale.inc is basically all locale.module subfunctionality, and is not at all shared in the whole system. Locale functions like t() are elsewhere, not in locale.inc. So locale.inc should move under locale.module and named like other Drupal sub-module includes are now named.

lilou’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Needs work

Looks like we could remove this nested if:

         if (substr($filepath, 0, strlen($dir)) != $dir) {
+          if (drupal_function_exists('_locale_parse_js_file')) {
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.73 KB
Unable to apply patch locale_inc_callback.patch View

This patch includes other places where inclusion of locale.inc was explicit and not implicit through locale_inc_callback. These should now also make use of drupal_function_exists(). This gets us much closer to execute a simple patch at #187398: Re-split locale module (which requires these changes or would need to modify incldue paths from includes/locale.inc to modules/locale/locale.admin.inc or so).

Please review this patch since it holds up #187398: Re-split locale module which holds up further nice developments (at least I have some cool ideas :) on locale functionality.

drewish’s picture

Status: Needs review » Needs work

I ran through the install in English and it worked fine but after grabbing the Traditional Chinese D6 language set (there aren't any D7 ones yet) and then re-running the installation I got the following warning:

Notice: Undefined index:  locale in /Users/amorton/Sites/dh/install.php on line 558

Call Stack:
    0.0049     375236   1. {main}() /Users/amorton/Sites/dh/install.php:0
    0.0081     597108   2. install_main() /Users/amorton/Sites/dh/install.php:1189
    0.1237    6350756   3. install_select_locale() /Users/amorton/Sites/dh/install.php:111

And no languages were displayed. I'm not sure if it was because there's a difference in D6 and D7 translations or a bug in the patch.

drewish’s picture

After reverting the patch I notice that there's more warnings but the form actually appears. It seems like the call to drupal_get_form('install_select_locale_form', $locales); at the bottom of install_select_locale() isn't finding the form.

sun’s picture

Most probably because the registry has no idea of Locale module / locale.inc during installation. #310467: Slimmer hook_theme() contains a fix for this, which allows us to load locale.module + locale.inc in maintenance mode.

Gábor Hojtsy’s picture

Uhm, well #187398: Re-split locale module is quite important for any future development on locale module to happen, so it would be good to bring this forward, so we can get that committed too and get over the cleanups with actual feature development (eg. cleaning up the very dated locale UI).

Gábor Hojtsy’s picture

Status: Needs work » Postponed

Turns out that #310467: Slimmer hook_theme() would not help solve this issue, because locale.inc is not at all in the locale module's location and is not referenceable through its info file. Ironically, for that to happen, we would need to fix #187398: Re-split locale module first, which was marked postponed on this issue. So looks like our circle is back to fixing #187398: Re-split locale module first and mark this postponed on that.

andypost’s picture

Status: Postponed » Needs work

Make it active 'cos fixed #187398: Re-split locale module

Now all form-related staff is moved to locale.admin.inc

andypost’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
FAILED: [[SimpleTest]]: [MySQL] 17,731 pass(es), 3 fail(s), and 0 exception(es). View

Because locale.inc is loaded in locale_init() and used only in locale_js_alter() there's no reason to save locale_inc_callback()

Status: Needs review » Needs work

The last submitted patch, 332725-locale_inc_callback_d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 17,899 pass(es). View

Forget to include changed tests

andypost’s picture

FileSize
4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 17,905 pass(es). View

Added comment about locale_js_alter() could be called from simpletest after installing locale.module so locale_init() is not called

include_once() changed to require_once() to unify with other tests

Dries’s picture

+++ modules/locale/locale.module	22 Feb 2010 03:02:56 -0000
@@ -766,6 +756,9 @@ function locale_js_alter(&$javascript) {
+  // Could be called from simpletest without locale_init().
+  require_once DRUPAL_ROOT . '/includes/locale.inc';

I think it is odd that we specifically reference Simpletest here.

andypost’s picture

FileSize
4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 17,888 pass(es). View

Changed comment, locale_init() sometimes could be skiped

Dries’s picture

Status: Needs review » Fixed

Good! Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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