Locale module includes 1350+ lines of unneeded code on all page loads

Ouch, missed a crucial part of the export API. It is in fact 1350+ lines of code that are included on all page loads just for .po file import/export. Will update the summary above (originally said 900). There are also another 200+ lines of code only ever used on the translation admin pages (that will be a different patch though). This attached patch already breaks down locale.inc to half of its original size :)

It will most probably not pass, some more places need includes, the patch mostly just moves code around so far and I've only added includes on the import/export admin UI screens to use the API. Those tests should hopefully pass.

Added Drupal 7 note to the end.

@DamienMcKenna: this is almost a non-issue in Drupal 6. Let me give a little background. The huge locale.inc as we know it was introduced in 2004 August (almost 7 years ago). Now at that time Drupal was insistent on having one file per module, so we had system.module, locale.module, etc. all right in /modules, no subdirectories, no include files, etc. Locale module was such a big beast though that it "got an exception", and its very rarely used functionality got into includes/locale.inc (there was no better place to put it). All the horrible function names date back to that time, like _locale_import_read_po(). And yes, I was heavily involved, there was not much of a better possibility back then.

Now 7 years later, locale.inc is still full of very rarely used code, BUT people in the Drupal 7 cycle assumed that it is a modern include file and should actually be populated with code that is used often (like common.inc, language.inc and friends). Well, that meant we had the old big mostly unused stuff lying in there and some new things that are to be included on all page loads (if locale.module is enabled).

Back in Drupal 6, locale.inc is only included (a) in the installer if it needs to read .po files (b) for locale admin pages for which code was in locale.inc (c) if a new JS file is encountered and needs to be parsed for translatable files. That's it. Compared to that, Drupal 7 includes the file on each page load from locale_init(). While Drupal 7 did move the main admin pages to locale.admin.inc, some of the admin helper functions and all of the gettext code is still in locale.inc and is more than half of the size of that monster file.

One can argue that Drupal 6 still includes lots of unrelated code when new JS files are encountered, that should however be very rare on a production (stable) website, right? So I'm not sure this is applicable to Drupal 6. The Drupal 7 issue is there though due to this general misunderstanding of what locale.inc was used for. This should be cleared up once and for all.

I'm not sure locale.inc as a standalone actually makes sense (in Drupal 8) if we include it on each page load right form locale.module anyway. We could just as well put the common used functions to locale.module (while we move out the less used ones to their own place).

Finally, I can see use for the gettext API outside of locale module, for example, string overrides module uses the current gettext code to import .po files for its features, and it definitely does not need locale.module. So it would maybe make more sense to have a gettext.inc or gettextapi.inc in the includes dir after all, for reuse independent of locale.module. (It still needs the refactoring, see above).

As said, this patch now has includes/gettext.inc for general reuse. No code changes there still, that will be in other patches, mostly #1189184: OOP & PSR-0-ify gettext .po file parsing and generation.

@fietserwin: there is not a usual practice to separate admin pages of a module into big groups like I'm trying to do here. Lumping stuff into an admin.inc is more common. However, we have lots of stuff to deal with here, and piling them into one big file after another does not work IMHO. I think the import/export functionality is a logical piece, so I would not divide them further. (That would end us up with really very small files).

On gettext.inc or gettext.api.inc, none of the other files in includes/ are .api.inc, although they do provide APIs. Think cache.inc for example is a unique set of APIs. Also, Drupal uses .api.php suffixes for documentation, so .api.inc could be very misleading. I had api.inc at the end my original suggested filename (without a dot before api though), but then when it moves to includes, it is assumed it's an API, and no need to explicitly state it.

In terms of making gettext.inc a real API (the renaming and refactoring part), we already have, #1189184: OOP & PSR-0-ify gettext .po file parsing and generation that can be considered the existing follow-up issue. Already marked it postponed. Thanks for the tip!

Attached patch fixes a bug when enabling locale module, now includes the right file in locale_system_update(). Also added an include for locale.inc in locale_language_negotiation_info(), which was emitting notices on first locale enable. I think that bug should also exist in Drupal 7. It does not define negotiation options well at the first locale enable cycle, looks like, because it will use the wrong constant values. Or better said no constant values.

@sun: I'd really strongly prefer to postpone renaming the functions to be coupled with reworking their functionality. As their functionality stands now, calling them gettext_* is **incorrect** given they closely work with locale tables and some locale APIs. These functions are not yet the reusable ones, we'll have in Drupal 8, but why should one patch make them so right away? Also, on the gettext namespace, there is definitely a function called gettext in PHP, there do not seem to be others using this prefix: http://php.net/gettext

On the names for files and the global include, I agree.

On the last change, that fixes an issue I think already happens with Drupal 7. If you have E_ALL display and you enable locale module, this hook is invoked before locale_init() runs and therefore locale.inc will not yet be present, and the negotiation mode constants will be considered strings.

