The locale module is already sorta page-split. That is, its page handlers live in locale.inc, which is conditionally loaded, rather than in the module file. There's no reason we need to do that old and yucky way anymore. :-) Unfortunately much of the locale system is called from various places in the install process, so we can't just move the entire locale.inc file to locale.admin.inc. The attached page moves those parts of locale.inc that are page-handler only to locale.admin.inc, but leaves in locale.inc those pieces that are called at other times. At least, I think it does. :-) Reviews to confirm that it was reorganized properly are most welcome.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Needs review » Needs work

/admin/build/translate

    * warning: include_once(includes/locale.inc.) [function.include-once]: failed to open stream: No such file or directory in /drupal6/modules/locale/locale.admin.inc on line 501.
    * warning: include_once() [function.include]: Failed opening 'includes/locale.inc.' for inclusion (include_path='.:/usr/share/php:/usr/share/pear') in drupal6/modules/locale/locale.admin.inc on line 501.
Gábor Hojtsy’s picture

There is settings.php modified in this patch. Not good.

Crell’s picture

Status: Needs work » Needs review
FileSize
74.02 KB

OK, let's take this again, from the top...

Crell’s picture

Status: Needs review » Needs work

This has, I'm certain, been broken by later patches. However, there's still a string-freeze patch pending so I am going to defer to that for now, since RC/string-freeze is nearly upon us. I'll reroll this after string freeze hits since this should have side effects other than code reorganization, unless Gabor tells me otherwise.

Crell’s picture

Status: Needs work » Needs review
FileSize
73.84 KB

Here we go, against latest HEAD.

Gábor Hojtsy’s picture

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

Oh, well, I regret not looking at this more closely before RC1... it looks like a nice cleanup without much of a side effect (only doing admin forms/pages)... *but* RC1 was a string freeze, and moving around this amount of string from the includes folder to the locale module folder would probably not get welcomed by our translators (then all the strings move to a different translation file for them, and it complicates their work a lot).

Postponing a proper locale module split to Drupal 7.

Crell’s picture

Fortunately this one shouldn't have a noticeable performance impact the way the others should; it's just a code clean up. See ya in 7.

lilou’s picture

Status: Needs review » Needs work

Patch no longer apply.

Gábor Hojtsy’s picture

It would be lovely to see this committed in Drupal 7 sooner then later :)

lilou’s picture

For a beginning ...

Locale tests : 68 passes, 18 fails, 0 exceptions

Gábor Hojtsy’s picture

Locale.inc hunk missing from patch?

lilou’s picture

FileSize
75.85 KB

More tests fails : 67 passes, 44 fails, 0 exceptions

Gábor Hojtsy’s picture

What are remaining issues with the patch?

Gábor Hojtsy’s picture

Status: Needs work » Postponed

Now waiting on #332725: locale_inc_callback() is a thing of the past which eliminates locale_inc_callback() and uses the Drupal 7 registry, so that the exact location of the functions will not be so relevant. After that patch landed, this patch will almost just be moving of includes/locale.inc to modules/locale/locale.admin.inc. (The menu related callback lookups are already fixed due to the registry).

sun’s picture

subscribing

Gábor Hojtsy’s picture

Status: Postponed » Needs work

Turns out #332725: locale_inc_callback() is a thing of the past requires #310467: Slimmer hook_theme() to be solved, which requires locale include files to be with locale module, not in the general includes folder. So we are back at this issue, marked #332725: locale_inc_callback() is a thing of the past postponed on this instead.

Gábor Hojtsy’s picture

Assigned: Crell » Gábor Hojtsy
Status: Needs work » Needs review
FileSize
107.62 KB

Keeping part of the code in locale.inc and some moved to locale.admin.inc does not fly well due to how one needs to cross-include them in many cases. Again, locale was the first module to pioneer the split module code style, way before any other module started to do it. Locale.inc contains a bunch of "API" functions which are used very little outside of the locale module and make no sense whatsoever if locale module is not enabled (unlike the t(), format_plural() etc. API in common.inc and the languauge_*() API in language.inc). So the locale.inc APIs and admin screens closely belong to locale module and make no sense when that module is not enabled.

Therefore I am proposing moving all of locale.inc to the locale module as locale.admin.inc. We can slice and dice API functions into a locale.api.inc or somesuch, or even break down to locale.import.inc, locale.export.inc, etc. if need be, but let's make the first step of moving this to the locale module and work from there. As it is now, I would not like to propose new modifications for a file, which is ought to be moved ASAP to its module, since then we'd results in conflicting patches.

How does this look like?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

I'd RTBC this patch, if it would pass. Am I blind - or is the new file not added in there?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
205.01 KB

Right. Forgot to actually use diff -N. Attached. Let's review/test again.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Dave Reid’s picture

sun’s picture

Priority: Normal » Critical
Issue tags: +API clean-up, +D7 API clean-up

I'm not sure what exactly lives in locale.inc, but in the name of #593746: Prepare Drupal core for dynamic data translation this issue here does not only sound like a badly needed clean-up, but also a critical step for that issue, because the UI translation interface of Locale will only remain for t() strings.

This issue is mainly about moving functions around. However, we should absolutely ensure to move them correctly.

Is it possible that only the .po file detection and import functions are required for the installer?

I'm trying to quickly make the most sense out of this:

1) locale.inc contains many administrative functions and form builders that really belong into locale.admin.inc.

2) locale.inc contains functionality required for installing Drupal, which may has to be kept in there.

