Require parentheses when constructing classes

[westonruter]

In WordPress's JSHint config there is an error that gets raised when you try doing this:

x = new Thing

Missing '()' invoking a constructor.

This is likewise a coding convention in WordPress's PHP to always use the parentheses when constructing a class, however this is currently not getting enforced by PHPCS. I actually haven't been able to find an existing sniff that checks for that issue, but it seems like something that should be in Squiz.

[jrfnl]

This is likewise a coding convention in WordPress's PHP to always use the parentheses

@westonruter I can't find anything in the handbook about this and a search of core gives very mixed results:

• Class instantiation without arguments or parenthesis new Something; used in approx 45 files
• Class instantiation without arguments with parenthesis new Something(); used in approx 92 files

PHP technically, there is absolutely nothing wrong with leaving the braces off when not passing arguments to a constructor. I could argue it actually makes for cleaner code.

FYI: Regarding JS: there is a discussion over in https://github.com/WPTRT/WordPress-Coding-Standards/issues/120 about adding the ESLint sniff (at least for themes) with a list of best practices which could be added.

[westonruter]

I can't find anything in the handbook about this and a search of core gives very mixed results:

Here are results on specific instances as opposed to file counts:

• new X();: 623 instances via ack 'new \w+\(\);' --no-group --php src/wp-admin/ src/wp-includes/ tests/
• new X;: 123 instances via ack 'new \w+;' --no-group --php src/wp-admin/ src/wp-includes/ tests/

So there are 5 times more instances of using parentheses than not. For consistency's sake, I think one style should be advised as the standard convention.

PHP technically, there is absolutely nothing wrong with leaving the braces off when not passing arguments to a constructor. I could argue it actually makes for cleaner code.

There is nothing wrong with leaving braces off in JS as well. But

[jrfnl]

Maybe something which should be in https://github.com/GaryJones/PHP-Coding-Standards ?

[GaryJones]

If it's not in the handbook already, a sniff could definitely go into WordPress-Extra, until such time that the handbook was updated.

[jrfnl]

As we're working on the core fixes now and updating the handbook for related things, I'm wondering if this is a good time to revisit this discussion and take a decision.

@pento @ntwb Have you got an opinion on this ?

While we're at it, we should also consider if this suggested rule should (also) be applied to anonymous classes.

And how should new self, new parent or new static be handled ? Demand parenthesis or not ?

On the practical side of things: There are currently no sniffs upstream which cover this, so a new sniff would need to be written which potentially could/should be upstreamed at some point.

If we're writing a new sniff for this anyway, what else should be covered by this ? Some ideas:

• No assigning new by reference (deprecated in PHP 5.3, removed in PHP 7 - see http://php.net/manual/en/migration70.incompatible.php#migration70.incompatible.other.new-by-ref ). This is of course covered in the PHPCompatibility standard, but not everyone uses that.
• Have we got an opinion on object chaining on instantiation ?, i.e. ( new MyClass() )->my_function();

[GaryJones]

I'm fine with ( new MyClass() )->my_function(); - comes in handy for unit tests.

I thought PSR-2 had a note about always using parenthesis for new classes (my preference, even with no constructor arg), but I can't spot that reference now.

[jrfnl]

I thought PSR-2 had a note about always using parenthesis for new classes

I tested with each of the PHPCS native standards and none throw an error, so this is not covered by PSR2.

[GaryJones]

Current figures:

No parentheses

ack 'new \w+;' --php -hc src/wp-admin/ src/wp-includes/ tests 119

Parentheses

ack 'new \w+();' --php -hc src/wp-admin/ src/wp-includes/ tests 658

So that's about 85% in favour of using parentheses. Gets my vote.

[jrfnl]

Just thinking - a sniff for this could easily also check & potentially auto-fix this in JS files. Should it ?

[jrfnl]

I've just ran some metrics on core using PHPCS and the results are even more convincing (and accurate) with nearly 95% of all instantiations using parenthesis:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Assigning new by reference: no [1476/1476, 100%]

Object instantiation with parenthesis:
        yes [1394/1476, 94.44%]
        no => 82 (5.56%)
----------------------------------------------------------------------

The most prevalent exception I've seen is new stdClass;

I'm wondering about the lower nr of no parenthesis, but suspect that is because I excluded JS files from my analysis.

[GaryJones]

Not sure where the discrepancy is, but for JS files:

ack 'new \w+;' --js -hc src/wp-admin/ src/wp-includes/ tests 2

ack 'new \w+();' --js -hc src/wp-admin/ src/wp-includes/ tests 79

[jrfnl]

Not sure where the discrepancy

Ah, ok, so the original nrs where - just like mine - only PHP.

By the way, my numbers are based on a run only on /src, so do not include /tests

I think I can explain why my totals are a lot higher: new $className() and variants of that would not be caught by the ack regex, nor would new \ClassName though I suspect that is not used (yet) in core.

And after analysing the files identified by my PHPCS run - with only two false positives and no false negatives in the files inspected - I suspect that the lower nr of no parenthesis might be because of docblock/inline comments containing code samples, which would be caught by the regex, but not by PHPCS.

To know for sure, we'd have to look at the difference in files where cases of no parenthesis are identified between our two runs.

[pento]

Opinions!

Always required parentheses, including for self, parent, static, and stdClass.

( new MyClass() )->my_function(); is PHP 5.4+ syntax, but I'm fine with defining it as the Core standard, for when we can use it.

Fixing it in JS would be good, too.

[jrfnl]

@pento Thanks for pitching in. What would be the rule for the object chaining though ? That we allow it ? In that case, we don't need to do anything for it.

Regarding the rest - I've got a sniff nearly ready which will check the following for both PHP as well as JS:

• Always use parenthesis.
• Don't have spaces between the class name and the parenthesis.
• Don't assign new by reference. (only relevant for PHP).

The only thing the sniff currently does not cover is variable variables used for the class name - i.e. new ${$classnamePtr);. I don't think that's used in core anyhow, but it might be in userland code.

For the sniff to be pulled to the WordPress-Core ruleset, something along the lines of the above three bullets should be added to the handbook, providing everyone agrees upon this, of course.

[jrfnl]

@GaryJones The figures I get for JS files, excluding the /tests directory and excluding files containing minified code:

Object instantiation with parenthesis:
        yes [558/560, 99.64%]
        no => 2 (0.36%)

(at least the no / 2 is the same here :wink: )

[pento]

What would be the rule for the object chaining though?

We already use it in Core, in the style $foo->bar()->baz(). We can clarify that it's explicitly allowed in the standards doc.

I don't think we multiline format object chaining, like:

$foo->bar()
    ->baz()
    ->bat();

I'm not sure how to define a rule to say when to use single line and when to use multiline.

[jrfnl]

I'm not sure how to define a rule to say when to use single line and when to use multiline.

AFAICS there is no rule in the handbook about maximum line length, so I'd leave it out (of the handbook) for now.

Regarding object chaining and what - if anything - should be checked for: that should be another issue really. This issue is about class instantiation using the new keyword and chaining happens more often with already instantiated objects than with objects being instantiated.