[remicollet]

Failing since b9f4113

TEST_PHP_EXTRA_ARGS is -n -c '/work/build/php84/tmp-php.ini' ...

So the test is failing with

001- bool\(false\)
002- string\(\d+\) ".*[\/\\]loaded\.ini"
001+ string(29) "/work/build/php84/tmp-php.ini"
002+ string(56) "/work/build/php84/ext/standard/tests/ini_info/loaded.ini"

[remicollet]

Ping @orlitzky and @Girgias for commit raising this failure

[orlitzky]

sorry :\

Maybe there's a way we can make it work for both of us?

• Should -n override the earlier -c?
• If not, could we look for -c in the $args and skip the test?

We need the extra args on Gentoo so that we can tell php not to use the system's php.ini when testing a newly-built (but incompatible) version of php. Otherwise we get failures like

---- ACTUAL OUTPUT
Failed loading /usr/lib64/php8.2/lib64/opcache.so:
/usr/lib64/php8.2/lib64/opcache.so: undefined symbol: _emalloc_32
int(111)
---- FAILED

and the user is prevented from upgrading. (That's an old example, but the same thing happens when e.g. switching from a debug build to a non-debug build.)

[remicollet]

Should -n override the earlier -c?

It doesn't

$ php -n -c  /etc/php.ini -n -r 'var_dump(php_ini_loaded_file());'
string(12) "/etc/php.ini"

This test is only to ensure that

• php -n loads no file
• php -n -c foo.ini loads foo.ini

TEST_PHP_EXTRA_ARGS has no value in this test (and it breaks it as it brings a -c option)
Please test this change, I think it will not change anything for you

when testing a newly-built (but incompatible) version of php

This problem exists since ever...
One of the reasons why building in a safe env. is necessary for safety

[remicollet]

I think your problem is https://github.com/php/php-src/blob/master/build/Makefile.global#L96

	INI_FILE=`$(PHP_EXECUTABLE) -d 'display_errors=stderr' -r 'echo php_ini_loaded_file();' 2> /dev/null`; \

It is useful for extension but bad for php itself

$ pwd
/work/build/php86          <==  this is the build directory
$ sapi/cli/php  -r 'echo php_ini_loaded_file();'
/opt/php86/etc/php.ini   <== this is the installation directory

Perhaps

if [ -f php.ini-development ]; then
  # in php-src, use development configuration
  INI_FILE=php.ini-development
else
  # for extension use installed configuration
  INI_FILE=`$(PHP_EXECUTABLE) -d 'display_errors=stderr' -r 'echo php_ini_loaded_file();' 2> /dev/null`;
fi

P.S. but IMHO changing this is not suitable for stable branches

P.S.2: there is also an issue with installed extensions (in extension_dir) which can be incompatible with the build engine

[orlitzky]

We're not using make test to run tests: https://github.com/gentoo/gentoo/blob/master/dev-lang/php/php-8.5.12.ebuild#L745

There may be a way to work around it for this one test, for this one failure, but wouldn't it be better if the TEST_PHP_EXTRA_ARGS were always passed to later invocations of PHP as documented? IMO this test has found a bug, namely that -c tmp-php.ini -n will still try to load tmp-php.ini. Later flags almost always override earlier ones, but in this case there are two separate flags (nothing we can do about it now) for the same option, so the arg parser misses the override.

[orlitzky]

I've thrown my entry into the ring at #25261. In main/php_ini.c we're doing

if (sapi_module.php_ini_path_override) {                                                                                                               
    ...                                                                                                                
} else if (!sapi_module.php_ini_ignore) {                                                                                                              
    ...
}

which is why the -n is ignored in the presence of -c. But fixing it in the CLI argument parser was not too hard, we just have to unset the earlier option when a new one is encountered.

[remicollet]

Can we concentrate on the very small change in this PR (other problems should be discussed somewhere else)

The test check that -n option doesn't parse any file
So "$php -n -r $code" is enough

[orlitzky]

The test check that -n option doesn't parse any file

Except this is not true, as the failing test now shows. I would prefer not to have to disable this test and come back and fix it again in three years, but I don't want to argue forever about something that can be fixed with a single rm on my end.

[derickr]

With -n, there should be no parsed ini file — unless you then add that back in again with -c. That seems to be what this test is about, and hence, this is a good patch. I don't think either of the two asserts in here should have brought in $args.

[bukka]

I agree with Derick and Remi here.

[remicollet]

Thanks for the review

Merged in 8.4+