Page MenuHomePhabricator

Make use of PHP 8.0 and 8.1 features in existing code
Open, Needs TriagePublic

Description

PHP 8.0 and 8.1 come with lots of new stuff. There are some major features that will be useful in new code, such as union and intersection type annotations, named arguments, attributes, enums, or readonly properties.

But there are also many smaller, straightforward syntax improvements that could be used to improve our existing code, in MediaWiki as well as extensions:

I'd like us to have a good look at them, and figure out:

  • Which of them we could require to be used with PHPCS, Phan or other tools, and introduce semi-automatically with regexp replacements and careful review?
  • Which of them we could introduce entirely automatically with PHPCBF or other tools?

We should also review the code conventions on MediaWiki.org and see if anything should be updated to recommend these.

We can use these features today (following T328921 and T319432), but we may want to wait with large-scale changes until support for MediaWiki 1.39 is dropped in November, to avoid inconvenience when backporting patches.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/extensions/FileImportermaster+160 -377
mediawiki/extensions/ArticlePlaceholdermaster+17 -91
mediawiki/extensions/Wikistoriesmaster+69 -358
mediawiki/coremaster+40 -141
mediawiki/coremaster+305 -260
mediawiki/extensions/Echomaster+173 -568
mediawiki/extensions/ReportIncidentmaster+41 -121
mediawiki/extensions/CategoryTestsmaster+8 -13
mediawiki/extensions/Babelmaster+23 -49
mediawiki/coremaster+55 -55
mediawiki/coremaster+61 -163
mediawiki/extensions/TimedMediaHandlermaster+63 -140
mediawiki/extensions/Phonosmaster+32 -59
mediawiki/extensions/SecurePollmaster+53 -315
mediawiki/extensions/WikiLovemaster+9 -30
mediawiki/extensions/RedirectManagermaster+3 -10
mediawiki/extensions/Gadgetsmaster+29 -80
mediawiki/extensions/SyntaxHighlight_GeSHimaster+8 -28
mediawiki/extensions/ImageSuggestionsmaster+24 -75
mediawiki/extensions/PageImagesmaster+30 -71
mediawiki/extensions/TocTreemaster+1 -4
mediawiki/extensions/CategoryTreemaster+21 -56
mediawiki/extensions/ReplaceTextmaster+28 -78
mediawiki/extensions/Kartographermaster+61 -107
mediawiki/extensions/WikiEditormaster+4 -13
mediawiki/extensions/CodeEditormaster+7 -10
mediawiki/extensions/CodeMirrormaster+19 -33
mediawiki/extensions/Citemaster+64 -131
mediawiki/extensions/CollapsibleVectormaster+2 -7
mediawiki/extensions/UploadWizardmaster+22 -51
mediawiki/coremaster+50 -50
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Trailing commas in (multi-line) function parameter lists

