I tried to run a localized install with the attached locale package (Drupal 5 translations packaged in the Drupal 6 expected format). This used to work nicely on Drupal 6 development versions, but this post-beta version does not seem to run nicely. After the progress bar seemed to run nicely, I got this:

Error importing interface translations
Please continue to the error page
An HTTP error 200 occured. http://localhost/drupal6/install.php?locale=hu&profile=default&id=1&op=do

The "error page" text was a link to http://localhost/drupal6/install.php?locale=hu&profile=default&id=1&op=f... (which does not show any errors obviously, so the link is quite misleading).

Also, HTTP error(!) 200 is an interesting idea :)

Watchdog contains this error:

Cannot modify header information - headers already sent by (output started at /Users/gabor/Websites/drupal6/includes/common.inc:2170) - /Users/gabor/Websites/drupal6/includes/common.inc - 141. sor.

Lin 2170 is echo drupal_to_js($var); which comes before the header() calls on line 141 as it seems. For some reason.

I also added diagnostic code to _locale_batch_import() to see when it goes broken. Added these two lines:

    watchdog('locale', 'before:'. $filepath);
    _locale_import_read_po('db-store', $file, LOCALE_IMPORT_KEEP, $langcode[2]);
    watchdog('locale', 'after:'. $filepath);

This showed that several po files are properly imported in the first batch request, and just as the request was about to output something, the "cannot modify header information" error occurs and the next batch request is not run, resulting in this error page.

CommentFileSizeAuthor
#13 install_json.patch1.3 KBgábor hojtsy
#9 instal.php_.patch483 bytesyched
#4 json_batch.diff509 bytesgábor hojtsy
hu-6.0.tgz144.69 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Might be important to note that this does not happen if I have JS turned on (and no has_js cookie obviously). Then the locale import runs nicely. This is not surprising much, as the offending output which causes the error comes from a JSON function.

yched’s picture

yched commented

Subscribing - I'm not able to actually look at the code just right now, but I'll give a closer look.

yched’s picture

yched commented

this does not happen if I have JS turned on

You mean turned *off*, right ?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review
StatusFileSize
new json_batch.diff509 bytes

OK, the error basically comes down to _batch_do() outputting the JSON content, but then returning and letting anything else do output. This shows in the 'locale-batch' task of install.php where $output .= _batch_page(); is used, and then $output is used as contents to a maintenance themed page. So while in _batch_page(), _batch_do() is called and output the JSON with headers, the maintenance page themeing tries to mess with headers again (and print out a big page of Drupal stuff), which it should not do.

The obvious fix is to exit once the json is printed, but we definitely need yched to look at this, to validate the idea. I tested translation imports with this exit() added, and it run very nicely, without page reloads in JS, and the result was just as nice as it was without JS :)

meba’s picture

meba commented

subscribing

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@yched: yes, I meant JS turned off, sorry.

Steven’s picture

Steven commented

The 'HTTP 200 error' is because when Drupal.parseJson was removed, its graceful handling of unparseable JSON was replaced with, well, nothing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

OK, so then this might have other negative effects in Drupal elsewhere too. Do you have a suggestion. Someone remembers other parts of Drupal which were dependent on the graceful degradation of JSON parsing? I am not sure this was a good practice, as this kind of approach also put more strain on the server (not critical in this situation with bathes but in others reusing the same approach, it might be important).

yched’s picture

yched commented
StatusFileSize
new instal.php_.patch483 bytes

OK, 'HTTP error 200' is sure, er, nice, but I'll leave the side-effects of the removal of Drupal.parseJson for now.

About exit() and the patch in #4 : I'm not sure. Before that commit, _batch_do() used drupal_set_header() / drupal_to_js() / exit(). Then drupal_json() was added and I moved _batch_do() to use it, but noticed that other drupal_json() calls in core were not followed by exit(), so I removed it.

Other calls to _batch_page in core ('regular' batch page callback in system.admin.inc and update.php) both use 

if (isset($output)) {
  print theme('page', $output);
}

as a safeguard (as does index.php, for that matter)

Doing something similar in install.php as well would keep both drupal_json() and print theme('page') consistent with the rest of core, so I guess I'd vote for that.

Quick patch attached, no time to actually test tonight :-)

hass’s picture

hass commented

subscribe

hass’s picture

hass commented

I've got this error with JS activated, too.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

hass: all we get this error when JS is enaled, thats when JSON kicks into the picture.

yched: I looked into the install.php code and using isset() does not seem to be right there, because $output is initialized to an empty string. So !empty() would be better there. Or we can remove the init code and use $output = ... to start adding values to it everywhere. BTW only the batch code uses $output .= ... so this way can also be easily fixed. A small doc comment would also be nice there to note why we do this.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Assigned: Unassigned » gábor hojtsy
StatusFileSize
new install_json.patch1.3 KB

OK here is a quick patch which should work with the isset() now that output is not set before. Needs testing!

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

BTW forgot to mention that my testing shows that the installer translation import works perfectly now with this patch applied. Needs to be validated by someone else too. Test!

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Tested with the Hungarian package provided above. Works perfectly :-)

hass’s picture

hass commented

I'd like to test this in some hours, too. For not reinventing the test wheel - have you tried to move the POT files in subdirs? Is this working, well? I'm thinking about issue http://drupal.org/node/179234... haven't tested the installer import very much yesterday... maybe the install have same issues if a module pot file is inside a module translation subdir and i activate the module via module administration.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Well, hass, this bug has nothing to do with moving POT files around or whatever. Thankfully you opened another issue for the other bug, so we can solve them both. Thanks yched for testing! I committed this patch and looking forward to fixing the other issue soon. Let's squash these bugs!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
greenmother’s picture

greenmother commented

I has this error too and patch #13 don't help with it.
It should be something wrong with jquery 1.1.2, that used by Drupal 6 beta 1. Ajax query at misc/project.js:71 must be sended to http://localhost/drupal6/install.php?locale=ru&profile=ru&id=1&op=do, but really REQUEST_URI = "http://localhost/drupal6/install.php?profile=ru" at server in my case. I refresh jQuery in misc dir up to 1.2.1 version and all works right now. Hope, release of Drupal 6 will be with last version of jQuery.

(sorry for my stupid english)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Well, try the latest development version first please (http://drupal.org/node/97368) and report if it does not work in that version.