Fix generated php.ini config file

[theofidry]

My PHP setup may be bit unorthodox (I'm using phpbrew) in the sense that I have different files for the configuration. Because of how they are included, there is no redundancy. However in the case of the ConfigBuilder, it simply collects the different configurations and put everything in a single file. The issue is that it may result in duplicated lines:

 Output:                                                                                                             
  ================                                                                                                    
                                                                                                                      
  Warning: Module 'blackfire' already loaded in Unknown on line 0                                                     
                                                                                                                      
  Warning: Module 'iconv' already loaded in Unknown on line 0                                                         
                                                                                                                      
  Warning: Module 'pdo_sqlite' already loaded in Unknown on line 0                                                    
                                                                                                                      
  Warning: Module 'redis' already loaded in Unknown on line 0                                                         
  #!/usr/bin/env php                                                                                                  
  PHPUnit 6.0.13 by Sebastian Bergmann and contributors.  

So I would just make sure there is no duplicated lines in the dumped content :)

[theofidry]

Hm, actually this doesn't seem to fix the issue I have. It fixed something like that on Box but that's not enough here

[theofidry]

The issue is that I'm using phpbrew which configure the compiled PHP version:

php --ini                                                                   tfidry@Theos-MBP
Configuration File (php.ini) Path: /path/to/.phpbrew/php/php-7.2.2/etc
Loaded Configuration File:         /path/to/.phpbrew/php/php-7.2.2/etc/php.ini
Scan for additional .ini files in: /path/to/.phpbrew/php/php-7.2.2/var/db
Additional .ini files parsed:      /path/to/.phpbrew/php/php-7.2.2/var/db/ext-blackfire.ini,
/path/to/.phpbrew/php/php-7.2.2/var/db/iconv.ini,
/path/to/.phpbrew/php/php-7.2.2/var/db/pdo_sqlite.ini,
/path/to/.phpbrew/php/php-7.2.2/var/db/redis.ini,
/path/to/.phpbrew/php/php-7.2.2/var/db/xdebug.ini

What is happening is you take all the ini files and dump the content (removing xdebug on the way) to restart the process using that temporary ini file instead. However, because of my PHP configuration it scans fort the additional ini files again causing the modules saved above to be loaded twice causing an error.

Now according to the doc, it is possible to override that behaviour. So what needs to be done is also configuring PHP_INI_SCAN_DIR to /nowhere or something.

This is however where this gets tricky: I didn't find a way to do that in a manner working with passthru(). I tried several things: exec() to export the environment variable, putenv(), prefixing the $cmd executed with the export of the env variable. None of that works. However if I do:

export PHP_INI_SCAN_DIR=/nowhere; php --ini                                 tfidry@Theos-MBP
Configuration File (php.ini) Path: /path/to/.phpbrew/php/php-7.2.2/etc
Loaded Configuration File:         /path/to/.phpbrew/php/php-7.2.2/etc/php.ini
Scan for additional .ini files in: /nowhere
Additional .ini files parsed:      (none)

so that works.

Even more confusing, I don't have this issue with neither Composer or Box

Any idea @borNfreee?

[maks-rafalko]

However, because of my PHP configuration it scans fort the additional ini files again

Unfortunately, by just reading the code I don't understand why it scans again, because we reset PHP_INI_SCAN_DIR.

We need some input from @sidz here, I will touch base with him tomorrow ;)

[sidz]

@theofidry please double check that PHP_INI_SCAN_DIR is equal empty string. Also we use the same idea as composer could you please clone composer and try to run it with the same config?

[theofidry]

double check that PHP_INI_SCAN_DIR is equal empty string

It is. The INI files are scanned because their location has been passed with the --with-config-file-path option when PHP was being compiled. My environment variable PHP_INI_SCAN_DIR is not defined.

could you please clone composer and try to run it with the same config?

It works fine with Composer. Actually it works fine with Infection on both PHP-Scoper and Box so maybe this is a project issue. I'm trying to get Infection working on Symfony (the project, not a Symfony app)

[sidz]

if it will be possible for you to create a vagrant box for example I can take a look.

[theofidry]

I'll give it a try

[johnstevenson]