This should be done in PHPCS, but I suppose it's going to be the one with the most changes. We have long supported and encouraged trailing commas in arrays, and yet we don't even enforce those (and there's plenty of arrays not using them). Check if there's any upstream PHPCS sniff for it, if not I guess we can make one.

Like our current rules for arrays, I think this should not be enforced.

I'd like to try enforcing it only for the really obvious cases where it's annoying not to have them, like this and this. I filed T397111 with some more details and to have a place for some experiments. If that would still be annoying, then we can decline that and I guess I'll live without the commas.

I agree. Trailing commas are useful because they make future diffs less dirty. Creating huge dirty diffs to achieve this is completely counterproductive. I support encouraging them in new code, but not mass-updating existing code.

I don't think this is a problem when it's done in a separate commit, so that you can ignore these commits when git-blame'ing, and ideally together with a version bump to one of the linters, so that you don't have to review the change either. We do it all the time, e.g. in mediawiki/mediawiki-codesniffer 46.0, 41.0, 37.0 (that was a pretty huge one), at a glance roughly every other release comes with some automated code changes (although most are small, since they just enforce conventions we already follow).

Change #1161046 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Use str_starts_with

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

Named arguments require that all implementations/overrides of a method use the same name for the method arguments. psalm has a sniff for this and an automatic fixer, I don't know if phan/phpcs do? This should be turned on IMO as it is a prerequisite to using named arguments in our codebase. (I've already merged patches to fix Parsoid's argument naming, written by psalm.)

Named arguments require that all implementations/overrides of a method use the same name for the method arguments. psalm has a sniff for this and an automatic fixer, I don't know if phan/phpcs do? This should be turned on IMO as it is a prerequisite to using named arguments in our codebase. (I've already merged patches to fix Parsoid's argument naming, written by psalm.)

phpcs wouldn't really do it, it's mostly for syntax-level code conventions, and doing anything that needs to analyze the world like this is terribly unpleasant.

phan could do it, but it doesn't quite: it checks function/method calls (which are always a runtime error), but doesn't check method overrides (which only cause a runtime error at call time, but the overriding itself is fine according to PHP, just ill-advised – see Parameter name changes during inheritance). I could probably write a plugin or an upstream patch to warn about this too.

See also T397789: Define stable interface policy and coding conventions for named arguments (I just created it based on discussion elsewhere).

str_contains(), str_starts_with() and str_ends_with() functions

These have been available for a while via the Symfony polyfill package, I think?

str_contains(), str_starts_with() and str_ends_with() functions

These have been available for a while via the Symfony polyfill package, I think?

But not mass migrated yet. I have a patch to get some of them done.

Change #1161046 merged by jenkins-bot:

[mediawiki/core@master] Use str_starts_with

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

Change #1164636 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Use str_contains

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

Change #1164689 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] Make method parameter names consistent for PHP 8 named arguments

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

Named arguments require that all implementations/overrides of a method use the same name for the method arguments. psalm has a sniff for this and an automatic fixer, I don't know if phan/phpcs do? This should be turned on IMO as it is a prerequisite to using named arguments in our codebase. (I've already merged patches to fix Parsoid's argument naming, written by psalm.)

phpcs wouldn't really do it, it's mostly for syntax-level code conventions, and doing anything that needs to analyze the world like this is terribly unpleasant.

phan could do it, but it doesn't quite: it checks function/method calls (which are always a runtime error), but doesn't check method overrides (which only cause a runtime error at call time, but the overriding itself is fine according to PHP, just ill-advised – see Parameter name changes during inheritance). I could probably write a plugin or an upstream patch to warn about this too.

See also T397789: Define stable interface policy and coding conventions for named arguments (I just created it based on discussion elsewhere).

psalm does it: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1164689

Change #1165588 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/TocTree@master] Use PHP Constructor Property Promotion syntax

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

Change #1165974 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/CategoryTree@master] Use PHP Constructor Property Promotion syntax

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

Change #1166169 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/WikiEditor@master] Use PHP Constructor Property Promotion syntax

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

Change #1166766 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/RedirectManager@master] Use PHP Constructor Property Promotion syntax

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

Change #1166904 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Kartographer@master] Use PHP Constructor Property Promotion syntax

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

Change #1167832 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/UploadWizard@master] Use PHP Constructor Property Promotion syntax

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

Change #1167832 merged by jenkins-bot:

[mediawiki/extensions/UploadWizard@master] Use PHP Constructor Property Promotion syntax

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

Change #1167872 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/CodeEditor@master] Use PHP Constructor Property Promotion syntax

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

Change #1167904 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/CodeMirror@master] Use PHP Constructor Property Promotion syntax

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

Change #1167934 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/PageImages@master] Use PHP Constructor Property Promotion syntax

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

Change #1168116 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Cite@master] Use PHP Constructor Property Promotion syntax

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

Change #1168129 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/CollapsibleVector@master] Use PHP Constructor Property Promotion syntax

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

Change #1168129 merged by jenkins-bot:

[mediawiki/extensions/CollapsibleVector@master] Use PHP Constructor Property Promotion syntax

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

Change #1168116 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Use PHP Constructor Property Promotion syntax

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

Change #1167904 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] Use PHP Constructor Property Promotion syntax

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

Change #1167872 merged by jenkins-bot:

[mediawiki/extensions/CodeEditor@master] Use PHP Constructor Property Promotion syntax

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

Change #1166169 merged by jenkins-bot:

[mediawiki/extensions/WikiEditor@master] Use PHP Constructor Property Promotion syntax

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

Change #1166904 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Use PHP Constructor Property Promotion syntax

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

Change #1169247 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/ReplaceText@master] Use PHP Constructor Property Promotion syntax

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

Change #1169732 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/ImageSuggestions@master] Use PHP Constructor Property Promotion syntax

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

Change #1165974 merged by jenkins-bot:

[mediawiki/extensions/CategoryTree@master] Use PHP8 constructor property promotion syntax

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

Change #1169247 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@master] Use PHP8 constructor property promotion syntax

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

Change #1169732 merged by jenkins-bot:

[mediawiki/extensions/ImageSuggestions@master] Use PHP8 constructor property promotion syntax

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

Change #1165588 merged by jenkins-bot:

[mediawiki/extensions/TocTree@master] Use PHP8 constructor property promotion syntax

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

Change #1167934 merged by jenkins-bot:

[mediawiki/extensions/PageImages@master] Use PHP8 constructor property promotion syntax

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

Change #1171194 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/extensions/SecurePoll@master] Use constructor property promotion for service classes

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

Change #1172050 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Use PHP8 constructor property promotion syntax

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

Change #1172310 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Gadgets@master] Use PHP8 constructor property promotion syntax

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

Change #1172050 merged by jenkins-bot:

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Use PHP8 constructor property promotion syntax

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

Change #1172310 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Use PHP8 constructor property promotion syntax

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

Change #1172893 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FileImporter@master] Use PHP8 constructor property promotion syntax

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

Change #1166766 merged by jenkins-bot:

[mediawiki/extensions/RedirectManager@master] Use PHP8 Constructor Property Promotion syntax

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

Change #1174451 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/WikiLove@master] Use PHP8 constructor property promotion syntax

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

Change #1174451 merged by jenkins-bot:

[mediawiki/extensions/WikiLove@master] Use PHP8 constructor property promotion syntax

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

Change #1171194 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Use constructor property promotion for service classes

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

Change #1175051 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Phonos@master] Use PHP constructor property promotion syntax

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

Change #1175088 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/TimedMediaHandler@master] Use PHP8 constructor property promotion syntax

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

Change #1175051 merged by jenkins-bot:

[mediawiki/extensions/Phonos@master] Use PHP8 constructor property promotion syntax

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

Change #1175088 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Use PHP8 constructor property promotion syntax

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

Change #1175202 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Echo@master] Use PHP8 constructor property promotion syntax

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

Change #1176772 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Use constructor property promotion in all edit constraints

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

Change #1176772 merged by jenkins-bot:

[mediawiki/core@master] editpage: Use constructor property promotion in all edit constraints

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

Change #1164636 merged by jenkins-bot:

[mediawiki/core@master] Use str_contains

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

Change #1178097 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Babel@master] Use PHP8 constructor property promotion syntax

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

Change #1178097 merged by jenkins-bot:

[mediawiki/extensions/Babel@master] Use PHP8 constructor property promotion syntax

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

Change #1179109 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/CategoryTests@master] Use PHP8 constructor property promotion syntax

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

Change #1181264 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/Wikistories@master] Use PHP8 constructor property promotion syntax

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

Change #1175202 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Use PHP8 constructor property promotion syntax

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

Change #1179109 merged by jenkins-bot:

[mediawiki/extensions/CategoryTests@master] Use PHP8 constructor property promotion syntax

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

Change #1181264 merged by jenkins-bot:

[mediawiki/extensions/Wikistories@master] Use PHP8 constructor property promotion syntax

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

Change #1182961 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/core@master] includes/libs/Leximorph: Use constructor property promotion

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

Change #1183116 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/extensions/ReportIncident@master] Use PHP8 constructor property promotion syntax

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

Change #1183116 merged by jenkins-bot:

[mediawiki/extensions/ReportIncident@master] Use PHP8 constructor property promotion syntax

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

Change #1182961 merged by jenkins-bot:

[mediawiki/core@master] includes/libs/Leximorph: Use constructor property promotion

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

Change #1204975 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/ArticlePlaceholder@master] Use native type hints and PHP8 constructor property promotion

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

Change #1204975 merged by jenkins-bot:

[mediawiki/extensions/ArticlePlaceholder@master] Use native type hints and PHP8 constructor property promotion

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

I found this ticket from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/1230574 and I was wondering if there is a script to convert old code to use constructor property promotion. I would like to apply this to MathSearch.

Change #1172893 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Use PHP8 constructor property promotion syntax

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