Spacing in closures

[mboynes]

Many of my Travis-tested repos just started failing due to the merge of #417. I see that WPCS now wants:

add_filter( 'excerpt_length', function() {
    return 40;
} );

to be:

add_filter( 'excerpt_length', function () {
    return 40;
} );

I see no mention of this in the WordPress Coding Standards. Is there a secondary guide that WPCS is using to make decisions like this? If it's a matter of opinion, I'd love to see the discussion so I can better understand why this format was chosen. I personally think function() { makes more, as it follows the non-closure function standard more closely, where the parens are adjacent to the function name like function foo() {. If it were just a matter of opinion, I would hide it in my ruleset, though it doesn't look like this can be ignored without also affecting other control structures (please correct me if I'm wrong) covered by WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterStructureOpen and WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeOpenParenthesis.

Thanks!

[lkraav]

Yes, this is ouch. I was just about to file the same issue.

EDIT it's possible to override it in your own project configuration #417 (comment)

<rule ref="WordPress.WhiteSpace.ControlStructureSpacing">
    <properties>
        ...
        <property name="spaces_before_closure_open_paren" value="0" />
    </properties>
</rule>

[GaryJones]

I raised this at #417 (comment) . In this case, I can only assume that @JDGrimes defaulted (or let the Squiz sniff default) to PSR-2, which does have spaces: http://www.php-fig.org/psr/psr-2/#6-closures

Anonymous functions / closures tend not to be found in Core, so things like this, namespaces and other non-core goodies are something that I've raised with @DrewAPicture so that we can get it clarified in the handbooks regardless of whether WP core is using it.

Drew, can this particular point be raised sooner with the Core team, so we can amend as necessary? FWIW, no-space (like WP CS JS uses) is my slight preference. The only occurrence I could see in core was in tests/phpunit/actions/closures where no-space is used twice. If this is sufficiently indicative, then a change will be needed in WPCS.

[JDGrimes]

I don't have a strong preference either way, but I tend to use the space. ("When in doubt, space it out!" 😈) But we can make 0 the default, or even have it default to no preference until core makes a decision.

[GaryJones]

The fact that 100% of instances have no-space in core (OK, so 2 out of 2, in tests (!)), named function declarations don't have a space, and anonymous functions in JS don't (and there being some intent to have consistency between JS and PHP), I think I'd prefer no preference to be expected for function(), until a decision is made otherwise.

[JDGrimes]

OK, should we release a 0.7.1 with that change?

[westonruter]

@JDGrimes great idea 👍

[GaryJones]

+1 for 0.7.1. I meant to suggest that in my previous comment.

[mboynes]

Thanks for the quick turnaround!

[schlessera]

I'm not happy with the reasoning behind this decision, as I would argue that it does not make sense at all.

A named function declaration would be function whatever( $item ) { [...] }.

In a closure, what gets omitted is the name of the function, so you'd remove the whatever above. This leaves you with function ( $item ) { [...] }.

You're NOT passing arguments to function, you're declaring an unnamed entity of type function with so and so arguments, just like you would have int 5 instead of int5...

[GaryJones]

@schlessera also pointed out that PHPStorm doesn't even have the Coding Style option (yet?) to remove that space.

[schlessera]

Haha, apparently they consider this to be a bug in PHPStorm...

Oh well, nevertheless, I maintain my arguments, it is not a call to function with arguments, it is function <no-name-here>( $item ) {}...

[JDGrimes]

@schlessera I agree, which is why I use the space. But this project doesn't control the standards that WordPress uses, we just implement sniffs for those standards. We need to be compatible with WordPress's declared standards by default, which is why this change was made. Those of us that disagree with WordPress's standards can try to get them changed. Until that happens, you can override the default in your phpcs.xml config:

<rule ref="WordPress.WhiteSpace.ControlStructureSpacing">
    <properties>
        ...
        <property name="spaces_before_closure_open_paren" value="1" />
    </properties>
</rule>

[schlessera]

Yes, I agree with that, but in this case, WordPress does not have any standard to speak of. The two instances that @GaryJones managed to find can't possibly be considered conclusive.

So, why not just ignore this until Core has made a decision?

Yes, I'll consider overriding it in my projects. Thanks!

[JDGrimes]

Yes, I agree with that, but in this case, WordPress does not have any standard to speak of. The two instances that @GaryJones managed to find can't possibly be considered conclusive.

No, those instances aren't conclusive. I would have favored turning off this check entirely by default, except that it is a part of the JavaScript standards, and a strong emphasis has been placed on parity between the PHP and JS standards in regards to whitespace.

I'm still not against ignoring this check by default until it is clarified in the PHP standards. However, that will mean that everyone will need to implement a custom configuration if they want to sniff for that. @GaryJones @westonruter any thoughts?

[westonruter]

I think it should be ignored by default. It can be an opt-in for now, and once Core has a standard for closure formatting, we can enable the check.

[GaryJones]

As per 24 days ago:

I think I'd prefer no preference to be expected for function(), until a decision is made otherwise.

---

Those of us that disagree with WordPress's standards can try to get them changed
Unfortunately, there isn't a Slack channel or other method that can deal with this, without conversations getting lost in core chat.

@DrewAPicture has taken my suggestions (raised in Docs chat) about the PHP CS listed in the Handbooks to core devs, but really, the choosing of standards for use in the community should be a separate group. They'd (core and non-core devs) decide on the standards (with strong influence from historical use in core, obviously), the standards are written up via the Docs team, given to WPCS to be programatically testable, and then core, plugin and theme authors could use them. This lack of the initial CS group is just frustrating.

[JDGrimes]

As per 24 days ago:

I think I'd prefer no preference to be expected for function(), until a decision is made otherwise.

Ah, I must have missed that somehow. I don't know why I defaulted it to 0. (But don't blame me, you're the one who merged it 😄).

[GaryJones]

But don't blame me, you're the one who merged it

I deferred to your expert decision :-)