The documentation about PHP_INI_SCAN_DIR is confusing and incomplete. What it doesn't say is that if the environment variable is defined and its value is an empty string, then the engine will not scan for additional inis, even if compiled --with-config-file-scan-dir (incidentally, this value is available at runtime as the predefined constant PHP_CONFIG_FILE_SCAN_DIR).

Unfortunately, by just reading the code I don't understand why it scans again, because we reset PHP_INI_SCAN_DIR.

Sure and the process will restart fine. But when it restarts it sets the variable to its original value. However, you seem to have missed out providing this rather crucial bit of information for some reason, so instead the environment variable is always unset.

Now, this problem has come up before and it occurs when you call another php process from the restarted process and have the need to give it an ini file (ie php -c my.ini). In this case, I presume that the tmp ini file is being given to the process so the engine loads this, then follows the --with-config-file-scan-dir information (because PHP_INI_SCAN_DIR is now unset).

If you could explain your use case for using the tmp ini again, then I can perhaps come up with a way of incorporating https://packagist.org/packages/composer/xdebug-handler to make things a bit more robust.

[sidz]

@johnstevenson Sure, nothing special

If you could explain your use case for using the tmp ini again, then I can perhaps come up with a way of incorporating https://packagist.org/packages/composer/xdebug-handler to make things a bit more robust.

The main Infection process runs 2 types of the sub processes:

• generates code coverage report (we need to have debugger enabled)
• and to other processes where we need to disable debugger to speed up Infection performance.

