Allow blank lines between multi-line function arguments

[iquito]

With constructor property promotion in PHP8 (especially in conjunction with attributes) new styling issues arise which make it harder to write as readable code as before. A real-world example explains this most easily - in PHP 7.4 I had a class like this:

class InstanceChangeAction
{
    /** @Cascade */
    public InstanceCustomerAction $customer;

    /** @Cascade */
    public ?InstanceCustomerInitialAction $customerInitial = null;

    /** @Cascade */
    public ?InstanceResellerAction $reseller = null;

    /** @Cascade */
    public ?InstanceAdminAction $admin = null;
}

With PHP8 I changed it to the following definition:

class InstanceChangeAction
{
    public function __construct(
        #[Cascade]
        public InstanceCustomerAction $customer,

        #[Cascade]
        public ?InstanceCustomerInitialAction $customerInitial = null,

        #[Cascade]
        public ?InstanceResellerAction $reseller = null,

        #[Cascade]
        public ?InstanceAdminAction $admin = null,
    ) {
    }
}

The intention is still the same, and thanks to constructor property promotion a valid value on $customer is always enforced (which was not the case before) without making the class larger or more complex.

The issue with PHP_CodeSniffer is that the above definition generates Blank lines are not allowed in a multi-line function declaration errors when using PSR12. Yet if the newlines are removed, the readability suffers, and it is less obvious that these arguments are also class properties:

class InstanceChangeAction
{
    public function __construct(
        #[Cascade]
        public InstanceCustomerAction $customer,
        #[Cascade]
        public ?InstanceCustomerInitialAction $customerInitial = null,
        #[Cascade]
        public ?InstanceResellerAction $reseller = null,
        #[Cascade]
        public ?InstanceAdminAction $admin = null,
    ) {
    }
}

I read through the exact definition in PSR12 and it does not state that there cannot be blank lines between function arguments - it mainly mentions the first item in the list needs to start on the next line (so no blank line at the beginning), and that there can be a maximum of one argument per line. My suggestion would be to allow extra newlines between arguments for multi-line function declarations in general - no newlines at the beginning or end, but in-between it can be a tool for increased readability, especially with constructor property promotion, but even without it (attributes can also be defined just for a function argument, so having the possibility to separate these items should be helpful in different situations).

[gsherwood]

PSR-12 was written before constructor property promotion was part of the language, so defining how this should be done would best be decided by the authors, not within this project.

We could have a conversation about allowing blank lines in multi-line function declarations, but PSR-12 uses the same language throughout. So what we'd really be talking about is allowing 1 or more blank lines pretty much anywhere in code, which starts to make code look different to what is written in the standard, and introduces inconsistencies.

While a single blank line between promoted properties might be fine, what about if some have 2 blank lines, or what if we do this for standard function arguments as well? Maybe the answer is just to leave people to do whatever they want, but then developers are likely to want/need some control over these rules so code still looks like it is consistently formatted, which adds a lot of extra work for the project and users.

A better outcome (I think) would be an agreement on how to define promoted properties and then an exception built into the PSR-12 standard to enforce this.

My suggestion would be to open a discussion in the php-fig group: https://groups.google.com/u/2/g/php-fig and see if the authors themselves can weigh in on where the boundaries should be.

[iquito]

PSR-12 does not disallow blank lines between multi-line function arguments: it is just not mentioned. The overall guiding principle of PSR-12 is: Blank lines MAY be added to improve readability and to indicate related blocks of code except where explicitly forbidden.. You can have as many blank lines between class methods or class properties as you want, for example. I think the same should be the case for multi-line function arguments, as it is not explicitly forbidden there. PSR-12 leaves a lot of freedom to the coder on purpose, to have different options on how to write code in a more readable way.

Because of that, if PHP-FIG replaces PSR-12 they might not need to change anything about blank lines for function arguments, because that is already not forbidden. I also suspect a new coding style PSR will make a lot more sense after PHP 8.1 is released, because there are quite a few new features coming which might impact code styling even more, and any new PSR will take quite some time - having some kind of solution near-term would be helpful to all the projects upgrading to PHP8.

My suggestion would be to make the current behavior to not allow blank lines in multi-line function arguments optional/changeable. Making that sniff more permissive and being able to opt-in to "no blank lines in multi-line function arguments" or "a maximum of one blank line between multi-line function arguments" (which would be a useful variant) would not take away any options and leave it up to the project which one they prefer. Even an opt-out (instead of an opt-in) of the current behavior could be an option specifically for blank lines, just as a pragmatic solution.

Temporarily I managed to allow blank lines by excluding the Squiz.Functions.MultiLineFunctionDeclaration sniff, but this of course allows a lot more than just blank lines.

[gsherwood]

Temporarily I managed to allow blank lines by excluding the Squiz.Functions.MultiLineFunctionDeclaration sniff, but this of course allows a lot more than just blank lines.

If you exclude Squiz.Functions.MultiLineFunctionDeclaration.EmptyLine instead, it will only stop printing the blank line found error. The rest of the sniff will work correctly and your sample code will run without error.

[iquito]

Thanks! I don't think I would have found that out by myself, and it is a good workaround for now.

[gsherwood]

I don't think I would have found that out by myself

If you run PHPCS with the -s command line argument, the specific error message code is displayed next to each message. You can exclude each of these, turn them into errors/warnings, or change their message content using a ruleset.xml file.

[iquito]

Another issue connected to this one is Docblocks for multi-line function arguments, which become more relevant with constructor property promotion because any comments or any annotations used for the property part are easily affected. An example:

class NumberRangeListAction
{
    public function __construct(
        /**
         * This nested annotation cannot be replaced with attributes for now:
         *
         * @Assert\All({
         *   @Assert\Type(type="Component\PhoneNumberRange")
         * })
         *
         * @var PhoneNumberRange[]
         */
        #[Assert\NotBlank]
        #[Cascade]
        public array $numbers = [],
    ) {
    }
}

The following errors are reported (the first error for all lines of the Docblock after the initial line, so it occurs 8 times, the second error for the first line with an attribute):

Multi-line function declaration not indented correctly; expected 8 spaces but found 0
The first parameter of a multi-line function declaration must be on the line after the opening bracket

Single line Docblocks work for the identation error, but as soon as there are multiple lines of comments it is not supported anymore. I thought I would add this problem here as it might be helpful to collect these issues around constructor property promotion, which are always connected to how multi-line function arguments are styled / checked. It might be an option to relax all these rules only for class constructors, that would be less sweeping than changing how multi-line function arguments are checked in general.

[jrfnl]

Another issue connected to this one is Docblocks for multi-line function arguments, which become more relevant with constructor property promotion because any comments or any annotations used for the property part are easily affected.

@iquito This particular part of this issue should be fixed by PR #3343. Testing appreciated.