Don't include trailing newline in comment token

[nikic]

Don't include trailing newline for # and // style comments in T_COMMENT tokens. Instead leave it to the following T_WHITESPACE.

Logically the newline doesn't belong to the comment, and having it there adds the need for ugly special-cases in tooling (as pretty much nothing else includes a trailing newline -- the opening tag and heredocs do, but due to the way they work it's less problematic there.)

[TysonAndre]

This seems like a reasonable improvement and should work in edge cases (e.g. files without trailing newlines).

I'd assume any tools using token_get_all() are already handling whitespace after comment tokens (e.g. tolerant-php-parser, php-parser, php_codesniffer).

It'd be worth checking if PHP_CodeSniffer and phpcbf continued to work and file bug reports if any of the sniffs/unit tests break (I haven't tried that), but I'd consider those to be bugs in the application.

[carusogabriel]

cc @gsherwood @jrfnl @kukulich @SpacePossum @julienfalque @keradus

[jrfnl]

cc @sebastianbergmann re php-token-stream

[jrfnl]

This seems like a sane improvement to me, improving the consistency of tokenization.

It'd be worth checking if PHP_CodeSniffer and phpcbf continued to work and file bug reports if any of the sniffs/unit tests break (I haven't tried that), but I'd consider those to be bugs in the application.

I expect unit test will start breaking as PHPCS does not have any special handling of // comments.
I wouldn't consider that a bug in the application at this time.
All the same, if this PR gets accepted and the tokenization changes in PHP, it won't be hard to backfill this in PHPCS one way or another.

[gsherwood]

All the same, if this PR gets accepted and the tokenization changes in PHP, it won't be hard to backfill this in PHPCS one way or another.

If it is accepted, PHPCS would need to re-attach the newline to the comment token - to effectively reverse this change - in order to maintain backwards compatibility with existing sniffs.

If PHPCS backfilled this new behaviour to older PHP versions by splitting the token, any sniffs that rely on this existing structure would need to be modified, and that would be considered a serious breaking change. I'd need to reserve that for a major release.

Personally, I found the inclusion of the newline strange when I first encountered it, but not overly problematic. I'd be just as happy leaving it as-is.

[sebastianbergmann]

@jrfnl Thanks for the ping.

[julienfalque]

PHP CS Fixer already does this internally so I don't think it would be a big deal to be compatible with the change proposed here.

[SpacePossum]

Thanks for the ping @carusogabriel !
As Julien stated; for PHP-CS-Fixer the change it is not an issue (luckly :))