Revert multiline calls to actions and filters

[johnbillion]

The docs parser that generates the documentation for developer.wordpress.org doesn't currently support multiline calls to actions and filters. The code is poetry changes in #41057 introduced 44 of these. They should be reverted for consistency and so the docs parser can continue to work until such time that it's updated to support multiline calls.

Refs:

• ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1323

[johnbillion]

43644.diff​ is a partial patch which shows the sort of changes that would be made. Note that some of the actions, for example wp_handle_upload, are currently an odd sort of broken multiline call but with some parameters still on the end of a single line.

[jrf]

Just out of curiousity: where is the sister-ticket for the doc generator ?

IMO, fixing the doc generator to handle this should have priority over undoing previously made CS fixes.

[ocean90]

While I think multiline calls are fine per se, the "code is poetry" commit has made them actually not so poetry IMO. Comparing wp_comment_reply and list_cats, wouldn't it be more readable if wp_comment_reply would be like this:

<?php
$content = apply_filters(
        'wp_comment_reply',           // First arg.
        '',                           // Second arg.
        array(                        // Third arg.
                'position' => $position,
                'checkbox' => $checkbox,
                'mode'     => $mode,
        )
);

[jrf]

@ocean90 I totally agree and "proper" multi-line function calls, i.e. one argument per line, are actually quite easy to enforce as there are already sniffs available to do this.

However, this is not something the PHP CS standards in the handbook currently proscribe, so this was, for that reason, not enforced for the big CS patch.

[azaozz]

However, this is not something the PHP CS standards..

Lets add it then. The above example for wp_comment_reply is really hard to read.

[jrf]

@azaozz For significant changes to the WordPress Coding Standards, an RFC process has been put in place: ​https://github.com/WordPress-Coding-Standards/rfcs

I look forward to the proposal 👍

[azaozz]

Replying to jrf:

Who made the proposal to have these hard to read bits in the coding standards in the first place? I've never seen that formatting in core before. Lets review that first! :)

[jrf]

Who made the proposal to have these hard to read bits in the coding standards in the first place? I've never seen that formatting in core before. Lets review that first! :)

I've not been involved in the original creation of the Core coding standards, so I cannot answer that question.

However, most of these kind of function call layout changes come from the WP Core rule that ​multi-item associative arrays should be multi-line and the fixers kicking in to change the code from single line to multi-line arrays.

[ocean90]

Replying to azaozz:

I'd go one step further: for best readability arrays should be defined outside of function and filter calls.

That's a nice workaround that I'd support too.

Replying to jrf:

@azaozz For significant changes to the WordPress Coding Standards, an RFC process has been put in place: ​https://github.com/WordPress-Coding-Standards/rfcs

I don't think we need a RFC process for this or at all. That's not really straightforward and just delays stuff unnecessarily. WordPress has worked pretty well without them. If you need them for WPCS sniffs that's fine, of course.
What we need here is 1-2 sentences with a small example for the Indentation section on our handbook:

For multi-line argument lists, each argument should start on a new line and indented once. There should be only one argument per line.

Note that there's already a similar example in the ​Opening and Closing PHP Tags section, added 9 months ago.

[jrf]

I don't think we need a RFC process for this or at all.

@netweb Could you please weight in on this ?

[azaozz]

...most of these kind of function call layout changes come from the WP Core rule that multi-item associative arrays should be multi-line and the fixers kicking in to change the code from single line to multi-line arrays.

Ah, I see. So it was mostly accidental that these arrays changed. That would explain it :)

[netweb]

Replying to ocean90:

I don't think we need a RFC process for this or at all. That's not really straightforward and just delays stuff unnecessarily. WordPress has worked pretty well without them. If you need them for WPCS sniffs that's fine, of course.
What we need here is 1-2 sentences with a small example for the Indentation section on our handbook:

For multi-line argument lists, each argument should start on a new line and indented once. There should be only one argument per line.

Agreed, if the handbook needs updating update it, the RFC process has not been implemented to add bureaucracy for the sake adding bureaucracy, its for major changes as @azaozz just wrote ​on GitHub:

Also, the RFC process is good when proposing major new changes but IMHO shouldn't apply when fixing bugs :)

