Handbook: discourage multi-line parameters in function calls (assign to a variable then pass the var)

[azaozz]

This is a follow-up from #1323.

It seems that the parser has introduced unwanted changes in single function and filter/action parameters: associative arrays were formatted to be multi-line. This was never intended in the original "Associative arrays should be multi-line" rule.

Please note: this is about a single parameter being on multiple lines. Example 1:

do_action(
    'upgrader_process_complete', $this, array(
        'action' => 'update',
        'type'   => 'core',
    )
);

This is not about having each parameter on a new line. When the parameters are long, having each on a new line is better. Example 2:

do_action(
    'upgrader_process_complete', 
    'some long parameter that is easier to read when on a new line',
    'another long parameter that is more readable when on a new line'
);

1323 already deals with partially fixing this. I'd go one step further. The best way to fix this bug would be to discourage or even ban single parameters inside function and filter/action calls being on multiple lines.

In that terms:

• This should be "illegal":

  do_action(
  'upgrader_process_complete', $this, array(
      'action' => 'update',
      'type'   => 'core',
  )
  );

• This should be discouraged, possibly illegal too as it makes the source harder to read:

  do_action(
  'upgrader_process_complete',
  $this,
  array(
      'action' => 'update',
      'type'   => 'core',
  )
  );

• This should be the preferred/proper way as it is easiest to read:

  
  $param = array(
  'action' => 'update',
  'type'   => 'core',
  );

do_action( 'upgrader_process_complete', $this, $param );

To fix this we will need:
1. A rule that enforces proper multi-line parameters (see example 2 above). Ideally this will be only when at least one parameter is long, lets say over 50 characters.
2. A rule that flags or prevents single parameters that are on multiple lines.
3. Fix for the associative array rule to make it not apply inside function and filter/action calls. Ideally this will flag cases where an array is used "inline" and suggest setting it to a variable above the function/filter/action call.

[GaryJones]

At first glance at least, I could get behind this proposal.

I generally like keeping data away from what you do with that data (as is the example here), though it would often mean introducing single-use variables.

Extracting the array from how it's used also potentially leads to a refactoring opportunity:

protected function get_params() {
    return array(
        'action' => 'update',
        'type'   => 'core',
    );
}

...
do_action( 'upgrader_process_complete', $this, $this->get_params() );

[jrfnl]

It seems that the parser has introduced unwanted changes in single function and filter/action parameters: associative arrays were formatted to be multi-line. This was never intended in the original "Associative arrays should be multi-line" rule.

The ruleset was created on spec, with the spec being the handbook. This also applies to any sniffs within the WPCS library which are included in the WP-Core ruleset.

Over the course of the WP Core fixer project, there has been quite extensive discussion about particularly the rules you are now addressing and both the handbook, as well as the relevant sniff, have been adjusted because of it.

The sniff as-is has now been in a number of releases of WPCS and is being used by a significant nr of projects, not just by WP Core.

The fact that what is in the handbook does not accurately reflect your original intentions for the rule, does not constitute a bug in the sniff.

More than anything, it highlights a bug in the original process, where anyone with edit rights and an opinion could adjust the handbook.

This has - as you demonstrate with this issue - led to a situation were "hard rules" in the handbook are either worded incorrectly or open for too much interpretation leading to confusion and inconsistencies.

The RFC process fixes that bug by making sure that any significant changes to the CS rules and the handbook are scrutinized, discussed for their merit, discussed for the impact they will have on WP core and other projects using WPCS and that the choice of words is precise and correct.

The changes you are proposing have further reaching consequences than a bug-fix would have, so I kindly request you (again) to please use the RFC process when proposing significant changes to WPCS, as you are doing in this issue.

[pento]

Moving discussion here from the the Core tickets: WP#44600, WP#43644.

It seems like there are two parts to implementing a fix for this. First is to standardise multi-line function calls as only allowing one parameter per line. Core will shortly have a custom rule to enforce this, by adding:

    <rule ref="PEAR.Functions.FunctionCallSignature">
        <properties>
            <property name="allowMultipleArguments" value="false"/>
        </properties>
    </rule>

