Skip to content

Commit 09a96d9

Browse files
committed
Config: show user friendly error message when ini change failed
Inspired by discussion 415. Previously, when a PHP ini option which cannot be changed at runtime was passed to PHPCS, it would be silently ignored (by PHP, PHPCS would still try to handle it, but would not report that PHP did not change the value). This commit changes that behaviour by adding a new "ERROR: Ini option %s cannot be set at runtime" error to alert the end-user to the fact that they are passing a PHP ini option which PHPCS cannot change. The new error will be thrown both when the user passes the invalid ini setting via the command line, as well as when it is passed via a custom ruleset. The behaviour when trying to change an ini setting which _doesn't exist_ (typo, extension not available) is unchanged. In that case, the ini directive will still be silently ignored. Includes unit tests to safeguard the new behaviour. Closes 416 Also note: when this error occurs due to an invalid setting being passed via a ruleset, the error will be thrown directly and not collected via the `MessageCollector`. This is due to the error coming from the `Config` class. Once the `MessageCollector` would be implemented in the `Config` class, this can potentially be changed.
1 parent 8ac3cad commit 09a96d9

File tree

8 files changed

+534
-5
lines changed

8 files changed

+534
-5
lines changed

.github/workflows/test.yml

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,15 @@ jobs:
6565
- php: '8.4'
6666
skip_tests: true
6767

68+
# Run a couple of builds with custom extensions to allow for testing ini handling within PHPCS.
69+
# Ref: https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/416
70+
- php: '7.3'
71+
os: 'ubuntu-latest'
72+
extensions: ':mysqli' # Run with mysqli disabled.
73+
- php: '8.2'
74+
os: 'ubuntu-latest'
75+
extensions: ':mysqli' # Run with mysqli disabled.
76+
6877
# The default libxml library on Ubuntu images is a little out of date.
6978
# To safeguard support for the latest libxml we need to update the library on the fly.
7079
# This only needs to be tested with one PHP version for each libxml minor to verify support.
@@ -183,6 +192,7 @@ jobs:
183192
with:
184193
php-version: ${{ matrix.php }}
185194
ini-values: ${{ steps.set_ini.outputs.PHP_INI }}
195+
extensions: ${{ matrix.extensions }}
186196
coverage: none
187197
tools: cs2pr
188198

@@ -269,10 +279,12 @@ jobs:
269279
custom_ini: [false]
270280

271281
include:
272-
# Also run one coverage build with custom ini settings.
282+
# Also run one coverage build with custom ini settings for testing the DisallowShortOpenTag sniff.
283+
# Also run with a disabled extension for testing the handling of unsettable ini settings by PHPCS.
273284
- php: '8.1'
274285
os: 'ubuntu-latest'
275286
custom_ini: true
287+
extensions: ':mysqli' # Run with mysqli disabled.
276288

277289
# yamllint disable-line rule:line-length
278290
name: "Coverage: ${{ matrix.php }} ${{ matrix.custom_ini && ' with custom ini settings' || '' }} (${{ matrix.os == 'ubuntu-latest' && 'Linux' || 'Win' }})"
@@ -297,6 +309,7 @@ jobs:
297309
with:
298310
php-version: ${{ matrix.php }}
299311
ini-values: error_reporting=-1, display_errors=On${{ steps.set_ini.outputs.PHP_INI }}
312+
extensions: ${{ matrix.extensions }}
300313
coverage: xdebug
301314

302315
# Install dependencies and handle caching in one go.

src/Config.php

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -739,10 +739,23 @@ public function processShortArgument($arg, $pos)
739739
case 'd' :
740740
$ini = explode('=', $this->cliArgs[($pos + 1)]);
741741
$this->cliArgs[($pos + 1)] = '';
742-
if (isset($ini[1]) === true) {
743-
ini_set($ini[0], $ini[1]);
744-
} else {
745-
ini_set($ini[0], true);
742+
if (isset($ini[1]) === false) {
743+
// Set to true.
744+
$ini[1] = '1';
745+
}
746+
747+
$current = ini_get($ini[0]);
748+
if ($current === false) {
749+
// Ini setting which doesn't exist, or is from an unavailable extension.
750+
// Silently ignore it.
751+
break;
752+
}
753+
754+
$changed = ini_set($ini[0], $ini[1]);
755+
if ($changed === false && ini_get($ini[0]) !== $ini[1]) {
756+
$error = sprintf('ERROR: Ini option "%s" cannot be changed at runtime.', $ini[0]).PHP_EOL;
757+
$error .= $this->printShortUsage(true);
758+
throw new DeepExitException($error, 3);
746759
}
747760
break;
748761
case 'n' :

0 commit comments

Comments
 (0)