WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 3 months ago

#43644 accepted task (blessed)

Revert multiline calls to actions and filters

Reported by: johnbillion Owned by: johnbillion
Milestone: 5.0 Priority: normal
Severity: normal Version: trunk
Component: General Keywords: needs-patch 2nd-opinion
Focuses: docs, coding-standards Cc:

Description

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:

Attachments (1)

43644.diff (16.0 KB) - added by johnbillion 3 months ago.

Download all attachments as: .zip

Change History (15)

#1 @johnbillion
3 months ago

  • Owner set to johnbillion
  • Status changed from new to accepted

@johnbillion
3 months ago

#2 @johnbillion
3 months ago

  • Keywords 2nd-opinion added

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.

#3 @jrf
3 months ago

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.

#4 @ocean90
3 months ago

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,
        )
);

#5 @jrf
3 months ago

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

#6 @azaozz
3 months ago

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.

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

$array = array(
   'position' => $position,
   'checkbox' => $checkbox,
   'mode'     => $mode,
);

$content = apply_filters( 'wp_comment_reply', '', $array );
Last edited 3 months ago by azaozz (previous) (diff)

#7 follow-up: @jrf
3 months ago

@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 👍

#8 in reply to: ↑ 7 @azaozz
3 months ago

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! :)

Last edited 3 months ago by azaozz (previous) (diff)

#9 @jrf
3 months ago

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.

#10 follow-up: @ocean90
3 months ago

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.

#11 @jrf
3 months ago

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

@netweb Could you please weight in on this ?

#12 @azaozz
3 months ago

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

#13 in reply to: ↑ 10 ; follow-up: @netweb
3 months ago

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

#14 in reply to: ↑ 13 @netweb
3 months ago

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

Note: See TracTickets for help on using tickets.