Moving this to WordPress-Core is easy enough.

The next step is to add a custom sniff that detects when a function call parameter is more than 1 line long, as @azaozz mentioned in his original comment. I agree that we should do this, let's make it a warning. (Ultimately, the aim is to fix all errors and warnings in Core, so the difference is kind of semantic.)

Of course, the Handbook needs to be updated to reflect these changes, as there's nothing that specifically requires either of these, but that's something we can easily rectify.

[jrfnl]

The next step is to add a custom sniff that detects when a function call parameter is more than 1 line long, as @azaozz mentioned in his original comment.

As mentioned in the referenced tickets. We need to discuss the implementation details of such a sniff. Opinions welcome.

Some open questions:

• How should such a sniff handle blank lines found in multi-line function calls ? And what about blank lines preceeding a comment line within the function call ?
• How should such a sniff handle comment lines found in multi-line function calls ? Example:

  printf(
      /* translators: 1: explanation; 2: explanation. */
      esc_html__('Text %1$s with %2$s replacements'),
      $replacement1,
      $replacement2
  );

• How should such a sniff handle long, formatted text strings in multline function calls ? Example:

  sprintf(
      '<div>
              <p><strong>%1$s</strong>%2$s</p>
      </div>',
      $replacement1,
      $replacement2
  );

• How should such a sniff handle heredoc/nowdoc (always multi-line) in multline function calls ?
• How should such a sniff handle nested multi-line function calls ? This includes closures passed as parameters.

  add_filter(
      'filter_name',
      function( $param ) {
          // Do something.
          return $filtered_param;
      }
  );

etc

[pento]

How should such a sniff handle blank lines found in multi-line function calls ? And what about blank lines preceding a comment line within the function call ?

Single blank lines are allowed, including before a comment line in a function call.

How should such a sniff handle comment lines found in multi-line function calls?

Single line comments are allowed. Multi-line comments are not.

How should such a sniff handle long, formatted text strings in multiline function calls?

Not allowed.

How should such a sniff handle heredoc/nowdoc (always multi-line) in multiline function calls?

Not allowed.

How should such a sniff handle nested multi-line function calls? This includes closures passed as parameters.

Ooo, that's a good one. I'm inclined to take a hard line on this for now, and say it's not allowed. The JS standards will continue to allow closures, as that more closely fits with the modern JS model, but we've never allowed closures in Core's PHP: I don't expect that to change in the near future.

[jrfnl]

we've never allowed closures in Core's PHP: I don't expect that to change in the near future.

Please take into consideration that WPCS is not just used by Core, but also by plugins and themes which may have dropped PHP 5.2 support already and do use closures (or multi-line long formatted text strings).

[pento]

Shouldn't those rules go into WordPress-Extra?

[jrfnl]

Shouldn't those rules go into WordPress-Extra?

The Extra ruleset includes the Core ruleset, so the sniff will be automatically included in the Extra ruleset.

As for having them just in Extra - I don't think having two different sniffs - one for Core, one for Extra - addressing the same is a good idea. We could add configuration options to say $allow_multiline_closures and $allow_multiline_textstrings, but having Extra allow more than Core is counter-intuitive.

Extra, generally speaking, adds additional rules. It doesn't change/remove/subtract from the core rules.

[pento]

Ah, thank you for explaining the difference.

It seems like adding some config options would be the move to make here: allow folks to continue their existing practices, and give us an easier change should Core ever start allowing closures, for example.

[jrfnl]

Single line comments are allowed. Multi-line comments are not.

Just to confirm with an example, you are saying the following would not be allowed:

$a = myfunction(
    $param1,
    /**
     * Filter docblock.
     * ...
     */
    apply_filter( 'filter_name', $value, $additional ),
);

[pento]

Correct, that should be moved to before the myfunction() call.

