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.
Comment | File | Size | Author |
---|---|---|---|
#40 | 187398_locale_split.patch | 112.43 KB | andypost |
#36 | 187398_locale_split.patch | 101.2 KB | andypost |
#34 | 187398_locale.patch | 115.97 KB | andypost |
#27 | drupal-locale.27.patch | 114.24 KB | sun |
#20 | locale-admin-inc.patch | 205.01 KB | Gábor Hojtsy |
Comments
Comment #1
catch/admin/build/translate
Comment #2
Gábor HojtsyThere is settings.php modified in this patch. Not good.
Comment #3
Crell CreditAttribution: Crell commentedOK, let's take this again, from the top...
Comment #4
Crell CreditAttribution: Crell commentedThis 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.
Comment #5
Crell CreditAttribution: Crell commentedHere we go, against latest HEAD.
Comment #6
Gábor HojtsyOh, 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.
Comment #7
Crell CreditAttribution: Crell commentedFortunately 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.
Comment #8
lilou CreditAttribution: lilou commentedPatch no longer apply.
Comment #9
Gábor HojtsyIt would be lovely to see this committed in Drupal 7 sooner then later :)
Comment #10
lilou CreditAttribution: lilou commentedFor a beginning ...
Locale tests : 68 passes, 18 fails, 0 exceptions
Comment #11
Gábor HojtsyLocale.inc hunk missing from patch?
Comment #12
lilou CreditAttribution: lilou commentedMore tests fails : 67 passes, 44 fails, 0 exceptions
Comment #13
Gábor HojtsyWhat are remaining issues with the patch?
Comment #14
Gábor HojtsyNow 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).
Comment #15
sunsubscribing
Comment #16
Gábor HojtsyTurns 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.
Comment #17
Gábor HojtsyKeeping 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?
Comment #19
sunI'd RTBC this patch, if it would pass. Am I blind - or is the new file not added in there?
Comment #20
Gábor HojtsyRight. Forgot to actually use diff -N. Attached. Let's review/test again.
Comment #22
plachComment #23
Dave ReidSubscribing since this would affect #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks
Comment #24
sunI'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.
Comment #25
nedjoThe 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.
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.
Comment #26
Gábor HojtsyThis 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 :)
Comment #27
sun@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.
Comment #29
mattyoung CreditAttribution: mattyoung commented.
Comment #30
andypostSuppose moving functions is only possible task for D7 it does not change API but give a little performance - less useless code parsed on request
Comment #31
sunSince 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.
Comment #34
andypostReroll, let's test this
Comment #36
andypostLanguage providers should live in locale.inc but forms move into locale.admin.inc
Comment #37
andypostBot is happy, now need reviews to include in upcoming alpha
Comment #38
plachI'll try to give this a look ASAP
Comment #39
plachThe 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.
Comment #40
andypostNow all form-related staff moved to locale.admin.inc
Comment #41
sun.core CreditAttribution: sun.core commentedEven if it doesn't kill locale.inc, I still badly want to see this.
Hopefully, @plach can RTBC.
Comment #42
plachThe 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.
Comment #43
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.