TYPO3 Core

Hey Sebastiaan,

nice patch. Can you send it to the core newsgroup?

I made a new patch. which addresses max_execution_time as well. I've already sent it to the typo3-dev list.

--- typo3/sysext/impexp/class.tx_impexp.php 2007-12-14 15:08:55.000000000 0100
++ typo3/sysext/impexp/class.tx_impexp.php 2008-06-10 12:17:01.000000000 +0200
@ -179,8 +179,34 @
require_once (PATH_t3lib.'class.t3lib_extfilefunc.php');
require_once (PATH_t3lib.'class.t3lib_refindex.php');

@ini_set('max_execution_time',600);
@ini_set('memory_limit','256m');
if (ini_get('max_execution_time') < 600) {
@ini_set('max_execution_time',600);
}
// Set the memory_limit to a specified minimum (in MB)
$memory_minimum = 256;
$memory_limit = ini_get('memory_limit');
// -1 means no limit
+if ($memory_limit != -1) {
switch (strtolower(substr($memory_limit, -1))) {
+ case 'g': // Gigabytes
+ // Minimum is 1GB, so no need to raise it
+ break;
+ case 'm': // Megabytes
+ if (substr($memory_limit, 0, -1) < $memory_minimum) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ break;
+ case 'k': // Kilobytes
+ if (substr($memory_limit, 0, -1) < ($memory_minimum * 1024)) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ break:
+ default: // Just bytes
+ if (ctype_digit($memory_limit) && $memory_limit < ($memory_minimum * 1024 * 1024)) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ }
+}

Changed the patch according to some comments on the core list.

--- typo3_svn/typo3/sysext/impexp/class.tx_impexp.php 2008-06-10 12:41:18.000000000 0200
++ typo3_4.2/typo3/sysext/impexp/class.tx_impexp.php 2008-06-11 09:47:28.000000000 +0200
@ -179,8 +179,34 @
require_once (PATH_t3lib.'class.t3lib_extfilefunc.php');
require_once (PATH_t3lib.'class.t3lib_refindex.php');

@ini_set('max_execution_time',600);
@ini_set('memory_limit','256m');
if (ini_get('max_execution_time') < 600) {
@ini_set('max_execution_time',600);
}
// Set the memory_limit to a specified minimum in bytes
$memory_minimum = 256 * 1024 * 1024;
$memory_limit = ini_get('memory_limit');
// -1 means no limit
+if ($memory_limit != -1) {
switch (strtolower(substr($memory_limit, -1))) {
+ case 'g': // Gigabytes
+ // Minimum is 1GB, so no need to raise it
+ break;
+ case 'm': // Megabytes
+ if ((substr($memory_limit, 0, -1) * 1024 * 1024) < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ break;
+ case 'k': // Kilobytes
+ if ((substr($memory_limit, 0, -1) * 1024) < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ break;
+ default: // Just bytes
+ if (ctype_digit($memory_limit) && $memory_limit < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ }
+}

After reading the discussion in the core list twice, I made a patch to remove the init_set() completely.

Hi Peter,
would you send the patch to core list? The trunc version still using that 256 limit.

In my opinion, Peter is right and the ini_set() lines need to be removed completely.
Setting limits should never be part of a script or defined at some executed code at all. This is generally bad and ugly. Limits should always be defined "out-of-band". For this particular limits at php.ini, fastcgi parameters and/or ulimit settings. In general, a value of 256M or 10minutes of execution time is always a bad guess, as these values are individual and only project dependent. Even Sebastiaans patch which tries to avoid lowering limits can only be seen as a workaround and not a solution. Some things should definitely remain at system level and trying to consolidate them into a script will always result in errorrs which need to be worked around.
Some spice doesn't belong into soups, so PLEASE get off these ini_set() lines.

ini_set is bad in general and shouldn't be used.
In this case we need a different way writing the data to file - sequentially, without needing so much memory.
I try to come with a better solution in 4.5 that solves all the memory hassles

It would be nice, if that bug could be fixed anyway.

If the rewrite needs more time and cannot get into 4.5, the ini_set() would stay at is it. So please, remove it now.

Any news on this?

Did you check my other bugnote with a proper fix for this problem;
http://bugs.typo3.org/view.php?id=5045

It solves the problem like explained in this message on the core list:
http://lists.typo3.org/pipermail/typo3-team-core/2008-July/017914.html

I do not have very much time. But if people think this is the right way and offer some of their time to test (and review) the patch I would invest some more time - because until now almost two workdays have been invested in this issue and it simply gets ignored.

Bernhard, I really like your approach of writing chunks or segments or how you want to name it. Indeed, this should keep the memory footprint small even on huge exports. But I'm afraid, this won't get mature soon, so I'ld like to see the ini_set() statements removed as an interims solution anyway.

What do you mean by "not mature"? The code works and I used it for some quite large exports/imports without any problems?

The case is just there was quite a lot of "improve this, improve that" instead of something like "ok. let's use what you have and improve the rest later on" ...

I mean it works fully - when you read the whole thread I linked above you will notice that the concerns was: * The name of a variable * CGL issues * gzip compression

Martin Kutschker suggested I should compress each of the chunks on it's own and don't compress things like png's or other files which are already compressed - while my solution is to create the final T3D and run gzip over it (like it is currently the case).

So I stick to current behaviour, and made a fix for a problem. And my fix doesn't get accepted because some people suggest things I should also repair. But the memory issue and the handling of already compressed files are some completly DIFFERENT things.

greets,
Bernhard

Ok. I started updating the patch to current trunk version.
I will also add some unit-testing code so it is easier to check if everything works.
As soon as my updates are ready I will post them here and on the core list.

Patch set 2 of change I3e690eea1a079ed6cadd548230afc405e005b115 has been pushed to the review server.
It is available at http://review.typo3.org/2610