[jrfnl]

Reminder for when the "no multi-line parameters in function calls" sniff will be written:

This new sniff will create a functional conflict with the PreparedSQL sniff. SQL queries are often layed out over several lines for readability. The PreparedSQL sniff expects the query to be passed in directly to the $wpdb->...() methods and will throw errors when it hasn't been. This sniff will need adjustment and possible need to be rewritten to allow for queries defined outside of the function call. Also see #1331

[azaozz]

I'm inclined to take a hard line on this for now, and say it's not allowed. The JS standards will continue to allow closures, as that more closely fits with the modern JS model, but we've never allowed closures in Core's PHP: I don't expect that to change in the near future.

+1000 :)

Was thinking about that too. Lambda (anonymous) functions are generally harder to read, and hard to document properly. Passing a lambda function as a parameter to another function not only breaks the "no multiline params" rule but also skips some of the context. One of the good ways code can be "self-documenting" is to choose nice, descriptive function and var names (I know, that's often the hardest part of writing good code!). Lambda functions not only miss a docblock but also miss that opportunity for self-documenting.

I agree, as Gary mentions above using a lambda as a callback is a very well established pattern in JS. We should support that there, but thinking it is a very good idea to discourage it in PHP.

Also, thinking we can limit the cases where anonymous functions are allowed even in JS. A closure can be written in few different ways. For example, something like:

• Generally avoid lambda functions if at all possible. All functions should have a name and a docblock.
• One lambda function can be used as a (callback) param to another function. (Although we would still want a docblock for it somewhere around there.)
• No nested lambda functions.

This includes shortcuts like () => return 'bah!'. If a "full" docblock cannot be used, at least an inline comment should describe what's going on.

I understand, the above should probably be "recommendation" for JS, but hoping we can have a sniff that detects it and throws a warning/notice (or perhaps the "no multiline params" sniff will catch there too?).

[mundschenk-at]

Lambda functions not only miss a docblock but also miss that opportunity for self-documenting.

I'd strongly vote for a configuration switch. In my experience, a short lambda function used as a callback for, say, one of the array_* functions is much easier to read than having some a separate named callback method that is decoupled from its context. I don't see how forcing people to name single-use single-line functions is "good practice".

Also I'd ask you to reconsider function calls containing only a single array parameter when using the short array syntax in more modern PHP versions. I don't think that

$args  = [
    'number'       => 1,
    'meta_key'     => 'something',
    'meta_value'   => 'something',
    'meta_compare' => '=',
];
$users = \get_users( $args );

is in any way easier to read than

$users = \get_users( [
    'number'       => 1,
    'meta_key'     => 'something',
    'meta_value'   => 'something',
    'meta_compare' => '=',
] );

[lkraav]

I don't see how forcing people to name single-use single-line functions is "good practice".

True. Same for one-time closures that are never supposed to be unhooked or the whole work loses meaning (instead, you should deactivate the plugin, or similar). But I recognize this may be territory outside Core, thereby I'd be fine with overriding via local configuration.

[jrfnl]

But I recognize this may be territory outside Core

@lkraav Thank you for joining the discussion. WPCS is for Core and for the wider WP community. For this topic, we are still searching for a good balance which would work for both.

What's your view on the options suggested above, like $allow_multiline_closures ?

[lkraav]

$allow_multiline_closures and friends would certainly be nice, if it's maintainable (incl. documentation). Ideally you'd find the specific WPCS sniff options documentation page on the first page of Google as soon as you hit a sniff you think is weird.

[jrfnl]

@lkraav Documentation for all custom properties WPCS offers is available in the wiki: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki and these would be added there too. A quick check with Google on "WPCS sniff options" shows me that same page as the first result, so we must be doing something right :wink:

[azaozz]

First of all, please keep in mind these are "recommendations" that throw warnings, not errors. I understand there are some exceptions where some of these rules can be broken and the readability won't suffer that much. However in most cases they should be followed as that will ensure better inline documentation and readability with relatively very little effort.

In my experience, a short lambda function used as a callback for, say, one of the array_* functions is much easier to read than having some a separate named callback...

It depends. If that callback is so short that it would fit "comfortably" on one line, you are probably right. If it is not, you're probably better off defining it at separate place with a proper name, docblock, etc. Then at least half of the people trying to read your code will be thanking you :)

I'd ask you to reconsider function calls containing only a single array parameter when using the short array syntax

I'd rather the short syntax is not used in PHP. It's true, the coder will save 5 keystrokes typing, but that short syntax puts a tiny bit of strain on the eyes on all people that will read the code, every time they read it. So the question becomes: Is it worth not typing 5 chars but making everybody wear thicker glasses down the line? :)

