Page MenuHomePhabricator

Recommend PHP7-only features after dropping HHVM support
Closed, ResolvedPublic

Description

There are various PHP7-only features that we should IMHO recommend in our coding conventions and during code review, but that we cannot currently use with HHVM. Note, this task is not meant to be a tracking task. It should collect what we think are the most useful features, and can be considered resolved as soon as all of these features are either recommended on mw.org, or enforced via PHPCS depending on the single case. The ones I can think of now: (see T231710#5520397)

Also, note that this task shouldn't become a lame list of new PHP7 features. Everything included here should either improve (static) analysis, code's self-documentation, or overcome annoying HHVM limitations that force(d) us to write worse code (e.g. constant arrays and variargs).

Details

Related Gerrit Patches:

Event Timeline

Daimona created this task.Aug 31 2019, 8:27 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 31 2019, 8:27 AM

It looks like soon we're probably going to require PHP 7.2, so features from 7.1 and 7.2 should be considered as well. These pages are useful:

https://www.php.net/manual/en/migration71.php
https://www.php.net/manual/en/migration72.php

It looks like soon we're probably going to require PHP 7.2, so features from 7.1 and 7.2 should be considered as well. These pages are useful:
https://www.php.net/manual/en/migration71.php
https://www.php.net/manual/en/migration72.php

Yes, probably. Nullable typehints (included in the description) are PHP71+. I skimmed through the new features and didn't find anything new to add to the list. But I may have missed something, of course.

7.1:

  • void return value for functions, for the same reason we want to type-hint generally
  • Convert list() to [] just like we converted array()
  • Specify visibility of class constants (some are only used internally and should be made protected or private)

7.2:

  • object type hint where appropriate

7.1:

  • void return value for functions, for the same reason we want to type-hint generally

Yes, although I've not commonly seen the void type in docblocks, nor PHPCS requires it (obviously). So I'm unsure whether it should be recommended.

  • Convert list() to [] just like we converted array()

This isn't urgent IMHO. It's just an alternative syntax, with roughly the same readability. We can of course discuss and eventually recommend it, but I don't think it should be prioritized.

  • Specify visibility of class constants (some are only used internally and should be made protected or private)

Agreed.

7.2:

  • object type hint where appropriate

Yes. Although usually, at least in the docblock, it would be better to specify all classes. And I'm unsure whether PHPCS lets you use @param Class1|Class2 $x in the docblock and object $x in the signature.

Krinkle moved this task from Backlog to Other on the PHP 7.0 support board.Sep 6 2019, 12:24 AM

Change 538726 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/tools/codesniffer@master] Remove prohibitions on new PHP features

https://gerrit.wikimedia.org/r/538726

Daimona updated the task description. (Show Details)Sep 24 2019, 9:09 AM

Explicit variargs in method signatures

I’m not sure what the “explicit” part means, but ...$args in parameters is also supported in HHVM, and we’ve already been using it in MediaWiki for a while (example)

Explicit variargs in method signatures

I’m not sure what the “explicit” part means, but ...$args in parameters is also supported in HHVM, and we’ve already been using it in MediaWiki for a while (example)

Yes, however there's a bug in HHVM reflection which prevents method with variargs from being mocked: Reflection adds an array typehint to the parameter, and array ...$args is not supported by HHVM. See for instance T191668#5263929, or T228695#5450795. Various PHPUnit tests are disabled under HHVM due to that.

That's also (partly) why some methods like ContextSource::msg aren't using variargs - see https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/534189/

Based on my original list, I think this is what we should do:

Scalar typehints

Remove PHPCS rule (patch above), ensure they're recommended on mw.org.

Explicit variargs in method signatures

Add a PHPCS rule to forbid things like function foo( /* ...$params */ ) with all the variations. Ensure that we don't tolerate variargs documented in the method's docblock, but absent in its signature [1]

Constant arrays

I think it's enough to enforce it during code review.

Nullable typehints (i.e. ?MyClass $x instead of MyClass $x = null) - PHP71+

This deserves a line on mw.org, and a PHPCS rule.

Visibility on class constants

Same as constant arrays, enforcing during code review is probably enough.


[1] - I think PHPCS allows variadic arguments to be documented without actually being in the signature; we'd have to check https://github.com/wikimedia/mediawiki-tools-codesniffer/blob/master/MediaWiki/Sniffs/Commenting/FunctionCommentSniff.php to see if it's actually the case.

Daimona updated the task description. (Show Details)Sep 25 2019, 9:46 AM

Published this edit to the coding conventions for scalar typehints and nullable arguments. At this point, I think that anything left can be handled within PHPCS.

Change 539177 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/codesniffer@master] Improve checks for variargs

https://gerrit.wikimedia.org/r/539177

Now we only need a PHPCS rule to enforce nullables. I'll probably write it tomorrow. The problem is differentiating optionals from nullables. I think a safe approach could be to just emit a warning if we find ClassName $varname = null followed by another non-optional (and non-nullable) parameter.

Change 539308 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/codesniffer@master] Add rules for PHP71 nullable types

https://gerrit.wikimedia.org/r/539308

Daimona updated the task description. (Show Details)Sep 26 2019, 12:39 PM

Change 538726 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Remove prohibitions on new PHP features

https://gerrit.wikimedia.org/r/538726

Daimona updated the task description. (Show Details)Oct 3 2019, 6:22 PM

Change 540830 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Use varargs for IDatabase::buildLike

https://gerrit.wikimedia.org/r/540830

Change 541021 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] PPFrame: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541021

Change 541023 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] PermissionManager: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541023

Change 541024 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] BaseTemplate: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541024

Change 541023 merged by jenkins-bot:
[mediawiki/core@master] PermissionManager: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541023

Change 541021 merged by jenkins-bot:
[mediawiki/core@master] PPFrame: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541021

Change 541024 merged by jenkins-bot:
[mediawiki/core@master] BaseTemplate: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/541024

Change 539308 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Add rules for PHP71 nullable types

https://gerrit.wikimedia.org/r/539308

Daimona updated the task description. (Show Details)Oct 8 2019, 4:49 PM

Change 539177 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Improve checks for variargs

https://gerrit.wikimedia.org/r/539177

Daimona closed this task as Resolved.Oct 8 2019, 5:37 PM
Daimona claimed this task.
Daimona updated the task description. (Show Details)
Daimona removed a project: Patch-For-Review.

Change 544254 had a related patch set uploaded (by Reedy; owner: TK-999):
[mediawiki/core@REL1_34] PPFrame: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/544254

Change 544256 had a related patch set uploaded (by Reedy; owner: TK-999):
[mediawiki/core@REL1_34] BaseTemplate: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/544256

Change 544254 merged by jenkins-bot:
[mediawiki/core@REL1_34] PPFrame: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/544254

Change 544256 merged by jenkins-bot:
[mediawiki/core@REL1_34] BaseTemplate: Use explicit varargs in method parameters

https://gerrit.wikimedia.org/r/544256