View Issue Details

IDProjectCategoryView StatusLast Update
0034400mantisbtcode cleanuppublic2024-04-22 04:14
Reporterhotzeplotz Assigned To 
PrioritynormalSeverityfeatureReproducibilityhave not tried
Status confirmedResolutionopen 
Target Version2.27.0 
Summary0034400: Introduce a cs fixer
Description

Recent sample: https://github.com/mantisbt/mantisbt/pull/1989#discussion_r1554994663

This could have been avoided with:

Check …

vendor/bin/ecs check view_user_page.php

… and fix

vendor/bin/ecs check view_user_page.php --fix

Introducing a cs fixer

This 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 go

My 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/Rules

The 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.

Rules

Some i don't know, others are fixed "automatically" by a already used fixer. Its always time to figure it out in detail.

<rule ref="Generic.PHP.DisallowShortOpenTag"/>

<rule ref="PSR2.Files.ClosingTag"/>

<rule ref="Generic.PHP.DeprecatedFunctions"/>

  • Ups … forgotten, will use the sniff for it.

<rule ref="Generic.PHP.LowerCaseKeyword"/>

<rule ref="Generic.PHP.LowerCaseConstant"/>

<!-- Control structures:
    - inline not allowed
-->
<rule ref="Generic.ControlStructures.InlineControlStructure"/>

ControlStructureBracesFixer: https://cs.symfony.com/doc/rules/control_structure/control_structure_braces.html

<rule ref="PSR2.ControlStructures.ControlStructureSpacing">
    <properties>
        <property name="requiredSpacesAfterOpen" value="1"/>
        <property name="requiredSpacesBeforeClose" value="1"/>
    </properties>
</rule>

SpacesInsideParenthesesFixer ('space' => 'single'): https://cs.symfony.com/doc/rules/whitespace/spaces_inside_parentheses.html

<!-- Strings -->
<rule ref="Squiz.Strings.DoubleQuoteUsage"/>
<rule ref="Squiz.Strings.ConcatenationSpacing">
    <properties>
        <property name="spacing" value="1"/>
        <property name="ignoreNewlines" value="true"/>
    </properties>
</rule>
<!-- Function declarations -->
<rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie"/>
<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
    <properties>
        <property name="equalsSpacing" value="1"/>
        <property name="requiredSpacesAfterOpen" value="1"/>
        <property name="requiredSpacesBeforeClose" value="1"/>
    </properties>
</rule>
<rule ref="Squiz.WhiteSpace.FunctionOpeningBraceSpace"/>
  • FunctionDeclarationArgumentSpacing Not 100% sure.
  • OpeningFunctionBraceKernighanRitchie Not 100% sure. No empty line in body (opening)?

Seems good with:

<rule ref="Generic.WhiteSpace.DisallowSpaceIndent"/>

  • Base setting ->withSpacing(Option::INDENTATION_TAB)

<rule ref="Generic.Formatting.NoSpaceAfterCast"/>

<rule ref="Generic.Files.LineEndings">
    <!-- Use Unix new lines -->
    <properties>
        <property name="eolChar" value="\n"/>
    </properties>
</rule>

<rule ref="PSR2.Files.EndFileNewline"/>

<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/>
<rule ref="Squiz.WhiteSpace.ScopeClosingBrace"/>

Todo: don't know what they do.

<!-- Arrays -->
<rule ref="Generic.Arrays.DisallowShortArraySyntax"/>
<rule ref="Squiz.Arrays.ArrayBracketSpacing"/>
<rule ref="Mantis.Arrays.ArrayDeclaration">
    <exclude name="Mantis.Arrays.ArrayDeclaration.SingleLineNotAllowed"/>
    <exclude name="Mantis.Arrays.ArrayDeclaration.KeyNotAligned"/>
    <exclude name="Mantis.Arrays.ArrayDeclaration.ValueNotAligned"/>
    <exclude name="Mantis.Arrays.ArrayDeclaration.CloseBraceNotAligned"/>
</rule>
<!-- Naming conventions -->
<rule ref="Squiz.Classes.ValidClassName"/>
  • Forgotten. Will add the sniff, even i do not know what a valid class name is.
<!-- Function calls:
    - 1 space after '(', 1 space before ')'
    - only one argument per line in multi-line function calls
-->
<rule ref="PEAR.Functions.FunctionCallSignature">
    <properties>
        <property name="allowMultipleArguments" value="false"/>
        <property name="requiredSpacesAfterOpen" value="1"/>
        <property name="requiredSpacesBeforeClose" value="1"/>
    </properties>
</rule>

Not sure, is it https://cs.symfony.com/doc/rules/function_notation/method_argument_space.html#example-5?

<rule ref="Generic.Functions.FunctionCallArgumentSpacing"/>

