Skip to content

Generic/LineLength: use different warning and errors for namespaced names #1367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rjd22 wants to merge 2 commits into
base: 4.x
Choose a base branch
from
Open

Generic/LineLength: use different warning and errors for namespaced names #1367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e92d838
feat: Use different warning and errors for namespaced names
rjd22 Feb 4, 2026
31c7c05
Only throw namespace error and warning if line cannot be shortened
rjd22 Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions src/Standards/Generic/Sniffs/Files/LineLengthSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,21 +177,62 @@ protected function checkLineLength(File $phpcsFile, array $tokens, int $stackPtr
if ($this->absoluteLineLimit > 0
&& $lineLength > $this->absoluteLineLimit
) {
$code = 'MaxExceeded';
if ($this->isNamespacedNameExceedingLength($tokens, $stackPtr, $this->absoluteLineLimit) === true) {
$code = 'NamespacedNameMaxExceeded';
}

$data = [
$this->absoluteLineLimit,
$lineLength,
];

$error = 'Line exceeds maximum limit of %s characters; contains %s characters';
$phpcsFile->addError($error, $stackPtr, 'MaxExceeded', $data);
$phpcsFile->addError($error, $stackPtr, $code, $data);
} elseif ($lineLength > $this->lineLimit) {
$code = 'TooLong';
if ($this->isNamespacedNameExceedingLength($tokens, $stackPtr, $this->lineLimit) === true) {
$code = 'NamespacedNameTooLong';
}

$data = [
$this->lineLimit,
$lineLength,
];

$warning = 'Line exceeds %s characters; contains %s characters';
$phpcsFile->addWarning($warning, $stackPtr, 'TooLong', $data);
$phpcsFile->addWarning($warning, $stackPtr, $code, $data);
}
}


/**
* Checks if a line is a namespaced name

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the method changed the description needs to be updated.

*
* @param array $tokens The token stack.
* @param int $stackPtr The last token on the line.
* @param int $lineLimit The line limit.
*
* @return bool
*/
private function isNamespacedNameExceedingLength(array $tokens, int $stackPtr, int $lineLimit): bool

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest naming $stackPtr to something else that better describes what this variable holds. $stackPtr is typically used to refer to the position of the token that triggered the execution of the sniff code in the $tokens array. In the case of this sniff, checkLineLength() already uses this variable name to refer to something else, which I find confusing, but maybe is the reason why you opted to use this name in this method as well.

{
$line = $tokens[$stackPtr]['line'];

for ($i = $stackPtr; $tokens[$i]['line'] === $line; $i--) {

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of an edge case, but I think the code should protect against it. Consider the following code (note that the code is on the first line together with the PHP opening tag):

<?php $result = doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, \Short\Ns\myFunction($arg7));

It generates two warnings as this loop tries to access $tokens[-1]:

Undefined array key -1
Trying to access array offset on null

$length = ($tokens[$i]['column'] + $tokens[$i]['length'] - 1);

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still not correct. The code checks whether the token's end column position exceeds $lineLimit, rather than checking if the token's own length exceeds $lineLimit. This means that something like the code below will trigger the NamespacedNameTooLong warning, while my understanding from the discussion in the issue is that it shouldn't:

$result = doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, \Short\Ns\myFunction($arg7));

I believe the code should check simply if the length of the current token exceeds $lineLimit when the token is one of the namespaced name tokens.

@rjd22 rjd22 Feb 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have suggestions on how to deal with this? Since things like the following might also be a situation where we want to trigger the NamespacedNameTooLong warning:

$foobarVariableName = new \Long\Ns\Long\Ns\Long\Ns\Long\Ns\Long\Ns\Long\Ns\Long\Ns\Long\Ns($arg)

That was the reason for checking the column length.

@rodrigoprimo rodrigoprimo Feb 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of @jrfnl's suggestion is that the code sample you provided should not trigger the NamespacedNameTooLong warning. Instead, it should trigger the regular TooLong warning. Since it is not a single token that is causing the line to go over the length limit, the developer can still shorten the line by restructuring the code. The NamespacedName* codes should be reserved for cases where the namespace token alone exceeds the limit, as those are the truly "unfixable" cases where the developer has no option but to suppress the warning.

Does that help clarify things? What do you think?


// Check the line limit of the namespaced name is equal or over the line length. This check accounts for the
// fact that namespaced names are or closed by ; or opened by ( or {.
Comment on lines +225 to +226

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't seem accurate.

@rjd22 rjd22 Feb 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honest question: What is not accurate about it?

@rodrigoprimo rodrigoprimo Feb 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing/misunderstanding something, but to me it reads a bit confusingly (and it could be because I'm not a native English speaker). What is the "line limit of the namespaced name"? I guess the check is against the "line length limit" and not the "line length". And as far as I can understand, the second sentence isn't accurate since namespaced names can appear in many more contexts beyond ;, (, or {, and as far as I can see, what tokens come before or after namespaced name tokens doesn't seem relevant here.

That said, if the logic changes per the other discussion, this comment will need to be rewritten anyway, so no need to worry about it for now.

if ($length >= $lineLimit
&& ($tokens[$i]['code'] === T_NAME_QUALIFIED
|| $tokens[$i]['code'] === T_NAME_FULLY_QUALIFIED
|| $tokens[$i]['code'] === T_NAME_RELATIVE)
) {
return true;
}
}

return false;
}
}
45 changes: 45 additions & 0 deletions src/Standards/Generic/Tests/Files/LineLengthUnitTest.5.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