Well, there is also discussion if it would be possible to backport the removal of gettext.inc stuff from locale.inc for Drupal 7, so depending on @webchick and the community I guess it might get backported in some way.

Here is an updated patch that (a) renamed locale.import-export.inc to locale.bulk.inc, (b) includes gettext.inc on the top of that file (c) adds file level comment to locale.bulk.inc, (d) removes the change you are disputing does not belong here.

I've also (hopefully) fixed tests fails by including the bulk.inc code at places where it was not included yet (installer and system change response).

Any other comments?

Patch needed a reroll + a fix for the test fails. The tests were simply failing due to import-export.inc referenced in the batch production, not bulk.inc. Should hopefully pass now.

@sun: I've also added your suggested file level @todo, hightened the issue to major as per your suggestion and changed the title to express the OMG factor better and help people understand the major. Also adding needs backport to D7 tag.

Note that no tests failed, but we have 211 exceptions. Every single one of those are related to the above discussed bug that now locale_language_negotiation_info() at places where locale.inc is not anymore loaded but it needs constants from there. This happens when the locale module is enabled and previously http://api.drupal.org/api/drupal/modules--locale--locale.module/function... did include that in the process of the module being enabled (since it immediately reacted to itself being enabled), but locale.inc was only universally included on the next bootstrap. Now since the .po file import that happens in locale_system_update() is now not in locale.inc after the patch (which is the whole point :), it does not include this file which would be needed later. So we should indeed add the locale.inc inclusion in locale_language_negotiation_info(). Note that this merely mirrors the same code on top of the function above, locale_language_types_info(), which does the same.

I'd ask not to attempt to resolve the circular dependency inbetween locale.inc and locale.module here, and that locale.inc is effectively locale.module code at a different place (always included with bootsrap). This patch is already huge and that general issue would be great to resolve elsewhere.

With this fix, the patch should again pass.

Debated a bit with sun about the extent of the API change for moving funcitons around, and we maintain this is D8 only so far due to the required function move-arounds in D7, which are "massive" API changes (due to the number of stuff moved).

Updated patch now that #1188430: Rip out textgroup support from locale module landed, which totally touched the same code :) Hope I reproduced it proper. Due to the removed textgroup support, the patch is smaller.

Patch still moves around the same code to the same places. Still adds the same one-line additions to fix stuff where it was previously erroneously included on all page loads, and now need specific includes. It just needed an update for the textgroup removal patch land. Double-checked it still moves the same code chunks as before. So moving back to RTBC on these grounds.

@sun: I think we wanted to rework the code to be a real gettext API not tied to locale module, and we'll need a group there at that point. We can remove it at this point, I just left it there since it signals the related functions pretty well, so no need to work out which ones to include later. This file still has a huge todo attached to do a real rework (ALA gettextapi.module for D7 but we'll surely debate the approach to use in the rework later once this lands :).

On the include, you are right, updated that.

Thanks! There are some contrib modules that would work with this code, but instead they duplicate the code, because it is so inflexible. Modules like l10n_update, gettextapi, etc. all duplicate this code in their own modules. That is why we should continue working on API-ifying this, and we have plenty of issues for that in the #D8MI queue :)

@webchick: yeah, its this type of question. It is big amount of code included in all page requests, that it should not be. It is in an include file with other things that are not included with the page request. Maybe for Drupal 7, we should move the always to be available things to locale.module proper and leave the stuff that should not be included with locale.inc, so that whoever includes locale.inc in contrib will get some junk, but will not loose any code.

For Drupal 8, that does not really make sense, as we are moving big systems to their own include files to refactor one on one. The administrative functions in the main locale.inc file thing is not really how Drupal does stuff anymore, it is just that it kept being used for that in Drupal 7, and other people piled in code they wanted to include on all page requests, and the two things intermingled. Locale.inc still has this code comment up top:

/**
* @file
* Administration functions for locale.module.
*/

It is just that it was in a very unusual and non-standard place and that this note was not taken into account, that we are at a place where we are. I think this does not really reach to a level of change that the lock system was in Drupal 6, which was pretty big landing pretty late way into it being stable. It can be a tough call, but I'm not sure in this case it is as tough. We can approach the problem for Drupal 7 with the above more conservative approach.

@sun: thanks :) Are you referring to the full-on patch as in for Drupal 8 or the alternative approach outlines above where we'd move the always used code to locale.module and leave the less used code in locale.inc as it was in Drupal 6?

Noticed that an include was not updated in the install process. Patch attached for that.

#1297506: non-English installation: Call to undefined function _locale_import_read_po() in ../includes/install.inc on line 1113 in fact has a better solution. Since the bug only applied to 8.x, we can move this back to 7.x for backport. Ignore #45 for that please.