I've added a few more thoughts on this RFC thing being implemented on GitHub:

​https://github.com/WordPress-Coding-Standards/rfcs/issues/9#issuecomment-376827944

[netweb]

Replying to netweb:

Agreed, if the handbook needs updating update it, the RFC process has not been implemented to add bureaucracy for the sake adding bureaucracy, its for major changes as @azaozz just wrote ​on GitHub:

I should call myself out on this, as the consideration I've missed mentioning is that the plan is for core to run PHPCS for each commit on Travis CI.

To do that effectively having the handbook edited and the coding standards changed before the WPCS team has the opportunity to update and/or create any new PHPCS sniffs is not a workflow we will be able to operate under in the near future.

With the transition of the coding standards handbooks to ​https://developer.wordpress.org/coding-standards/ the pull request noting those changes, any WPCS PHPCS sniffs, and any core tickets and patches will all require coordination between each of the teams involved.

The hope is that this can be handled by the RFC process we've started building...

[pento]

Well, this has certainly dragged on. As the RFC process didn't really go anywhere, let's do the following:

• WPCS 1.0 is due out this week, Core will upgrade to using that, and start enforcing coding standards on commit. (#30153)
• @ocean90's suggestion in comment:4 is a good first step, we can easily enforce this in Core's phpcs.xml.dist, if it doesn't make it into WPCS 1.0.
• @azaozz's suggestion is a good next step, but I imagine it wouldn't be auto-fixable. We can add a sniff for it to be manually fixed.

I don't know if the PHPdoc parser will need work to handle this format, someone familiar with it should probably check that out.

[pento]

To address this issue, I'm leaning towards adding this to the phpcs.xml.dist file:

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

This will ensure that each parameter is on a seperate line.

@johnbillion: Will the PHPdoc parser be able to handle the format that @ocean90 proposed earlier?

[azaozz]

I'm actually thinking we should *ban* multi-line function parameters:

• There is no good, practical reason to have them. They only make some piece of coding harder to read.
• In core most were introduced "by mistake" last time the source code was reformatted to match the supposedly "core's own" coding standards. I see that as a bug that needs fixing.

I agree that the cute little example by @ocean90 above doesn't look that bad. But that's just an example. In reality allowing multi-line stuff inside of function calls would mean that we "agree" to this type of coding:

<?php
$content = apply_filters(
        'wp_comment_no_reply',           // First arg.
        '',                           // Second arg.
        array(                        // Third arg.
                'position' => $position,
                'checkbox' => $checkbox,
                'mode'     => $mode,
        ),
        esc_html(
                sprintf(
                        __( 'This is a long piece of text that should never be inside of a function call. Even worse: it might end up in a nested multi-line function call. %s, %s.' ),
                        add_query_arg(
                                array(
                                        'bah_where_am_i'     => 'undefined',
                                        'how_did_i_get_here' => 'who-knows',
                                ),
                                remove_query_arg(
                                        array(
                                                '_wp_http_referer',
                                                '_wpnonce',
                                                'error',
                                                'message',
                                                'paged',
                                        ),
                                        wp_unslash( $_SERVER['REQUEST_URI'] )
                                 )
                        ),
                        '<p>' . __( 'This is another long piece of text that should never be inside of a function call...' ) . '</p>'
                )
        )
);

Frankly, this code stinks. There is no practical reason to be written that way. It's hard to read, has a lot of pointless indentation, and is by no means "Poetry". Don't see why the coding standards should allow that.

[pento]

I agree, that's some ugly code.

As with everything, there are steps we can take to improve the situation. The first thing we can do is enforce the proposed "one argument per line" rule, as there's an existing sniff and fixer for it, and it applies to the vast majority of function calls. (Where "vast majority" is entirely unscientific, and based on my brief read through a handful of core files.)

I don't know if a sniff exists to block multline arguments, even if there is, I assume there isn't a fixer. I'm fine with updating Core's coding standards to prevent multiline arguments, and then proceeding with the ​corresponding WPCS ticket.

[peterwilsoncc]

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

[pento]

Closing this one as fixed. #44600 put each parameter on it's own line. For the larger problem of multiline parameters, this will need to be handled in ​WPCS1330.