@rodrigoprimo rodrigoprimo Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts on the tests:

Some of the test cases seem redundant to me. For example, lines 13-15 test use, use function, and use const, but from the sniff's perspective, all three produce the same T_NAME_QUALIFIED token and exercise the same code path. From my perspective, just one of those tests would be sufficient. This also applies to other test "groups" in this file.

On the other hand, some scenarios that exercise different code paths are missing. For example:

  • Tests using the T_NAME_RELATIVE token. The sniff checks for this type, but no test covers it.
  • A namespace token whose length is exactly at the $lineLimit boundary (if there is indeed no test that matches the $lineLimit boundary, I might have missed it).

Maybe there could be more variation as well using namespace tokens in other contexts (like type hints, instanceof, or catch blocks).


// This is just within the limit
namespace This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLongLongLongLoong;

// This is just within the absolute limit and should throw a NamespacedNameTooLong warning
namespace This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLongLongLongLongLongLongLongLongLongLongLongLongLoooooong;

// This is too long and should throw a NamespacedNameMaxExceeded error
namespace This\Name\Space\IsGoingToBeTooLong\LongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLoooong;

// These are just within the limits
use ThisReallyLong\Looooooooooooooong\UseStatement\ShouldBeNotTooLong\ClassName;
use function ThisReallyLong\Looong\UseStatement\ShouldBeNotTooLong\functionCall;
use const ThisReallyLong\Loooooooooong\UseStatement\ShouldBeNotTooLong\CONSTANT;

// These are just a bit too long and should throw a NamespacedNameTooLong warning
use ThisReallyLong\Long\Looooong\Looooooooooooooong\UseStatement\ThatShouldThrowAWarning\BecauseItsNotTooLong\ClassName;
use function ThisReallyLong\Long\Looooooong\Long\UseStatement\ThatShouldThrowAWarning\BecauseItsNotTooLong\functionCall;
use const ThisReallyLong\Long\Looooong\Loooooooooong\UseStatement\ThatShouldThrowAWarning\BecauseItsNotTooLong\CONSTANT;

// These are just a bit too long and should throw a NamespacedNameMaxExceeded error
use ThisReallyLong\Long\Loooooong\Looooong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\ClassName;
use function ThisReallyLong\Long\Looong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\functionCall;
use const ThisReallyLong\Long\Loooooooooong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\CONSTANT;

// These are just within the limits
echo \This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLooong\functionCall();
new \This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLongLooong\ClassName();

// These are just a bit too long and should throw a NamespacedNameTooLong warning
echo \ThisReallyLong\Long\Looooong\Loooooooong\UseStatement\ThatShouldThrowAWarning\BecauseItsNotTooLong\functionCall();
new \ThisReallyLong\Long\Looooong\Loooooooooooong\UseStatement\ThatShouldThrowAWarning\BecauseItsNotTooLong\ClassName();

// These are just a bit too long and should throw a NamespacedNameMaxExceeded error
echo \ThisReallyLong\Long\Long\Looooong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\functionCall();
new \ThisReallyLong\Long\Long\Looooooooong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\ClassName();

// These are a bit too long but can be shortened and should throw a TooLong warning
echo \This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLooong\functionCall('foo', 'bar', 'baz');
new \This\Name\Space\IsGoingToBeJustWithinLimits\LongLongLongLooong\ClassName('foo', 'bar', 'baz');

// These are just a bit too long but can be shortened and should throw a MaxExceeded error
echo \ThisReallyLong\Long\Long\Looong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\functionCall('foo', 'bar', 'baz');
new \ThisReallyLong\Long\Long\Looooooong\UseStatement\ThatShouldNotBeAllowed\AndThrowErrorsBecauseItsTooLong\ClassName('foo', 'bar', 'baz');
22 changes: 22 additions & 0 deletions src/Standards/Generic/Tests/Files/LineLengthUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ public function getErrorList($testFile = '')
case 'LineLengthUnitTest.2.inc':
case 'LineLengthUnitTest.3.inc':
return [7 => 1];
case 'LineLengthUnitTest.5.inc':
return [
10 => 1,
23 => 1,
24 => 1,
25 => 1,
36 => 1,
37 => 1,
44 => 1,
45 => 1,
];

default:
return [];
Expand Down Expand Up @@ -103,6 +114,17 @@ public function getWarningList($testFile = '')
10 => 1,
14 => 1,
];
case 'LineLengthUnitTest.5.inc':
return [
7 => 1,
18 => 1,
19 => 1,
20 => 1,
32 => 1,
33 => 1,
40 => 1,
41 => 1,
];

default:
return [];
Expand Down