How it works:
We run Infection with enabled xdebug. Our xDebugHandler will disable xdebug via custom php.ini (in the same way like you did in the https://packagist.org/packages/composer/xdebug-handler). But instead of removing the temporary file we save the absolute path to the environment variable as we need to use it in the second case which I've described above.

[sidz]

@johnstevenson I've investigated a bit (https://packagist.org/packages/composer/xdebug-handler) and seems like we need to have some kind of getter for the $tmpIni variable for our case

[johnstevenson]

@sidz Thanks for the info

we need to have some kind of getter for the $tmpIni variable for our case

You can get that now by calling XdebugHandler::getSkippedVersion(); For example:

if (XdebugHandler::getSkippedVersion()) {
    // We are in a restarted process
    $tmpIni = php_ini_loaded_file();
}

But I could possibly provide a method (say getRestartedIniFile()) that does that for you. Would this work? Sorry, I haven't had time to get take a look at how you do this.

[sidz]

thank you @johnstevenson for the feedback.

But I could possibly provide a method (say getRestartedIniFile()) that does that for you. Would this work? Sorry, I haven't had time to get take a look at how you do this.

👍 That is what we really need to

You can get that now by calling XdebugHandler::getSkippedVersion(); For example:

Your example will return the original path to the php.ini file but we need to get modified with disabled xdebug. From the Infection side we will need to do smth like this:

class XdebugHandler extends \Composer\XdebugHandler\XdebugHandler
{
    protected function restart($command)
    {
        putenv('INFECTION_TEMP_PHP_CONFIG_PATH=' . $this->getRestartedIniFile())

        passthru($command, $exitCode);

        exit($exitCode);
    }
}

So there here^ we've removed the condition with unlink as we will need to re-use the custom php.ini

[johnstevenson]

Your example will return the original path to the php.ini file but we need to get modified with disabled xdebug.

No it won't, it will return the modified tmp ini with xdebug entries disabled.

But I guess I need to understand the flow, so could you explain where the restart(s) occur based on your description:

The main Infection process runs 2 types of the sub processes:
generates code coverage report (we need to have debugger enabled)
and to other processes where we need to disable debugger to speed up Infection performance.

Thanks.

[johnstevenson]

Okay, as I see it (please correct me if I am wrong):

• the main process always restarts without xdebug, if it is loaded
• this can be overridden by setting INFECTION_DISABLE_XDEBUG (??) to a truthy value

So, if xdebug is loaded and INFECTION_DISABLE_XDEBUG is empty, then the library starts doing its thing in a restarted process. If you call php_ini_loaded_file() in a restarted process then you will get the tmp ini file that xdebug-handler created. This file will be available until the restarted process exits and the main process cleans up.

This ini file can be used to start a sub-process without xdebug, which is what you currently do, but PHP_INI_SCAN_DIR also needs to be set. Of course this could go horribly wrong if your sub-process calls another php process to a different script, because any required scan dir settings will be masked.

However, https://packagist.org/packages/composer/xdebug-handler can be used to emulate the current behaviour as it is, with a modification to PhpExecutableFinder::findArguments like:

    public function findArguments()
    {
        $arguments = [];
        $tempConfigPath = '';

        if (XdebugHandler::getSkippedVersion()) {
            $tempConfigPath = php_ini_loaded_file();              
            putenv('PHP_INI_SCAN_DIR=');            
        }

        if (!empty($tempConfigPath) && file_exists($tempConfigPath)) {
            $arguments[] = '-c';
            $arguments[] = $tempConfigPath;
        } elseif ('phpdbg' === PHP_SAPI) {
            $arguments[] = '-qrr';
        }

        return $arguments;
    }

Alternatively, you could save the tmp ini path to an environment variable in case the sub-process invokes another sub-process of itself (because XdebugHandler::getSkippedVersion() will only work in the restarted process).

[sidz]

@johnstevenson thank you for your time.

the main process always restarts without xdebug, if it is loaded

yes

this can be overridden by setting INFECTION_DISABLE_XDEBUG (??) to a truthy value

tbh I took this logic from https://github.com/composer/composer/blob/master/src/Composer/XdebugHandler.php#L63

Alternatively, you could save the tmp ini path to an environment variable in case the sub-process invokes another sub-process of itself (because XdebugHandler::getSkippedVersion() will only work in the restarted process).

Yeah, this is what I really want to do. But I think that we need to do this somewhere in the XdebugHandler instead of doing a trick in the PhpExecutableFinder. That is why I think it will be a good idea to have a getter for the $tmpIni variable and then we can override restart method of the original XdebugHandler.

[johnstevenson]

this can be overridden by setting INFECTION_DISABLE_XDEBUG (??) to a truthy value
tbh I took this logic from ...

I was actually questioning the renaming, from _ALLOW_XDEBUG to DISABLE_XDEBUG. It is not important, though it did confuse me a bit (not hard to do!)

That is why I think it will be a good idea to have a getter for the $tmpIni variable and then we can override restart method of the original XdebugHandler.

Why do you need to override the restart method?

[sidz]

I was actually questioning the renaming, from _ALLOW_XDEBUG to DISABLE_XDEBUG. It is not important, though it did confuse me a bit (not hard to do!)

Ah, not a big deal to rename.

Why do you need to override the restart method?
to save temporary php.ini to the env variable :) Of course we can do it somewhere else but I think it won't be obvious if for example we will do it in the PhpExecutableFinder like you mentioned above.

[johnstevenson]

When I suggested a possible getRestartedIniFile() I said it would be a wrapper around the XdebugHandler::getSkippedVersion() ... stuff, but this will not work if you override the restart method and use it there (as per your example code above).

So I've knocked that idea on the head because it is obviously confusing and could be misused.

I have every intention of making xdebug-handler more extendable in the future, but it is very new as a standalone package and I don't want to rush into anything at this stage, until we have more of an idea about what folks want to to with it.

Of course you can put the XdebugHandler::getSkippedVersion() ... stuff anywhere you want, but since you were already using findArguments (and not just for xdebug) it seemed as good a place as any.

A couple of points I noticed whilst debugging, which may or may not be useful:

• You cache the php binary in $this->cachedPhpPath but don't seem to use $this->cachedIncludedArgs (in AbstractTestFrameworkAdapter) so PhpExecutableFinder is called repeatedly for the same results.

• The VersionParser regex truncates a 2-digit patch version (ie phpunit 5.2.12 gets reported as 5.2.1).

[maks-rafalko]

The VersionParser regex truncates a 2-digit patch version (ie phpunit 5.2.12 gets reported as 5.2.1).

thanks for reporting, fixed d88d934

[johnstevenson]

@borNfreee Cool - apologies for the shortcut and not creating an issue. Nice library, by the way.

[sanmai]

Is this still an issue?

[theofidry]

Nope, if it is I'll open the issue again with the appropriate info