Review error message creation

While working on something else, I noticed a number of error messages in various sniffs using concatenation instead of passing variable parts of the error message to the add[Fixable]Error/Warning() methods via a $data array.

These are likely left-overs from long ago.

I believe it would be prudent to fix these up to follow the best practice of using %s placeholders and a $data array.

I seem to remember that even messages with placeholders can be changed from within a phpcs.xml[.dist] ruleset.
Before executing this action, it should be investigated whether updating the error messages would constitute a breaking change. And if so, it will need to be evaluated if this action needs to wait until the next major release of PHPCS or can be included in an earlier minor/patch version.

Action list

Action list based on what I came across (likely to be reasonably complete, though a critical look at all error messages in the sniffs involved is warranted):

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Line 419 in fcfefc6

$error = 'The closing parenthesis of a multi-line '.$type.' declaration must be on a new line';
PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Line 476 in fcfefc6

$error = 'Blank lines are not allowed in a multi-line '.$type.' declaration';
PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Line 492 in fcfefc6

$error = 'Multi-line '.$type.' declaration not indented correctly; expected %s spaces but found %s';
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/Classes/ClassDeclarationSniff.php

Line 213 in fcfefc6

$error = 'The '.$keywordType.' keyword must be on the same line as the %s '.$compareType;
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/Classes/ClassDeclarationSniff.php

Line 250 in fcfefc6

$error = 'Expected 1 space before '.$keywordType.' keyword; %s found';
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 72 in fcfefc6

$error = strtoupper($type).' keyword must be lowercase; expected "%s" but found "%s"';
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 103 in fcfefc6

$error = 'There must be no space before the colon in a '.strtoupper($type).' statement';
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 120 in fcfefc6

$error = 'The '.strtoupper($type).' body must start on the line following the statement';
PHP_CodeSniffer/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 174 in fcfefc6

$error = strtoupper($type).' statements must be defined using a colon';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 82 in c5e7f48

$error = strtoupper($type).' keyword must be lowercase; expected "%s" but found "%s"';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 95 in c5e7f48

$error = strtoupper($type).' keyword must be indented '.$this->indent.' spaces from SWITCH keyword';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 132 in c5e7f48

$error = 'There must be no space before the colon in a '.strtoupper($type).' statement';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 152 in c5e7f48

$error = 'Case breaking statement must be indented '.$this->indent.' spaces from SWITCH keyword';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Line 224 in c5e7f48

$error = 'Blank lines are not allowed after '.strtoupper($type).' statements';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php

Line 206 in c5e7f48

$error = 'Incorrect spacing between argument "%s" and equals sign; expected '.$this->equalsSpacing.' but found %s';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php

Line 240 in c5e7f48

$error = 'Incorrect spacing between default value and equals sign for argument "%s"; expected '.$this->equalsSpacing.' but found %s';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php

Line 191 in c5e7f48

$error = 'The first parameter of a multi-line '.$type.' declaration must be on the line after the opening bracket';
PHP_CodeSniffer/src/Standards/Squiz/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php

Line 238 in c5e7f48

$error = 'Multi-line '.$type.' declarations must define one parameter per line';

Before executing this action, it should be investigated whether updating the error messages would constitute a breaking change.

I looked into how PHPCS has handled error/warning message changes historically, and I don't think changes like the ones in #1240 have generally been treated as breaking changes.

There is historical precedent for this. Here are some examples:

PHPCS 2.3.2 changed the wording of PSR2.ControlStructures.SwitchDeclaration.WrongOpenercase to make the message clearer (commit 931842a).
PHPCS 3.3.0 changed the message format/data count for SpacingAfterOpen and SpacingBeforeClose, and documented that users who customized those messages should review their ruleset (commit 53a2840).
PHPCS 3.3.2 changed the Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma message to only have one data value instead of two (commit a06ff3b).

That said, there is a narrow ruleset compatibility concern for customized messages that should be taken into account.

A custom message containing an unescaped %, such as Custom 100% message, fatals with ValueError: Unknown format specifier when an existing message code that previously passed no $data starts passing $data. I'm not sure whether this edge case was considered in the past or not. Calling it out to help inform the decision on whether those changes should be considered breaking or not.

Given the historical handling above, I'm inclined to think the changes proposed here should not be treated as breaking changes. Any implementation should preserve existing rendered default messages where possible, and call out any newly introduced or changed message data contract in the changelog.

Wow, really useful information! I only did a quick search in git log, and since there are so many commits, I didn't dit into those earlier entries. :D

A custom message containing an unescaped %, such as Custom 100% message, fatals with ValueError: Unknown format specifier when an existing message code that previously passed no $data starts passing $data. I'm not sure whether this edge case was considered in the past or not. Calling it out to help inform the decision on whether those changes should be considered breaking or not.

~~#1358 is an example of this~~, it was tagged for 4.0.2, so I assumed that this kind of change wouldn't be considered a breaking change.

@rodrigoprimo

@rodrigoprimo Thanks for doing that research!

The examples you dug up, however, are not strictly comparable with what this ticket is about.

Commits 53a2840 (PHPCS 3.3.0) and a06ff3b (PHPCS 3.3.2) are loosely related, but remove placeholders, instead of adding them.

Looking at the above list, there will also be different considerations for messages which have no placeholders (only concatenation) and messages which currently have a mix of concatenation and placeholders.

I think updating messages which previously did not contain a placeholder should not be considered a breaking change.
A potential <message> parameter will contain a hard-coded message which will just overrule the new message, with the only risk being the ValueError mentioned by @rodrigoprimo, when an unescaped % is used in the hard-coded overrule message.
To me, this means that those type of changes should always get a changelog mention, but the chance of anything actually breaking is very slim as the % character is unlikely to occur much in overruled messages.

Updating messages which are currently a mix of placeholders and concatenation is a slightly different matter.
As long as the order of the placeholders remains as per the original message, I don't think it needs to be considered as a breaking change. The overruled message may miss some information (the new entry in $data), but as long as the placeholder order remains the same, the message should still make sense.
Again, this does need a changelog entry.

Changing the order of placeholders is a different matter as that will lead to overruled messages becoming garbled nonsense, so should be considered a breaking change.

Original msg: "Some rule; expected: %s, found %s."
Overruled msg: "Expected %s, but found %s for something"

New msg: "Some %3$s rule; expected: %1$s, found %2$s."
// When done like this, the overruled message will still make sense after the change.

As for changing error messages by removing placeholder(s) (the two above mentioned loosely related examples from PHPCS 3.3.x). That should be considered a breaking change as it will lead to an ArgumentCountError since PHP 8.0 for overruled messages which still have the originally expected number of placeholders.
I expect that these were okayed as non-breaking at the time because it was a warning in PHP 7.x, but with the change to an exception in PHP 8.0, I don't think that's acceptable anymore.
However, the breaking change can in these cases be avoided by not removing the placeholder and replacing the replacement value with an empty string. If there is reason for these type of changes in the future, I would suggest to also open an issue to clean up the message in the next major.

I hope this helps (and happy to hear counter-arguments if I missed something).

@ntdiary Note: The change made in #1358 is irrelevant to this discussion as it doesn't change the message for a pre-existing error code, but introduces a new error code instead.

@ntdiary

Closing as fully addressed by @ntdiary. The changes will all be included in the 4.0.2 release.

Navigation Menu

Repository navigation

Review error message creation #1240

Action list

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Review error message creation #1240

Description

Action list

Activity

rodrigoprimo commented on Jun 25, 2026

ntdiary commented on Jun 25, 2026

jrfnl commented on Jun 25, 2026

jrfnl commented on Aug 4, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions