PHP Fatal error: Maximum execution time of 240 seconds exceeded in _locale_import_po

@ndobromirov Can you provide a link to one of the huge .po files so I can reproduce the issue please?

If anyone following this can find a large enough .po file on https://localize.drupal.org/ to test with that will be great.

Can this instead be solved in the same way as #2233929-21: drupal_set_time_limit should not be able to change the time limit if it's already unlimited?

Scratch that. It apparently already should be. Not sure why my import is failing with 240 seconds exceeded with max_execution_time set to unlimited. Will report back if I find anything.

Instead of making this configurable, it would be better to fix it directly.

Couldn't we do additional drupal_set_time_limit() calls inside _locale_import_read_po() itself, e.g. after it has processed each X lines of the file? That way the time limit would always be long enough. This would be in addition to (or instead of) the single call that's currently in _locale_import_po().

Also, is this an issue in Drupal 8? I think maybe not - and I vaguely remember seeing a similar issue to this one at one point, although perhaps it's not in the Drupal 7 queue.

Thanks for the explanation, @ndobromirov. But looking again at the drupal_set_time_limit() function (the code from the patch referenced in #8 is in 7.43), I'm still flummoxed as to why max_execution_time = 0 isn't being registered. The argument set in _locale_import_po should need to survive this check. I don't want to distract you from the task at hand, but if you don't mind humouring me for a sec it seems like something that's important to understand.

At the time I was using the web UI to import the file. I am running multiple versions of PHP on Windows, and was careful about targeting the correct .ini file (or at least I thought I was :)). But perhaps I am missing something embarrassingly obvious.

If you are doing the import from web-UI, then your PHP's original time limit is very likely non-zero and it is the default one...

Would you mind expanding on this?

(As a side note, I was able to bypass the problem with the innodb_flush_log_at_trx_commit fix suggested over at #1131048: Interface translation import times out during install.)

@ndobromirov - Thanks, I appreciate that. I do understand all of this, already, so unfortunately my circumstance (in a development environment where I'm not necessarily using defaults) remains a mystery. Moving on!... I'll see if I can dig up that *.po file again and give the patch a test in the next few days. Cheers -

@ndobromirov the patch #11 looks good to me.

But I think the comment can be improved.

+    // Refresh time limit every 10 parsed rows.
+    if (!($lineno % 10)) {
+      drupal_set_time_limit(30);
+    }

IMO, "Refresh time limit every 10 parsed rows." doesn't provide very useful information. (It just explains what the following lines do.) Isn't the original comment "Try to allocate enough time to parse and import the data." better here?

In testing this morning (Drupal 7.50) I attempted to import a 655KB file and predictably ran into the timeout.

With this patch I was able to successfully import the file.

Note: For this test I set innodb_flush_log_at_trx_commit = 1 in MySQL my.ini, which I do believe is the default value and makes innodb inserts/updates very slow. Setting this to '2' makes imports speedy enough on my machine to avoid the timeout, with or without the patch (which is why this fell off my radar - sorry about that).

Regarding @hgoto's critique, I have to disagree. "Try to allocate enough time to parse and import the data." says to me that we're going to guess a number and hope it's enough, which was entirely appropriate before. "... to parse and import the data" should be obvious (it's an import function). Given that there are people with varying levels of PHP proficiency here I think it's entirely useful to describe what those three lines are doing, and "Refresh time limit every 10 parsed rows." does that nicely.

Setting to RTBC.
In answer to David Rothstein's question in #10, the problem doesn't exist in Drupal 8 since batch api is used.

Committed to 7.x - thanks!

I think this is OK - it gives me slight pause that we're calling drupal_set_time_limit() with a smallish number, given the fact that in some scenarios this function can decrease the overall time limit, plus the fact that this function is called from other locations including st().... But since we're doing it every 10 rows, I think in practice the time limit will usually get increased enough not to affect other things.

For the code comment, I split the difference and did this on commit:

diff --git a/includes/locale.inc b/includes/locale.inc
index 69d9f06..11f1413 100644
--- a/includes/locale.inc
+++ b/includes/locale.inc
@@ -750,7 +750,8 @@ function _locale_import_read_po($op, $file, $mode = NULL, $lang = NULL, $group =
   $lineno = 0;
 
   while (!feof($fd)) {
-    // Refresh time limit every 10 parsed rows.
+    // Refresh the time limit every 10 parsed rows to ensure there is always
+    // enough time to import the data for large PO files.
     if (!($lineno % 10)) {
       drupal_set_time_limit(30);
     }