The same "cause => effect" rule applies to excessive multi-level indentation. Thinking we should have a sniff to warn when there are more than three levels of indentation somewhere (regardless of the reason). Chances are that code can be written better and made more readable.

[lkraav]

First of all, please keep in mind these are "recommendations" that throw warnings, not errors.

Yeah, but the editor highlighting things for whatever reason is distracting.

The same "cause => effect" rule applies to excessive multi-level indentation. Thinking we should have a sniff to warn when there are more than three levels of indentation somewhere (regardless of the reason). Chances are that code can be written better and made more readable.

I like this a lot, but...

I'd rather the short syntax is not used in PHP. It's true, the coder will save 5 keystrokes typing, but that short syntax puts a tiny bit of strain on the eyes on all people that will read the code, every time they read it. So the question becomes: Is it worth not typing 5 chars but making everybody wear thicker glasses down the line? :)

... even though on surface this argument may look logical, I don't think I will ever be convinced to go back to array again. [] just works so smooth.

EDIT replying to @azaozz https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1330#issuecomment-421545146 - yes, lot of "I" in this claim, but only because I'd also bet money on an honest vote with sufficient sample size quantifying the developer preference towards [], rather than going back to array.

[mundschenk-at]

I'd rather the short syntax is not used in PHP. It's true, the coder will save 5 keystrokes typing, but that short syntax puts a tiny bit of strain on the eyes on all people that will read the code, every time they read it. So the question becomes: Is it worth not typing 5 chars but making everybody wear thicker glasses down the line? :)

... even though on surface this argument may look logical, I don't think I will ever be convinced to go back to array again. [] just works so smooth.

I don't think it even looks logical. Mandating array() for post-PHP5.3 code seems out of touch with development practices outside of WordPress Core and its PHP5.2 fixation.

Why is it more difficult to read [ ... ] (unambiguously an array, either a literal or an array access) than array( ... ) (the brackets can mean a lot of different things, from function calls and a variety of function-like language constructs to type casts, loops, etc.)?

[azaozz]

I don't think I will ever be convinced to go back to array again. [] just works so smooth.

I don't think it even looks logical

@lkraav @mundschenk-at, we are not talking about "personal preferences" here. I'm really not sure how to understand "We want it because it is smooth" in this context. We are talking about (basic) readability as dictated by simple/basic ergonomics. This is pretty much the same for too much indentation, too long or too short lines, spaces around ( and ), etc. etc. Personal "feelings" have no place here :)

(BTW I personally don't mind using [] for arrays as I've started with JS before PHP. However this shortcut does indeed make things a tiny bit harder to read, especially for people that are not used to it.)

...the editor highlighting things for whatever reason is distracting.

Hmmm, right. Would it be possible to have several levels (two-three) of "warnings" that can be turned on and off? That way you will be able to continue using whatever shortcuts you prefer in your own code, but not force them on "everybody", and we can turn all warnings "on" for core.

[mundschenk-at]

@azaozz, how is your preference for array() anything other than a personal preference? How is it easier to read than []? Because it spells out array? Then you would have to also always use the alternate forms for conditions and loops (i.e. "Pascal-like" syntax). To me it would appear to be the other way round, array() is easy to mistake for a function call on a quick glance.

[azaozz]

how is your preference for array() anything other than a personal preference? ... Because it spells out array?

Right, because it spells out array which makes it impossible to miss. And it is not a personal preference (as I explained above), it is dictated by "readability ergonomics". These are the same rules which state that too long lines are harder to read, and become impossible when far too long (1000 chars and more). On top of that they cause eye strain leading to certain medical conditions, etc. etc.

I agree, the example of array() vs. [] is really small and subtle, but the principle is important here too :)

And yes, I personally prefer Python's way of doing things: "explicit is better than implicit" as a core design rule. There are also some "thoughts" on ergonomics from the Rust language's blog: https://blog.rust-lang.org/2017/03/02/lang-ergonomics.html.

[mundschenk-at]

Right, because it spells out array which makes it impossible to miss.

"Impossible"? I highly doubt that. It looks like a function call, so in my experience, it's actually more difficult to spot when skimming.

And it is not a personal preference (as I explained above), it is dictated by "readability ergonomics".

You continue to claim this, but that's not any kind of proof. Not to be misconstrued: I do not say that certain ways to write code do not differ in readability. After all, that's why we use coding standards. But in this case, it is not at all self-evident that (or even "common knowledge").

On top of that they cause eye strain leading to certain medical conditions, etc. etc.

Again, I find it doubtful that one or the other array syntax is causal for any medical condition (or that there is any significant difference in eye strain between reading code using one or the other variant). If you can cite a study to that effect, OK, but otherwise it sounds like rationalization for a personal preference.

And yes, I personally prefer Python's way of doing things: "explicit is better than implicit" as a core design rule.

Funny then that Python lists and dictionaries are using [] and {}.

[azaozz]

@mundschenk-at Ugh, sorry, seems we are not on the "same page" here. Lets try this again :)

It looks like a function call, so in my experience, it's actually more difficult to spot when skimming.

"Reading ergonomics" refers to reading in general, from printed media or a computer screen. There are numerous studies on reading and readability conducted through the years. One of the conclusions is that the chance to misread a single character is greater than the chance to misread a whole word.

In your own experience that may not be the case as you are familiar with the meaning of that single character in PHP code. However not all people that will read the same text (code) will be familiar with it, hence I don't think it is a good idea to force this "coding syntax shortcut" on them. Furthermore this syntax shortcut is not currently supported in core, and because of the above reason I don't think it is a good idea to start supporting it now.

Somewhat unrelated:

Funny then that Python lists and dictionaries are using [] and {}.

If I understand your remark correctly, it seems this is another misunderstanding. Sorry for not expressing myself clearer. What I mean to say is that I personally (i.e. not related to this issue) prefer Phyton's philosophy "explicit is better than implicit", i.e. using whole words is always better than an abbreviation, an acronym or a shortcut. It makes any text, including code, easier to read especially for people that are not very familiar with particular language or syntax. Please read a bit more about it here: https://miguelgfierro.com/blog/2018/python-pro-tips-understanding-explicit-is-better-than-implicit/

[Rarst]

(Repeating some of my thoughts from slack on request)

In my opinion the easy way to write that example would be:

do_action( 'upgrader_process_complete', $this, array(
    'action' => 'update',
    'type'   => 'core',
) );

PSR–12 draft formalizes this practice very coherently:

A single argument being split across multiple lines (as might be the case with an anonymous function or array) does not constitute splitting the argument list itself.

https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#47-method-and-function-calls

I was a little surprised to check the current state of WP's PHP standard and find a hard rule for single line arguments. I don't think that fits with how PHP (actual PHP, not WordPress PHP) is commonly written, even more so for closures than arrays.

The main merit of coding style is in familiarity, not some "natural" readability. If readability was natural everyone would write in a same way and a coding style would be utterly useless concept.

I would like to see WP consider literally many years of established use of closures and meaningfully incorporate it, not denounce it as "not PHP 5.2 = bad".