Ruleset::explain(): refactor method to remove use of output buffers + add test #118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… add test As pointed out in 3876 / 84, the `Ruleset::explain()` method was using output buffers incorrectly - it contained three `ob_start()` calls and only one `ob_end*()` call, which meant that at the end, there would be at least one unclosed output buffer. The (incorrect) use of the output buffers also meant that any test calling this method would be marked as risky with a "Test code or tested code did not (only) close its own output buffers" notice. In my opinion, the use of output buffers is unnecessary for the "explain" command, so I've refactored the code to remove the use of output buffering completely. The actual function output remains the same, with only a tiny difference - where previously, the separator marker line for a standard of which only 1 sniff was used would be 1 dash too long, the separator marker line will now match the length of the title line for the standard (tiny bug fix). This change now allows for the method to be tested, so a test has been added as well. The test uses the PSR1 standard as: 1. That standard is not expected to change anymore, which should make this test quite stable. 2. That standard has "sub-standards" with both 1 as well as "more than 1" sniffs, meaning the output of the subtitles can be tested. While it may seem redundant to have a test for this code as this is not code which changes often, the deprecation notice fixed in 3876 / 84 would have been caught by this test, while now it was only by the luck of the draw of me running `phpcs -e` while on PHP 8.3 (early) that we found that deprecation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As pointed out in squizlabs/PHP_CodeSniffer#3876 / #84, the
Ruleset::explain()method was using output buffers incorrectly - it contained threeob_start()calls and only oneob_end*()call, which meant that at the end, there would be at least one unclosed output buffer.The (incorrect) use of the output buffers also meant that any test calling this method would be marked as risky with a "Test code or tested code did not (only) close its own output buffers" notice.
In my opinion, the use of output buffers is unnecessary for the "explain" command, so I've refactored the code to remove the use of output buffering completely.
The actual function output remains the same, with only a tiny difference - where previously, the separator marker line for a standard of which only 1 sniff was used would be 1 dash too long, the separator marker line will now match the length of the title line for the standard (tiny bug fix).
This change now allows for the method to be tested, so a test has been added as well.
The test uses the PSR1 standard as:
While it may seem redundant to have a test for this code as this is not code which changes often, the deprecation notice fixed in squizlabs/PHP_CodeSniffer#3876 / #84 would have been caught by this test, while now it was only by the luck of the draw of me running
phpcs -ewhile on PHP 8.3 (early) that we found that deprecation.Suggested changelog entry
N/A (internal change only)