Not sure. Maybe its already fixed with another applied fixer. eg. "SpacesInsideParenthesesFixer".

Comments

Comments are a bit critical/needs extra work due the need to force the "#" against the common style.

<!-- Comments -->
<rule ref="Squiz.Commenting.FileComment">
    <exclude name="Squiz.Commenting.FileComment.IncorrectAuthor"/>
    <exclude name="Squiz.Commenting.FileComment.IncorrectCopyright"/>
</rule>
<rule ref="Squiz.Commenting.FunctionComment">
    <!-- On PHP 7, this Sniff throws errors for missing type hints, but
         since we need to support older PHP versions we disable it
         @TODO the Sniff should throw a warning instead
    -->
    <exclude name="Squiz.Commenting.FunctionComment.TypeHintMissing"/>
</rule>
<rule ref="Squiz.Commenting.ClassComment">
    <!-- This sniff throws warnings for each DocBlock tag in the class comment.
         Since our guidelines don't specify what tags to use, we disable it
    -->
    <exclude name="Squiz.Commenting.ClassComment.TagNotAllowed"/>
</rule>
<rule ref="Squiz.Commenting.BlockComment"/>
Steps To Reproduce
composer require symplify/easy-coding-standard --dev

Add the "esc.php" (from below)

run the first check

vendor/bin/ecs check core

esc.php Dev version, will be cleaned up.
EDIT (dregad): move file to attachment 0034400:0068808 - it's a pain to scroll through so many lines, just to view the notes ;-)

Additional Information

TODO

  • Make the own sniff "check for prefixed varnames" available.
  • Check wether the other own sniffs still necessary.
  • Figure out which of the sniffs are not covered.
TagsNo tags attached.

Activities

hotzeplotz

hotzeplotz

2024-04-08 14:25

reporter   ~0068802

At very first i would suggest adding a .editorconfg file, so the IDE can use it to set up its formatting for this project. e.g. PHPStorm’s "Reformat code"

Running the "SingleBlankLineAtEofFixer" against "core" shows 42 Files without a "Blank line at eof".

.editorconfg for MantisBT

root = true

[*]
charset = utf-8
end_of_line = lf
indent_size = 4
indent_style = tab
tab_width = 4
max_line_length = 80
trim_trailing_whitespace = true
insert_final_newline = true
hotzeplotz

hotzeplotz

2024-04-08 16:02

reporter   ~0068803

Last edited: 2024-04-08 16:03

additional fixer

NoUnusedImportsFixer

1) api/rest/restcore/pages_rest.php

@@ -22,7 +22,6 @@
  * @link http://www.mantisbt.org
  */

