View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0034400 | mantisbt | code cleanup | public | 2024-04-08 14:20 | 2024-04-22 04:14 |
Reporter | hotzeplotz | Assigned To | |||
Priority | normal | Severity | feature | Reproducibility | have not tried |
Status | confirmed | Resolution | open | ||
Target Version | 2.27.0 | ||||
Summary | 0034400: Introduce a cs fixer | ||||
Description | Recent sample: https://github.com/mantisbt/mantisbt/pull/1989#discussion_r1554994663 This could have been avoided with: Check …
… and fix
Introducing a cs fixerThis work is based on the previous work of @dregad with the PHP_CodeSniffer and follows the MantisBT coding guidlines I use "easy coding standard" as the tool because I am reasonably experienced with it and it allows to combine the rules/fixers from PHP Coding Standards Fixer and the PHP_CodeSniffer. How to goMy suggestion is makeing a series of many very small fixes, rule by rule and/or file by file, to keep the changes reviewable. Means keep control over unwanted changes. For example: Starting with a basic fixer like "FullOpeningTagFixer" or "SingleBlankLineAtEofFixer" and going ahead with the next uncritial fixer to evolve gradually until the more "critial" fixers are applied. Which Fixers/RulesThe fixers based on this "ruleset.xml". As mentioned esc allows both cs-fixer and sniffs, but while working on this, i came across, that the most fixers are available as php-cs-fixers. When there are direct replacements, i used the ones from cs-fixer. Also i haven't found a nice overview with samples for the sniffer as the one for the cs-fixer, so it was more convienced using them. RulesSome i don't know, others are fixed "automatically" by a already used fixer. Its always time to figure it out in detail.
ControlStructureBracesFixer: https://cs.symfony.com/doc/rules/control_structure/control_structure_braces.html
SpacesInsideParenthesesFixer ('space' => 'single'): https://cs.symfony.com/doc/rules/whitespace/spaces_inside_parentheses.html
Seems good with:
Todo: don't know what they do.
Not sure, is it https://cs.symfony.com/doc/rules/function_notation/method_argument_space.html#example-5?
Not sure. Maybe its already fixed with another applied fixer. eg. "SpacesInsideParenthesesFixer". CommentsComments are a bit critical/needs extra work due the need to force the "#" against the common style.
| ||||
Steps To Reproduce |
Add the "esc.php" (from below) run the first check
esc.php Dev version, will be cleaned up. | ||||
Additional Information | TODO
| ||||
Tags | No tags attached. | ||||
At very first i would suggest adding a Running the "SingleBlankLineAtEofFixer" against "core" shows 42 Files without a "Blank line at eof". .editorconfg for MantisBT |
|
additional fixer NoUnusedImportsFixer 1) api/rest/restcore/pages_rest.php
2) core/commands/IssueFileGetCommand.php |
|
NOTE to myself, don’t read further! phpstan found some code smells. There must be a sniffer for it.
|
|
@hotzeplotz Thanks :-) |
|
General, preliminary commentOur coding guidelines were originally written nearly 20 years ago, and have only seen a few minor adjustments over time. In particular, PHP 7 and 8 improvements such as function return types, argument types, etc are not taken into account at all. I am perfectly fine with adapting the guidelines if needed, i.e. change existing rules and/or add new ones for language features currently not covered. In this case, as a general rule, I would probably go with PSR-12 standard. When in doubt, just ask. If you want a quick chat I'm generally available on Matrix Some remarks after a quick read
Not sure what you mean by that.
See
As mentioned before, I'm OK with changing the rules to accept short format, and also for it to be the default.
Valid class name = CamelCase starting with capital letter
Not really. Expectation is for function calls (and declarations) to be on a single line if possible (i.e. when it fits under 120 chars). Beyond that, it should be multi-line, with one argument per line like example 4 For the rest, I let you go ahead as you think is best, and again don't hesitate to ask if you have any questions or doubts. |
|
This is the original file, initially posted under Steps to reproduce above ecs.php (18,336 bytes)
<?php /* * MantisBT - A PHP based bugtracking system * * MantisBT is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, either version 2 of the License, or * (at your option) any later version. * * MantisBT is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with MantisBT. If not, see <http://www.gnu.org/licenses/>. * * @copyright Copyright MantisBT Team - mantisbt-dev@lists.sourceforge.net */ /** * The coding standard for MantisBT. * * @see https://mantisbt.org/wiki/doku.php/mantisbt:coding_guidelines#code_blocks * * A overview for the cs-fixer rules * @see https://cs.symfony.com/doc/rules * @see https://mlocati.github.io/php-cs-fixer-configurator/ */ use Symplify\EasyCodingStandard\Config\ECSConfig; return ECSConfig::configure() ->withPaths([ // __DIR__ . '/', // __DIR__ . '/api', // __DIR__ . '/core', // __DIR__ . '/tests', // __DIR__ . '/plugins/MantisCoreFormatting', __DIR__ . '/tests/fixer_test_malformed.php', // __DIR__ . '/ecs.php', ]) ->withSkip([ \PhpCsFixer\Fixer\Whitespace\SpacesInsideParenthesesFixer::class => [ __DIR__ . '/ecs.php', ], __DIR__ . '/build', __DIR__ . '/config', __DIR__ . '/lang', __DIR__ . '/library', ]) /** * General spacing * Indentation: Tab * Line endings: LF "\n" */ ->withSpacing( /* indentation: */ \Symplify\EasyCodingStandard\ValueObject\Option::INDENTATION_TAB, /* * devy - comment will removed if the question is answered * * @see LineEndingFixer; "\n" is the default for the LineEndingFixer * @see LineEndingsSniff, [ 'eolChar' => "\n" ] * * todo both fixers do the job without setting this value. * Check if this is required. */ // /* lineEnding: */ "\n", ) // PHP language level 7.4 // ->withPhpCsFixerSets(php74Migration: true) ->withRules([ /** * Basic: Encoding * * PHP code MUST use only UTF-8 without BOM (remove BOM). * * @see https://cs.symfony.com/doc/rules/basic/encoding.html */ \PhpCsFixer\Fixer\Basic\EncodingFixer::class, /** * Whitespace: Line encoding * * All PHP files must use same line ending. Default is "\n" * * Sniff: LineEndingsSniff with ['eolChar' => "\n"] * * @see https://cs.symfony.com/doc/rules/whitespace/line_ending.html */ \PhpCsFixer\Fixer\Whitespace\LineEndingFixer::class, /** * Whitespace: No trailing whitespaces * * Remove trailing whitespace at the end of non-blank lines. * * "$foo = 'bar'···" > "$foo = 'bar'" * * @see https://cs.symfony.com/doc/rules/whitespace/no_trailing_whitespace.html */ \PhpCsFixer\Fixer\Whitespace\NoTrailingWhitespaceFixer::class, /** * Whitespace: Single blank line at eof * * A PHP file without end tag must always end with a single empty line feed. * * Sniff: PSR2.Files.EndFileNewline * * @see https://cs.symfony.com/doc/rules/whitespace/single_blank_line_at_eof.html */ \PhpCsFixer\Fixer\Whitespace\SingleBlankLineAtEofFixer::class, /** * Whitespaces: No whitespace in blank lines * * Remove trailing whitespace at the end of blank lines. * * <input> * ··· * $a = 1;" * </input> * <output> * * $a = 1;" * </output> * * @see https://cs.symfony.com/doc/rules/whitespace/no_whitespace_in_blank_line.html */ \PhpCsFixer\Fixer\Whitespace\NoWhitespaceInBlankLineFixer::class, /** * String: Single quotes * * Convert double quotes to single quotes for simple strings. * * Configurable. Default is keep double-quoted strings if they contain a * single-quoted string. * * $a = "sample" > $a = 'sample' * $b = "sample with 'single-quotes'" > $b = "sample with 'single-quotes'" * * Sniff: Squiz.Strings.DoubleQuoteUsage * * @see https://cs.symfony.com/doc/rules/string_notation/single_quote.html */ \PhpCsFixer\Fixer\StringNotation\SingleQuoteFixer::class, /** * PHP tag: Full opening tag * * "<?" > "<?php" * * Sniff: Generic.PHP.DisallowShortOpenTag * * @see https://cs.symfony.com/doc/rules/php_tag/full_opening_tag.html */ \PhpCsFixer\Fixer\PhpTag\FullOpeningTagFixer::class, /** * PHP tag: Echo tag syntax * * Configurable. Default is "format" = "short", "long_function" = "echo" * * "<?= …" > "<?php echo …" * * @see https://cs.symfony.com/doc/rules/php_tag/echo_tag_syntax.html */ \PhpCsFixer\Fixer\PhpTag\EchoTagSyntaxFixer::class, /** * PHP tag: No closing tag * * The closing ?> tag MUST be omitted from files containing only PHP. * * Sniff: PSR2.Files.ClosingTag * * @see https://cs.symfony.com/doc/rules/php_tag/no_closing_tag.html */ \PhpCsFixer\Fixer\PhpTag\NoClosingTagFixer::class, /** * Casing: Constant case: lower * * The PHP constants true, false, and null MUST be written * using the correct casing. * * Configurable. Default is "lower" * * "$a = FALse" > "a = false" * * Sniff: Generic.PHP.LowerCaseConstant * * @see https://cs.symfony.com/doc/rules/casing/constant_case.html */ \PhpCsFixer\Fixer\Casing\ConstantCaseFixer::class, /** * Casing: Lowercase keywords * * PHP keywords MUST be in lower case. * * "FOREACH( $a AS $B )" > "foreach( $a as $B )" * * Sniff: Generic.PHP.LowerCaseKeyword * * @see https://cs.symfony.com/doc/rules/casing/lowercase_keywords.html */ \PhpCsFixer\Fixer\Casing\LowercaseKeywordsFixer::class, /** * Function: No spaces after function name * * When making a method or function call, there MUST NOT be a space * between the method or function name and the opening parenthesis. * * "foo ( test ( 3 ) );" > "foo( test( 3 ) )" * * Sniffs: Function * * @see https://cs.symfony.com/doc/rules/function_notation/no_spaces_after_function_name.html */ \PhpCsFixer\Fixer\FunctionNotation\NoSpacesAfterFunctionNameFixer::class, /** * Function: No space after function declaration * * Spaces should be properly placed in a function declaration. * * Configurable. Default match MantisBT CS * * "function foo () {}" > "function foo() {}" * * @see https://cs.symfony.com/doc/rules/function_notation/function_declaration.html */ \PhpCsFixer\Fixer\FunctionNotation\FunctionDeclarationFixer::class, /** * Operator: Space around operators * * Configurable. Default is "single_space" * * "$a=1+2;" > "$a = 1 + 2;" * * Sniff: OperatorSpacingSniff * * @see https://cs.symfony.com/doc/rules/operator/binary_operator_spaces.html */ \PhpCsFixer\Fixer\Operator\BinaryOperatorSpacesFixer::class, /** * Array: Array syntax * * Configurable. Default is "short" syntax. * * array(1,2) > [1,2]; * * @see https://cs.symfony.com/doc/rules/array_notation/array_syntax.html */ \PhpCsFixer\Fixer\ArrayNotation\ArraySyntaxFixer::class, /** * Array: Trim array spaces * * Arrays should be formatted like function/method arguments, * without leading or trailing single line space. * * [ ] > [] * [ 1,2 ] > [1,2] * * @see https://cs.symfony.com/doc/rules/array_notation/trim_array_spaces.html */ \PhpCsFixer\Fixer\ArrayNotation\TrimArraySpacesFixer::class, /** * Array: No whitespace before comma in array * * In array declaration, there MUST NOT be a whitespace before * each comma. * * [1 , 2 , 8] > [1, 2, 3] * * @see https://cs.symfony.com/doc/rules/array_notation/no_whitespace_before_comma_in_array.html */ \PhpCsFixer\Fixer\ArrayNotation\NoWhitespaceBeforeCommaInArrayFixer::class, /** * Array: Whitespace after comma * * In array declaration, there MUST be a whitespace after each comma. * * Configurable. * * [1,2,3] > [1, 2, 3] * * "ensure_single_space" => false * ['one', 'two', 'three'] * [1, 2, 3] * * @see https://cs.symfony.com/doc/rules/array_notation/whitespace_after_comma_in_array.html */ \PhpCsFixer\Fixer\ArrayNotation\WhitespaceAfterCommaInArrayFixer::class, /** * Whitespace: Array indention * * Each element of an array must be indented exactly once. * * <input> * $foo = [ * 'bar' => [ * 'baz' => true, * ], * ]; * </input> * <output> * $foo = [ * 'bar' => [ * 'baz' => true, * ], * ]; * </output> * * @see https://cs.symfony.com/doc/rules/whitespace/array_indentation.html */ \PhpCsFixer\Fixer\Whitespace\ArrayIndentationFixer::class, /** * Control structure: Continuation position: same line * * Configurable. Default is "same_line" * * <input> * if( $baz == true ) { * echo "foo"; * } * else { * echo "bar"; * } * </input> * <output> * if( $baz == true ) { * echo "foo"; * } else { * echo "bar"; * } * </output> * * @see https://cs.symfony.com/doc/rules/control_structure/control_structure_continuation_position.html */ \PhpCsFixer\Fixer\ControlStructure\ControlStructureContinuationPositionFixer::class, /** * Control structure: Braces * * The body of each control structure MUST be enclosed within braces. * * <input> * if( $foo === $bar ) * echo 'same' * </input> * <output> * if( $foo === $bar ) { * echo 'same' * } * </output> * * Sniff: InlineControlStructureSniff * * @see https://cs.symfony.com/doc/rules/control_structure/control_structure_braces.html */ \PhpCsFixer\Fixer\ControlStructure\ControlStructureBracesFixer::class, /** * Language construct: Single space around a language construct * * @todo consider about it. * * @see https://cs.symfony.com/doc/rules/language_construct/single_space_around_construct.html */ // \PhpCsFixer\Fixer\LanguageConstruct\SingleSpaceAroundConstructFixer::class, /** * Whitespace: Statement indention * * Each statement must be indented. * * @note: Issues with mixed indention (tab and whitespaces)? * * @see https://cs.symfony.com/doc/rules/whitespace/statement_indentation.html */ \PhpCsFixer\Fixer\Whitespace\StatementIndentationFixer::class, /** * Comment: Spacing of single line comment * * #comment > # comment * * @see https://cs.symfony.com/doc/rules/comment/single_line_comment_spacing.html */ \PhpCsFixer\Fixer\Comment\SingleLineCommentSpacingFixer::class, ]) /** * File: Line endings * * Line endings MUST be "\n". * * @see LineEndingFixer * * The LineEndingFixer does the same, but is not configurable, * "\n" is the default. This sniff is a little more explicit. * * @see https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/Files/LineEndingsSniff.php */ // ->withConfiguredRule(\PHP_CodeSniffer\Standards\Generic\Sniffs\Files\LineEndingsSniff::class, [ // 'eolChar' => "\n", // ]) /** * Basic: Position of braces * * Opening braces on same line * * "Class Name {}" * "function name() {}" * * @see https://cs.symfony.com/doc/rules/basic/braces_position.html */ ->withConfiguredRule(\PhpCsFixer\Fixer\Basic\BracesPositionFixer::class, [ 'classes_opening_brace' => 'same_line', 'functions_opening_brace' => 'same_line', ]) /** * Whitespace: Spaces inside parentheses * * "function foo($bar, $baz)" > "function foo( $bar, $baz )" * "if($bar === $baz)" > "if( $bar === $baz )" * "foo( )" > "foo()" * * Sniffs: * - build/CodeSniffer/Mantis/Sniffs/ControlStructures/ControlSignatureSniff.php * - ControlStructureSpacingSniff * - FunctionDeclarationArgumentSpacingSniff * * @see https://cs.symfony.com/doc/rules/whitespace/spaces_inside_parentheses.html */ ->withConfiguredRule(\PhpCsFixer\Fixer\Whitespace\SpacesInsideParenthesesFixer::class, [ 'space' => 'single', ]) /** * Sniff: control structure spacing * * this will fix only control structures * if ($baz == true) > if ( $baz == true ) * * but not * function('boo') > function( 'boo' ) * * for this the sniff FunctionDeclarationArgumentSpacingSniff is needed * * @see FunctionDeclarationArgumentSpacingSniff * * Both are fixed with one * @see SpacesInsideParenthesesFixer */ // ->withConfiguredRule(PHP_CodeSniffer\Standards\PSR2\Sniffs\ControlStructures\ControlStructureSpacingSniff::class, [ // 'requiredSpacesAfterOpen' => 1, // 'requiredSpacesBeforeClose' => 1, // ]) // ->withConfiguredRule( // \PHP_CodeSniffer\Standards\Squiz\Sniffs\Functions\FunctionDeclarationArgumentSpacingSniff::class, [ // 'equalsSpacing' => 1, // 'requiredSpacesAfterOpen' => 1, // 'requiredSpacesBeforeClose' => 1 // ] // ) // Sniff concat spaces // @see ConcatSpaceFixer // ->withConfiguredRule(\PHP_CodeSniffer\Standards\Squiz\Sniffs\Strings\ConcatenationSpacingSniff::class, [ // 'spacing' => 1, // 'ignoreNewlines' => true, // ]) /** * Operator: Concat spaces * * Spacing to apply around concatenation operator. * * @see https://cs.symfony.com/doc/rules/operator/concat_space.html */ ->withConfiguredRule(\PhpCsFixer\Fixer\Operator\ConcatSpaceFixer::class, [ 'spacing' => 'one', ]) // single_line_comment_style // https://mlocati.github.io/php-cs-fixer-configurator/#version:3.52|fixer:single_line_comment_style ->withConfiguredRule(\PhpCsFixer\Fixer\Comment\SingleLineCommentStyleFixer::class, [ 'comment_types' => ['asterisk'], ]) /** * Cast: No space after cast * * "$bar = ( string ) $a;" > "$bar = (string)$a;" * * Sniff: NoSpaceAfterCast * * @see https://mlocati.github.io/php-cs-fixer-configurator/#version:3.52|fixer:cast_spaces */ ->withConfiguredRule( \PhpCsFixer\Fixer\CastNotation\CastSpacesFixer::class, [ 'space' => 'none' ] ) /** * Whitespace: No extra blank line * * <input> * function foo() { * * return 'bar'; * } * </input> * <output> * function foo() { * return 'bar'; * } * </output> * * Sniff: OpeningFunctionBraceKernighanRitchie * * @see https://cs.symfony.com/doc/rules/whitespace/no_extra_blank_lines.html */ ->withConfiguredRule( \PhpCsFixer\Fixer\Whitespace\NoExtraBlankLinesFixer::class, [ 'tokens' => [ # no space after opening braces 'parenthesis_brace_block', 'extra' ]] ) /** * Function: Argument spaces * * @todo check * * Sniff: PEAR.Functions.FunctionCallSignature [allowMultipleArguments => false] * * @see https://cs.symfony.com/doc/rules/function_notation/method_argument_space.html#example-5 */ // ->withConfiguredRule(\PhpCsFixer\Fixer\FunctionNotation\MethodArgumentSpaceFixer::class, [ // 'on_multiline' => 'ensure_single_line' // ]) ; |
|
Thanks for your feedback. Will follow, but unfortunately, my working life has started again this week and - of course very urgent - work has to be done. So it may be a few more days before I can deal with this again. No hurry.
Forgot to add it to the rules, last night i tried but didn't get a warning, even when a real depreceted function were added. Haven't investigate further.
Should we clearify which of the rules from the current style guide are up for debate? I totally agree on PSR-12. In my opinion it's the way to go, radically. The PRs must run the fixer or have to rebase. Most of the PSR-12 is in many ways very contrasted to the current Mantis styles. Would you are also fine with The main reasons for why i vote for radically PSR-12 is the fact that the style looks good, its cheap because it requires no or less extra work (no own fixer to force the tab or the "#") and having a consist look for all is more important than how exactly it looks. Since you are using a modern IDE, have you ever noticed the However, that’s what I wish for :-) but of course i fully respect whatever your style is and follow it. The last two nights i learned a lot about fixing the cs of legacy code, how to mix sniffers and fixers, how they play together or the ways they interfere each other or in which order they have to execute. I also made some pretty good progress and stumbled across a lot of "special" cases, which i have all collected to document/report. Having a PSR-12 would be the "Krönung" :-) (its something like "crowning" in englich but not sure if it is the right translation of what it means.) |
|
When I mentioned PSR-12, I did not mean dropping our current standard and go for a full-blown switch. I was referring to adopting existing PSR-12 rules, when some new language construct or case is not covered by our current guidelines. A full switch to PSR-12 is something that should be discussed with the rest of the team, but to be honest I see little added value to it. And keep in mind such large changes are causing a break in code history, making it more difficult to trace changes back in time (which is something I find myself doing quite frequently) because whole files will be marked as updated at the same point in time when the code style change is applied
In theory yes, but see above comment...
It's true that with syntax highlighting this has become much less of an issue. Still does take more space than
Actually I have. Out of long habit (yes, I'm old), for some languages I actually have tab width set to 2.
I think I understand what you mean, we have a similar idiom in French (couronnement). Like apotheosis.
Take your time, this is not urgent or critical. Your contribution is much appreciated, whenever it comes. |
|
I knew you would argue with that … I thought too. :-) But look at it this way: we get one more character for each comment, but save five for each The Beside that, i am on track, i have already set up the things to match the current guidelines. Got your point. Edit:
Yes, i think i understand that point, but you have to make a choise at time. Either keep the old beloved forever or do a cut with a new timeline afterward. Simpler: "Make progress". Just what i personally think. Edit 2: Or we let fail the fixer (or remove it from the CI) and only each file that is currently changed ends up with two commits. 1. "Fix bug", 2. "Fix cs". Over the time, more and more files become stylish. |
|
OK, you win :-D
Oh you meant the header comment ? I thought you were referring to the actual line comments throughout the code... Actually in this case it makes perfect sense to replace that with a standard, file-level PHPDoc block. I have been considering that for years but it's always been a really low priority item. Maybe now is the time to finally do it ! Just need to carefully think what we want to put in there, in addition to the existing license and copyright info. EDIT: what I mean is that in most files, there is already a PHPDoc block below this header, and I believe these 2 should be merged.
That does not address the root cause (going backwards on a file's history with git blame), the problem remains the same and even worse as we spread over time instead of all changes in one go - so between these 2 options IMO the big bang approach is definitely better. |
|
I actually meant all. But I read from your comment that it will be difficult to switch completely from
Will investigate a bit, how other do. Is it a docblock Edit: Short look to the vendor dir. phpDocumentor it self uses a phpdoc block
But the most have a standard multiline comment
phpdoc is mostly used only for functions and methods. I think the question is what you want to achieve. Full automated documentation or just placing some information about the project? Honestly i've never run the phpDocumentor against some docblocks to see how they act. |
|
Thanks @hotzeplotz @hotzeplotz @dregad I have a concern about major changes relating to styles in an existing, especially open-source project. Even if we implement tooling that enforces our current guidelines as is, there will be a ton of generated changes that fix violations of our standard. Such changes will make it harder for forks/patches to merge their changes with the latest versions of MantisBT. Hence, I generally prefer making fixes locally when changing a piece of code (e.g. a function). In such a case, there is a change anyway, and that block of code will have to be merged anyway. So, in addition to discussing the details of a fixer, I think we need to discuss the high-level approach here and whether we should have a fixer and how it should be used if we have one. |
|
Yes, that is unavoidable This is why I am not so keen on switching from our current, homegrown coding standard to PSR-12 (see my comment in 0034400:0068810 and subsequent discussion) even if it means more efforts to implement it.
Of course it will introduce merge conflicts, but if we have automated tooling in place, resolving those shuld be as simple as running the fixer on the contributor's feature branch so I don't think that's a problem.
If we follow this approach, considering that our code base is quite stable overall, it will probably take 20-30 years until we have it all covered (if we ever do). I really don't think it's a good approach. We need to bite the bullet at some point.
I think there is no discussion about needing a fixer. We do, period. As to how it should be used, IMO the requirements are that it
AFAICT, the proposed solution satisfies these criteria. |
|