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
StatusFileSize
new2.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
StatusFileSize
new2.38 KB
Failed: Failed to install HEAD.
[ View ]
Damien Tournoud’s picture

StatusFileSize
new2.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

catch’s picture

Status:Needs review» Needs work

Looks like we could remove this nested if:

<?php
        
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
StatusFileSize
new5.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
StatusFileSize
new1.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
StatusFileSize
new4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 17,899 pass(es).
[ View ]

Forget to include changed tests

andypost’s picture

StatusFileSize
new4.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

StatusFileSize
new4.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.