-use Mantis\Exceptions\ClientException;

 $g_app->group('/pages', function() use ( $g_app ) {
        $g_app->get( '/issues/view/{id}/', 'rest_pages_issue_view' );

2) core/commands/IssueFileGetCommand.php

@@ -21,7 +21,6 @@
 require_api( 'helper_api.php' );
 require_api( 'user_api.php' );

-use Mantis\Exceptions\ClientException;

 /**
  * A command that gets issue attachments.
hotzeplotz

hotzeplotz

2024-04-09 10:11

reporter   ~0068805

Last edited: 2024-04-09 10:13

NOTE to myself, don’t read further!

phpstan found some code smells. There must be a sniffer for it.

 ------ --------------------------------------------------------------------- 
  Line   summary_api.php                                                      
 ------ --------------------------------------------------------------------- 
  198    Class DbQuery referenced with incorrect case: DBQuery.               
  307    Class DbQuery referenced with incorrect case: DBQuery.               
  371    Class DbQuery referenced with incorrect case: DBQuery.               
  423    Class DbQuery referenced with incorrect case: DBQuery.               
  482    Class DbQuery referenced with incorrect case: DBQuery.               
  509    Class DbQuery referenced with incorrect case: DBQuery.               
  614    Class DbQuery referenced with incorrect case: DBQuery.               
  680    Class DbQuery referenced with incorrect case: DBQuery.               
  755    Class DbQuery referenced with incorrect case: DBQuery.               
  891    Class DbQuery referenced with incorrect case: DBQuery.               
  1035   Class DbQuery referenced with incorrect case: DBQuery.               
  1301   Class DbQuery referenced with incorrect case: DBQuery.               
 ------ ---------------------------------------------------------------------
class DbQuery {}
$t_query = new DBQuery();
                ^ "b" lowercase
 ------ ------------------------------------------------------------------------ 
  Line   sponsorship_api.php                                                     
 ------ ------------------------------------------------------------------------ 
  193    Class SponsorshipData referenced with incorrect case: SponsorShipData.  
 ------ ------------------------------------------------------------------------ 
class SponsorshipData {}
$t_sponsorship_data = new SponsorShipData;
                                 ^ "s"  lowercase
dregad

dregad

2024-04-10 03:08

developer   ~0068806

NOTE to myself, don’t read further!

@hotzeplotz Thanks :-)

dregad

dregad

2024-04-10 04:06

developer   ~0068807

General, preliminary comment

Our 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

<rule ref="Generic.PHP.DeprecatedFunctions"/>
Ups … forgotten, will use the sniff for it.

Not sure what you mean by that.

<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/>
<rule ref="Squiz.WhiteSpace.ScopeClosingBrace"/>
Todo: don't know what they do.

See

Arrays
ArraySyntaxFixer ('short: []' is the default

As mentioned before, I'm OK with changing the rules to accept short format, and also for it to be the default.
Question: does this mean we have to change all occurrences of ` array() syntax to []`, or can the two of them be allowed ? (Thinking about reducing churn here)

<!-- Naming conventions -->
<rule ref="Squiz.Classes.ValidClassName"/>
Forgotten. Will add the sniff, even i do not know what a valid class name is.

Valid class name = CamelCase starting with capital letter

<!-- Function calls:
Not sure, is it https://cs.symfony.com/doc/rules/function_notation/method_argument_space.html#example-5?

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.

dregad

dregad

2024-04-10 04:08

developer   ~0068808

Last edited: 2024-04-10 04:12

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'
    // ])
;
ecs.php (18,336 bytes)   
hotzeplotz

hotzeplotz

2024-04-10 14:26

reporter   ~0068809

Last edited: 2024-04-10 14:30

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.

Ups … forgotten, will use the sniff for it.

Not sure what you mean by that.

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.

I would probably go with PSR-12 standard.

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 \t > \s{4} or # > //

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 // is hard to recognize as a comment? Or have you ever set a value other than 4 for the tab indent?

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.)

dregad

dregad

2024-04-11 07:35

developer   ~0068810

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

Would you are also fine with \t > \s{4} or # > //

In theory yes, but see above comment...

have you ever noticed the // is hard to recognize as a comment?

It's true that with syntax highlighting this has become much less of an issue. Still does take more space than # ;-)

Or have you ever set a value other than 4 for the tab indent?

Actually I have. Out of long habit (yes, I'm old), for some languages I actually have tab width set to 2.
But it's true that nowadays 4 has become the standard

Krönung

I think I understand what you mean, we have a similar idiom in French (couronnement). Like apotheosis.

my working life has started again this week and - of course very urgent - work has to be done

Take your time, this is not urgent or critical. Your contribution is much appreciated, whenever it comes.

hotzeplotz

hotzeplotz

2024-04-11 11:18

reporter   ~0068812

Last edited: 2024-04-11 11:42

Still does take more space than # ;-)

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 array() > []. :-P

The # indroduces a single line comment and does not really fit for a multiline comment like the header comment (which currently it is a series of single line comments). So I am still vote for a least the header comment could be a real multi line comment, like (almost?) all within the vendor folder.

Beside that, i am on track, i have already set up the things to match the current guidelines. Got your point.

Edit:

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

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.

dregad

dregad

2024-04-11 12:16

developer   ~0068813

Last edited: 2024-04-11 12:17

we get one more character for each comment, but save five for each array() > []. :-P

OK, you win :-D

multiline comment like the header comment (which currently it is a series of single line comments).

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.

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.

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.

hotzeplotz

hotzeplotz

2024-04-11 12:47

reporter   ~0068814

Last edited: 2024-04-11 13:08

Oh you meant the header comment? I thought you were referring to the actual line comments throughout the code...

I actually meant all. But I read from your comment that it will be difficult to switch completely from # to //.
So at least the header comments are the part with the best arguments against the #

PHPDoc

Will investigate a bit, how other do. Is it a docblock /** or just a multiline comment /*, followed by a docblock if needed.

Edit: Short look to the vendor dir.

phpDocumentor it self uses a phpdoc block

/**
 * phpDocumentor
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 *
 * @link      http://phpdoc.org
 */

But the most have a standard multiline comment

/*
 * This file is part of the Monolog package.
 *
 * (c) Jordi Boggiano <j.boggiano@seld.be>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

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.

vboctor

vboctor

2024-04-21 15:11

manager   ~0068854

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.

dregad

dregad

2024-04-22 04:14

developer   ~0068856

there will be a ton of generated changes that fix violations of our standard.

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.

Such changes will make it harder for forks/patches to merge their changes with the latest versions of MantisBT.

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.

making fixes locally when changing a piece of code (e.g. a function)

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.

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

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

  1. can be run manually on command line (or from within an IDE)
  2. runs automatically as part of continuous integration (TravisCI builds) so any problems from pull requests are automatically flagged and we no longer have to perform tedious manual reviews and request changes related to code formatting.

AFAICT, the proposed solution satisfies these criteria.