3) locale.inc contains JavaScript parsing and other necessary functionality for t() and locale().

4) locale.module contains the locale() function, conditionally used by t(), which lives in common.inc. Since we don't know of any other implementation of locale(), that should move into locale.inc, so locale.inc becomes the interface language translation sub-system, and we can make it pluggable (simple variable like password.inc), just like we make language.inc pluggable in preparation for DDT.

5) locale.inc contains Drupal's default interface language negotiation providers, which are hook implementations on behalf of Locale module. Since these are the system's default providers, they really should be provided by System module, and therefore move into system.locale.inc.

That would basically translate into the following summary:

a) The interface translation sub-system moves into locale.inc, and we make it pluggable (like password.inc). This mainly includes locale() and JS parsing.

b) Locale module becomes an administrative interface for the localization system.

c) The default interface language negotiation providers move into system.locale.inc.

Thoughts? Let's quickly get this done.

nedjo’s picture

The steps proposed indeed sound correct. An early concern in this issue though was the reuse of locale.inc code at install time and this is still an issue. We need to ensure all locale functions called in install.php and install.inc (and the functions' dependencies) are in locale.inc.

the UI translation interface of Locale will only remain for t() strings.

Yes, it looks like #593746: Prepare Drupal core for dynamic data translation will mean deprecating some of the initial steps taken in D6 to expand the locale system beyond the initial t() code-based string use case.

Fully deprecating non-t() use might include:

* removing the locales_source.textgroup field, currently used to store the type of string being stored, with 'default' being the value for code-based strings.
* removing various UI elements for textgroups.

But where then does contrib handle non-t() strings? Does it need to create a parallel interface?

Or could we e.g. have a model where contrib uses the existing locale UI but points it to its own data (through e.g. UNION queries)? In which case, the data handling is t()-specific but the UI is agnostic. Meaning we don't have to remove the textgroup support but we may need to tweak it to ensure it's extensible.

Gábor Hojtsy’s picture

This locale.inc is indeed a huge mess, and I'm happy this might get some last minute attention :) For install time, what it needs is:

- find .po files in the install profile's directory
- offer those for selection
- import the selected one .po file into memory (on each page load)
- use that data in st() to present the installer screens

After these, locale module is installed and available, so its normal database backend is used.

I'm not seeing where custom data handling goes, if we go off from using textgroups for that, other uses will not require textgroup support to stay as far as I'm concerned.

Finally, if you make locale.inc pluggable, make sure you move stuff there which will indeed need to be rewritten. Keeping the whole JS/.po parsing there might not need to be overriden. I totally agree it is best to make as much overridable as possible, but not sure about the use case here. So far, we provided overridability of locale() via replacements for locale module, since we thought JS or .po parsing was not target for replacement.

Hopefully I'm helpful :)

sun’s picture

Status: Needs work » Needs review
FileSize
114.24 KB

@nedjo: Thanks -- we won't have time to change how t() and locale() are working (regarding textgroups and stuff), so the overall functionality will stay the same in D7. The actual change will follow in D8 and will heavily depend on what we, the overall i18n team, will come up with in contrib. It rather belongs into the DDT issue, but I foresee an entirely different UI for DDT than what Locale module is providing; rather in the direction of l10n_client... but anyway, that's all TBD and totally off-topic right now :)

@Gábor: Thanks for your input - that's very helpful!

After spending some with the movement of functions and required changes elsewhere, I have the feeling we need to do this in two steps, i.e. first move stuff around, and then deal with locale() and making locale.inc pluggable.

Let's see what the bot thinks about this: Moves admin forms 1:1 into Locale module and only changes file paths accordingly.

Status: Needs review » Needs work

The last submitted patch failed testing.

mattyoung’s picture

.

andypost’s picture

Suppose moving functions is only possible task for D7 it does not change API but give a little performance - less useless code parsed on request

sun’s picture

Issue tags: -D7 API clean-up

Since this is only moving around code, this can probably still happen for D7, but I'm removing the critical tag now to get a more concise overview of really-horribly-critical issues.

Status: Needs work » Needs review
Issue tags: -API clean-up

Re-test of drupal-locale.27.patch from comment #27 was requested by andypost.

Status: Needs review » Needs work
Issue tags: +API clean-up

The last submitted patch, drupal-locale.27.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
115.97 KB

Reroll, let's test this

Status: Needs review » Needs work

The last submitted patch, 187398_locale.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
101.2 KB

Language providers should live in locale.inc but forms move into locale.admin.inc

andypost’s picture

Bot is happy, now need reviews to include in upcoming alpha

plach’s picture

I'll try to give this a look ASAP

plach’s picture

Status: Needs review » Needs work

The following functions are still defined in locale.module (and however they are declared to belong to locale.inc).

locale_date_format_language_overview_page()
locale_date_format_form()
locale_date_format_form_submit()
locale_date_format_reset_form()
locale_date_format_reset_form_submit()

Couldn't actually test this yet.

andypost’s picture

Status: Needs work » Needs review
FileSize
112.43 KB

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

sun.core’s picture

Even if it doesn't kill locale.inc, I still badly want to see this.

Hopefully, @plach can RTBC.

plach’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good: it simply moves form stuff in locale.admin.inc and updates locale_menu() accordingly.
The bot doesn't complains and I run an italian installation and all went as expected, thus RTBC.

However I had troubles to perform a translated installation (even with the patch not applied) and I couldn't understand if the fault was in my po files or in the installer. I opened a new issue for this: #678916: st() should take into account string contexts.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

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