Undefined index: locale in install.php

Install.php line 543 should be if (isset($_POST['locale']) && $_POST['locale'] == $locale->name) { - that code runs on every single page until you've selected a valid locale, but $_POST is empty first time (or on a page reload). The LANGUAGE_RTL constant is defined in locale.module - so locale.inc should not run without of locale.module (note: the later locale.inc includes in install.php are OK, because these are after full bootstrap with modules included, and locale module installation is enforced earlier by the very same condition, i.e. $install_locale != 'en').

Please test the attached patch.

I feel really uneasy about moving RTL constants to locale.inc:

locale.module is the main code for localization, always included (only it wasn't in the install process, by mistake I believe), while locale.inc is an additional part, generally relying on locale.module in many ways, only included in some special cases. Moving the definitions to locale.inc, you'll most probably cause problems with RTL languages on installed Drupal site, as the constants will be then undefined on regular localized pages, where locale.inc is generally NOT included. Note that the constants are not just local stuff, they are used on several places across the Drupal code.

The fatal error mentioned at #2 indicates a variable_set() call before database bootstrap. I can't reproduce that, can you give more information how to reproduce? But anyway, that's not related to my patch at #1. So #1 is the one for review, still, as far as I can tell.

The purpose of locale.inc is to NOT include unnecessary things on regular pages. The constants are used also elsewhere, so they are not "hidden" into that file. We're not going to re-arrange the way how Drupal is split into files here, post RC. Note that locale.inc is NOT a general include file (like includes/common.inc and others) - this one is different due to purely historic reasons, and serve a purpose similar to files like system/system-admin.inc and similar. It's internal add-on for administrative tasks inside locale.module, which make generally no sense without it's parent module included.

Otherwise no time to test now, sorry for the delay.

The constants will NOT be always available to locale.module. The file locale.inc is not included in most cases, it's only included in the rare situations when administrative tasks need to be performed. In all other cases, any constants defined in locale.inc are NOT available, although locale.module is in use. I also discovered, that the base system checks for these constants too (drupal_add_css() - common.inc), so changing the behavior is not a safe thing.

We need to research WHY it triggers an error in the first place.

I'd say it is safe to move the constants to locale.inc because in all other instances (in Drupal core), a defined() check is used before using the constant.

Eh, as far as I see, in common.inc the constant is used to add RTL stylesheets. Of course this only makes sense on a localized site, where locale.module is included, so that's the purpose of defined() check. Now if we move the constants to locale.inc, which is only included rarely (for administrative tasks), the constants will be undefined on regular pages (although locale.module is in use). Sure, defined() check will avoid PHP warnings, but the point is, that the *functionality* will be broken! No RTL stylesheets added, so on all Arabic, Hebrew and other RTL sites, RTL theming will be gone (except for administrative pages).

Fixing an installer notice by introducing a critical runtime RTL-theming bug is not nice.

Grepping for the constants:

$ grep -r "LANGUAGE_LTR" *
includes/locale.inc:    '#options' => array(LANGUAGE_LTR => t('Left to right'), LANGUAGE_RTL => t('Right to left'))
includes/locale.inc: *   LANGUAGE_LTR or LANGUAGE_RTL
includes/locale.inc:function locale_add_language($langcode, $name = NULL, $native = NULL, $direction = LANGUAGE_LTR, $domain = '', $prefix = '', $enabled = TRUE, $default = FALSE) {
includes/locale.inc:    $direction = isset($predefined[$langcode][2]) ? $predefined[$langcode][2] : LANGUAGE_LTR;
modules/locale/locale.module:define('LANGUAGE_LTR', 0);
modules/locale/locale.module.orig:define('LANGUAGE_LTR', 0);

$ grep -r "LANGUAGE_RTL" *
includes/common.inc:          if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
includes/common.inc:    if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
includes/locale.inc:    $form['direction'][$langcode] = array('#value' => ($language->direction == LANGUAGE_RTL ? t('Right to left') : t('Left to right')));
includes/locale.inc:    '#options' => array(LANGUAGE_LTR => t('Left to right'), LANGUAGE_RTL => t('Right to left'))
includes/locale.inc: *   LANGUAGE_LTR or LANGUAGE_RTL
includes/locale.inc:    "ar" => array("Arabic", /* Left-to-right marker "‭" */ "العربية", LANGUAGE_RTL),
includes/locale.inc:    "fa" => array("Persian", /* Left-to-right marker "‭" */ "فارسی", LANGUAGE_RTL),
includes/locale.inc:    "he" => array("Hebrew", /* Left-to-right marker "‭" */ "עברית", LANGUAGE_RTL),
includes/locale.inc:    "ps" => array("Pashto", /* Left-to-right marker "‭" */ "پښتو", LANGUAGE_RTL),
includes/locale.inc:    "ur" => array("Urdu", /* Left-to-right marker "‭" */ "اردو", LANGUAGE_RTL),
modules/book/book.module:  $variables['language_rtl'] = (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) ? TRUE : FALSE;
modules/color/color.module:          if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
modules/color/color.module.orig:          if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
modules/locale/locale.module:define('LANGUAGE_RTL', 1);
modules/locale/locale.module.orig:define('LANGUAGE_RTL', 1);
themes/garland/template.php:  if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {

All the times, the defined() check is there because if the locale module is not included, the constants are not available. Actually, we could do some cleanup by:

- moving the constants to some more common place (language.inc I would say, which is always included past the language bootstrap phase => then we should check that nothing requires it before/without the language bootstrap)
- removing the defined() checks

The critical part of the conditionals above is the $language->direction == LANGUAGE_RTL, the defined() is just there to serve as an E_ALL defense on cases, when the constant might not be there.

@robertgarrigos: #17 says "rerolled". Is there supposed to be a new patch attached? Thanks.

You should put the
if(!empty($_POST['locale'])){
before the foreach, so if it isn't set, then its unnecessary to walk trough the array.

Tyrael

Reroll patch #15 against CVS/HEAD and add check $_POST['locale'] before foreach according to #20.

If we are going to define these constants in bootstrap.inc, we should be able to remove many of the 'if defined' checks? More code clean-up would be in order, not?

I've ported #21 patch to Drupal 6.8
Waiting for reviews... and for a commit ;)
Regards! :)

I've tested the patch at #23 with Drupal 6.8 and it installed succesfully using spanish language with no errors.

Thanks thePanz for the patch!

This patch removes the defined() calls as requested in #22. The constants are defined very early in the bootstrap process (prior to calling drupal_bootstrap()), so none of these are necessary anymore.

Wow. This is probably the most annoying thing that can happen when you try, test, or install Drupal for the first time. Bumping to critical.

Attaching patches for both D7 and D6.

Nice. I love these patches that make sensible changes and remove a bunch of silly conditionals at the same time.

Committed to HEAD. Moving back to 6.x. However, I disagree with critical status. Not nice, yes, but it only affects environments with E_ALL reporting in php.ini which is not the norm for production sites.

Effectively, the patch for 6.x is the identical.

Effectively, the patch for 6.x is the identical.

Thanks, committed to Drupal 6 as well.