Use the current config

[theofidry]

In the case of Box for example, the PHP process may be restarted (via composer/xdebug-handler) with a different configuration file to disable xdebug among other things. Right now, it looks like the processes used are not using that configuration and as a result, even though the main process may not have xdebug enabled, the works will run with xdebug enabled.

It seems however that my patch is incomplete as it looks like some elements of the config are loaded twice, the following message is printed at the start of each worker:

PHP Warning:  Module 'sodium' already loaded in Unknown on line 0

[bwoebi]

Not sure, but don't we then need both -n (ignore default) and -c (use additionally)?

[theofidry]

I'm not sure either so I just aligned myself with what Composer XdebugHandler was doing

[kelunik]

I'm pretty sure -n is needed then.

[theofidry]

I've just tried it and indeed that solves the warning issue

[theofidry]

Any idea on why this is failing on Windows? I don't have a windows machine so I won't be able to do much debugging locally :/

[kelunik]

Don't worry, everything is currently failing on Windows, it's not related to this PR.

[sanmai]

@theofidry this happens because PHP loads not only the default php.ini, but all other files, which naturally include directive to load other module.

Try:

setenv("PHP_INI_SCAN_DIR=");

This should make all processes ignore those extra .ini files.

And yeah, -n does just that.

[sanmai]

If you want all child processes to exclude xdebug, this is also doable.

1. Put php.ini without xdebug into a directory (this is important), say:

    /tmp/xdebug-handler/php.ini
   

2. Run PHP setting two environment variables:

    PHP_INI_SCAN_DIR="" PHPRC="/tmp/xdebug-handler" php path/to/program.php
   

3. Now both program.php and all other PHP programs it starts will use your php.ini by default.

This is easy to verify:

$ mkdir /tmp/phprc
$ touch /tmp/phprc/php.ini
$ PHP_INI_SCAN_DIR="" PHPRC="/tmp/phprc" php -r 'echo `php --ini`;'
Configuration File (php.ini) Path: /usr/local/etc/php/7.2
Loaded Configuration File:         /tmp/phprc/php.ini
Scan for additional .ini files in: (none)
Additional .ini files parsed:      (none)

So PHP sees and loads only /tmp/phprc/php.ini

[theofidry]

Why not just adding -n -c xdebug-handler-tmp-cmd to the command?

[sanmai]

With -n -c if that command starts another command you can't control, it will run with xdebug.

With PHP_INI_SCAN_DIR="" PHPRC="/tmp/phprc" all PHP programs will use our xdebug-free php.ini. That unless they're directly told to use a different php.ini.

(I have not looked if this is particularly important in this case.)

[theofidry]

With -n -c if that command starts another command you can't control, it will run with xdebug.

I think that's fine. I mean that part is actually out of the scope of Amp it's more on the consumer like me how they want to integrate it with the xdebug handler.

However I'm more concerned about if this patch is safe or if it could break something. If it does break then I'll have to find an alternative way to handle it

[kelunik]

Any reason for not using php_ini_loaded_file()? Maybe also php_ini_scanned_files()? I don't think the implementation will break anything, but there's a case missing where no configuration file is present, e.g. PHP being executed with php -n.

[theofidry]

@kelunik I've tested with php -n and it works fine. $phpConfig will be false in that case so an empty string will be used.

Is there anything else concerning you? I'd like to include this patch in the next version of Box if possible

[sanmai]

xDebugHandler can be disabled. Best would be to check that this isn't the case.

[theofidry]

@sanmai is XDebugHandler really the issue here? What I see is that the worker is started with a different configuration than the main process, so if I pass a custom config it's just not gonna pick it up.

Note however that it is indeed only a patch not a real fix. For example if I call the main process with -d zend.gc_enable=0, the worker won't pick it up either...

I also tried to just not care and put XDebugHandler in the work process directly, but it's a +17% slowdown (box now takes 33s instead of 23s)

[sanmai]

