Repository navigation
Conversation
was added new function to php_ini_scanned_files();
|
i think its safer to use |
|
Hi @divinity76 , how are you? |
| marcosptf - <marcosptf@yahoo.com.br> - @phpsp - sao paulo - br | ||
| --SKIPIF-- | ||
| <?php | ||
| if (phpversion() < "5.3.0") { |
There was a problem hiding this comment.
IMO there's no need to check for PHP versions < 5.3.0, because the test is supposed to be included in PHP 7.1 upward only.
There was a problem hiding this comment.
okey, its true!
ill fix it asap!
|
@divinity76 @cmb69 |
|
Test seems fine to me @marcosptf |
|
@KalleZ The test only verifies that php_ini_scanned_files() returns a string, but not that the content of the string makes any sense. In my opinion, a rather weak test. |
|
@KalleZ @cmb69 in my opinion is impossible check each file that this function return because it can change in a server or environment different's. because this, i just see what type this function return and don't the values, like the docs said: |
|
@cmb69 I do agree, but like @marcosptf said then it can maybe be a little more trickier to get a consistent output, especially if you run with a system ini (I don't think run-tests.php passes -n to php by default, no?) which means that you would have to do something like: foreach(explode(',', php_ini_scanned_files()) as $ini) {
$ini = trim($ini);
// ... $ini ...
}And still get variable results, so despite it being "weak", I do think its fair enough to put in for coverage |
|
AFAIK run-tests.php sets up a controlled environment (using a temporary ini file) before actually running the test. And there is the --ARGS-- section which could be used. I'll have a closer look at this later today, if nobody beats me to it. |
|
@cmb69 ah brilliant, I totally forgot about the --ARGS-- section :) |
|
Just had a closer look at it. Actually, we would need to set the PHP_INI_SCAN_DIR environment variable to test php_ini_scanned_files. According to the docs we could set it to $filepath, but the documentation is completely wrong. The --ENV-- section actually contains lines in an ini style, and no replacement is done. :-( So I suggest to
|
|
Ah, it appears that the docs is correct for server-tests.php. Shouldn't both server-tests.php and run-tests.php understand the same format? :-/ |
|
I don't think server tests is even operational? On Oct 27, 2016 16:12, "Christoph M. Becker" notifications@github.com
|
|
As has already been pointed out, this is a weak test: We could change the behaviour of this function considerably and so long as it returns a string, the test would pass. That's not useful. Since this doesn't add any value whatsoever, I'm closing this PR. If you feel I am wrong to do that, please open a clean PR with a sensible test, one that actually does add value to our testing suite. Superficial coverage is not useful for us, and only serves to slow our CI down. |
Not regarding the CGI section might even been seen as a bug, and since server-tests.php appears to broken, anway[1][2], we implement it for run-tests.php in the way as described[3] for server-tests.php, i.e. respective tests are skipped if no CGI executable is found. [1] <#222 (comment)> [2] <#1811 (comment)> [3] <https://qa.php.net/phpt_details.php#cgi_section>
6 participants
was added new function to php_ini_scanned_files();