Coding Standards Fixer rules

[jim-parry]

Looking for someone familiar with Coding Standards Fixer to create a ruleset matching our style guide. This is initially intended for use inside an IDE, and ultimately perhaps as part of our travis-ci build.

[AkenRoberts]

I can help with PHP CS Fixer, if there's existing documentation for the code standards. Can also provide recommendations based on popular standards.

[jim-parry]

https://bcit-ci.github.io/CodeIgniter4/contributing/styleguide.html

[AkenRoberts]

How set in stone are these? There are a few rules I would disagree with and recommend changing.

Accepting PSR-2 as a whole as your core coding standing improves standardization, and would reduce conflicts with people whose environments default to PSR-2 support. It also allows you to reduce the Code Style documentation, as you can remove anything PSR-2 related and simply point to its documentation.

Enforcing list alignment looks nice, however it will require additional code changes / diffs if the alignment length ever changes. Personally I'm not bothered by unaligned lists, however some people are really adamant about it.

Not related to PHP CS Fixer but in the Code Style documentation, this needs clarification:

To maintain consistency between core classes, class properties MUST be private or protected, and the following public methods MUST be used for each such property “x”

The naming convention for get/set/has/new/is is good to have. The enforcement of public/private properties is not. That should be dictated by the needs of the classes themselves. Sure, in the interest of the public API and extension/inheritance, most properties will not be public. Having a hard rule against public entirely, though, is not recommended.

[jim-parry]

PSR-2 has come up many times, but we don't agree with all of it, sorry. We have major issues with PHP-FIG, and it sounds like they are having internal issues too, but that should not influence any standards adopted.

As near as I can tell, the major difference between our guidelines and PSR-2 have to do with indenting/alignment, and with the bracing for control structures. There could be more, but those come to mind.

We are not inclined to embrace PSR-2 verbatim.

[jim-parry]

As to property visibility, that issue was discussed extensively within the council, and we agreed that we could not come with justifiable public visibility for properties in the core classes.

I think that, if a case can be made for relaxing that for a core class, then we might have to revisit it, but in the absence of a compelling reason the hard rule is appropriate. You suggested that such a rule is not recommended ... do you have a reference for that that we should look at?

[jim-parry]

You mention list alignment ... I don't think we have a rule specific to that, but rather an example of the indenting/alignment and what is appropriate.

The most recent changes to the style guide stated that a PR which deald only or mainly with style changes, for instance becvause someone has a different idea for list alignment, is not likely to be well received. Our primary interest in readability, not in someone's specific interpretation of that.

[AkenRoberts]

We are not inclined to embrace PSR-2 verbatim.

For what it's worth, other major projects have had the same opinion and yet eventually moved to PSR-2. I would advise that it at least be taken into consideration again, at the very least to show the PHP community that CI is willing to accept modern practices and standards as fully as possible.

I am not sure if there is a fixer for the newline control structure bracing syntax. I will have to confirm if PHP CS Fixer has that. (Perhaps another reason to stick with PSR-2? :wink:)

You suggested that such a rule is not recommended ... do you have a reference for that that we should look at?

I don't have a specific reference, because it is less of a hard rule and more of a "do whatever makes sense for the class and its logic and API" best practice. Consider Symfony's HTTP Request object; it has multiple public properties available that each houses its own functionality. These properties are considered part of Request's public API. Forcing the user to go through a getter to access each just adds unnecessary verbosity.

Developers simply need to think critically about classes and how they are used, and expose properties as intended. This includes considering the public API, open/closed and Liskov principles, etc. Public properties are not inherently evil, so long as they are intentionally made public, and are treated as such during updates (to maintain backwards compatibility).

Another reason to discourage the wordage in the documentation is that the Config classes all use public properties, so right away you have code in the repo conflicting with the style guide. Perhaps the system folder is the only place it really matters, but do you really want to have to make that distinction for users? More rules != better.

You mention list alignment ... I don't think we have a rule specific to that, but rather an example of the indenting/alignment and what is appropriate.

Reading again, it is kind of ambiguous as to whether or not that's a rule or recommendation. Cleaning up the language in the docs would easily clear that up. 👍

[gdhnz]

I haven't looked at making CodeSniffer rules for a while but from memory, you can include any other standards you want as part of your rules and then just exclude the ones you don't want in your ruleset.xml. Your CI specific rules then just need to go inside a Sniffs directory at the same level as the ruleset.xml file.

<?xml version="1.0"?>
<ruleset name="Codeigniter">
    <description>CodeIgniter rules</description>

    <!-- Include the whole PSR-2 standard -->
    <rule ref="PSR2">
        <exclude name="PSR1.Classes.ClassDeclaration.MissingNamespace"/>
        <exclude name="PSR1.Files.SideEffects.FoundWithSymbols"/>
    </rule>
</ruleset>

[mwhitneysdsu]

@gdhnz This issue is for CS Fixer, not CodeSniffer. While you can customize the ruleset, the method of configuring CS Fixer and creating new rules for it is different from that used for CodeSniffer.