php -n will certainly break things if XDebugHandler would happen to be disabled e.g. by setting MYAPP_ALLOW_XDEBUG. Checking for XdebugHandler::getSkippedVersion() should be enough, I think.

[theofidry]

Can you give an example? I've been testing with Box but it just works fine (I had to do php -n -d phar.readonly=0 though since otherwise it will fail before Amp is started).

Also an issue is that I don't have more control on the command used for the base process at all: it's deeper in Amp and there is not much of an extension point AFAICS

[kelunik]

There needs to be an escapeshellarg around the $phpConfig.

[kelunik]

That'll make -n -c $phpConfig one argument. The escapeshellarg needs to be only around the $phpConfig.

[sanmai]

Must be

"-n -c ".\escapeshellarg($phpConfig)

[sanmai]

Can you give an example?

Let's say a user chose to keep xdebug enabled, and you started a process with -n -c. Then the process will only see the standard php.ini, and won't see any additional .inis. Hence any additional extensions won't load, and the process will fail if it happens to need anything from side-loaded extensions.

For example, under Debian-based even very basic things like SimpleXML are not compiled in:

$ ls /etc/php/7.2/cli/conf.d/
10-mysqlnd.ini   20-ctype.ini     20-iconv.ini     20-mysqli.ini     20-sockets.ini    20-xmlreader.ini
10-opcache.ini   20-curl.ini      20-igbinary.ini  20-pdo_mysql.ini  20-sysvmsg.ini    20-xmlwriter.ini
10-pdo.ini       20-dom.ini       20-imap.ini      20-phar.ini       20-sysvsem.ini    20-xsl.ini
15-xml.ini       20-exif.ini      20-intl.ini      20-posix.ini      20-sysvshm.ini    20-zip.ini
20-ast.ini       20-fileinfo.ini  20-json.ini      20-readline.ini   20-tidy.ini       25-memcached.ini
20-bcmath.ini    20-ftp.ini       20-mbstring.ini  20-redis.ini      20-tokenizer.ini
20-bz2.ini       20-gd.ini        20-memcache.ini  20-shmop.ini      20-wddx.ini
20-calendar.ini  20-gettext.ini   20-msgpack.ini   20-simplexml.ini  20-xdebug.ini

[theofidry]

True but on another hand scanning for them would require to do something a la XdebugHandler which is an order or magnitude more complex and you cannot also leave it to the user since there is no extension point for it. Which is why I say it's a patch not a real fix

[sanmai]

You don't have to do anything other than checking that XDebugHandler did its job by looking at output from XdebugHandler::getSkippedVersion(). If it did, then you're safe to use php -n -c, else fallback to a plain php with no arguments.

[theofidry]

@sanmai but then parallel would have a XdebugHandker only scenario. @kelunik are you fine with that?

But even with that, the core of the issue is not solved, the worker won't take the settings passed to the main process with php -d

[kelunik]

I'll need to test the patch with my system library. Usually I use a custom compiled one that has almost everything built-in.

[theofidry]

Any progress @kelunik or something I can help with?

[sanmai]

It appears to me Parallel should not be doing anything like this. Like, at all.

If a user needs to use a custom php.ini from XdebugHandler, it is as simple as:

putenv('PHPRC', XdebugHandler::getRestartSettings()['tmpIni']);
putenv('PHP_INI_SCAN_DIR', '');

Nothing else needs to be done to run all sub-processes without xdebug, other than adding the two above lines.

Work is underway in composer/xdebug-handler#67 to abstract away even these details.

[theofidry]

@sanmai ah I see what you mean. Yeah indeed that looks better thanks!

I think I'll have to adjust some things in Box to restart the process with XdebugHandler in any case to ensure the workers are started with the same configuration as the main process (xdebug or not). Maybe this should be done in parallel instead, but in any case this patch is not gonna cut it anyway

[kelunik]

@theofidry Sorry for not getting back to you on this topic. I'm glad to hear that the proposed solution probably works for you without changes on our side. #47 (comment) is an important issue with -n -c, which would need some attention if we'd modify the current way in that direction.

[theofidry]

No problem @kelunik