Page MenuHomePhabricator

Encourage type hints for function parameters and return after moving MediaWiki to PHP 7
Open, Stalled, NormalPublic

Description

This is blocked on: T172165: Require either PHP 7.0+ or HHVM in MW 1.31


Certain versions of HHVM have revealed issues with data types in MediaWiki (e.g. T163646, T140878, T126871, T177134, T140864). Although PHP 7.0 is likely to hide those issues from view again, as does PHP 5.6, I think using type hints in function declarations (PHP 7 supports type hints for scalar types) will make code more robust, will force repayment of technical debt (rather than using hacks like https://gerrit.wikimedia.org/r/381617, https://gerrit.wikimedia.org/r/302430 or https://gerrit.wikimedia.org/r/381616) and in the future, possibly, will lead to gains in performance (as does strict typing in HHVM when using repo authoritative mode).

I therefore suggest a goal more ambitious than merely dropping support for PHP 5.* (task T172165): to strongly encourage (preferably, using some technical means) strict typing of function parametres and results in MediaWiki code, using <?php declare(strict_types=1); wherever possible.

This task also implies raising PHP minimal version to 7.1 in the near future; as only 7.1 allows nullable parametres.

This requirement can be enforced by automated testing, as far as I understand.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 13 2017, 4:41 AM

In theory most of the time, phan will already enforce type hints from the comment block, so I'm not sure it would change much in practise in terms of code reliability (Unless we also got better at declaring more specific types).

Adding type hints to function method signatures can be a backwards compat break for extension, so that may be an issue depending on context.

Aklapper renamed this task from Make type hints for function parametres and return compulsory after moving MediaWiki to PHP 7 to Make type hints for function parameters and return compulsory after moving MediaWiki to PHP 7.Oct 13 2017, 11:07 AM
Anomie added a subscriber: Anomie.Oct 13 2017, 1:57 PM

Is this proposal only wanting to add scalar type declarations, or is it also wanting declare(strict_types=1); added to every file to disable PHP's standard typecasting when making function calls (doc ref)?

I note that you can't strictly typehint parameters that aren't restricted to only one type, except in the limited case where it's a specific type or null and you don't mind specifying null as the default. For example, a parameter that can be "string|false" can't be declared as such for PHP to enforce.

declare(strict_types=1); seems more attractive but there could be some problems with hooks. Unless they are restricted to callable, perhaps.

Personally, I think trying to disable automatic casting from int to string or bool and the like would turn out to be much more trouble than it's worth.

MaxSem moved this task from Unsorted to PHP 7 on the [DO NOT USE] NewPHP board.Oct 17 2017, 5:22 AM
alex-mashin updated the task description. (Show Details)Oct 17 2017, 6:01 PM
demon added a subscriber: demon.Feb 24 2018, 12:11 AM

I note that you can't strictly typehint parameters that aren't restricted to only one type, except in the limited case where it's a specific type or null and you don't mind specifying null as the default.

Even then, you're still stuck doing "hacks" that the OP seems to have an issue with.

Personally, I think trying to disable automatic casting from int to string or bool and the like would turn out to be much more trouble than it's worth.

This.

Should we encourage them? Probably. Should code without them be suspect? Eh, possibly? But I think with a dynamically typed language, "compulsory" is never going to happen. Your second example, gerrit 302430, I would hardly call a hack...but rather proper handling of an optional parameter. Your third example is similar.

I propose declining this.

alex-mashin added a comment.EditedFeb 24 2018, 5:58 AM

PHP 8 is expected to use JIT, which means that performance is lilkely to be boosted by strict typing.

Although PHP 8 is still in the indefinite future, MediaWiki codebase is huge and will take years to fully make use of PHP type hinting.

Perhaps "compulsory" was too strong a word, but "encourage" is not; I edited the task accordingly.

alex-mashin renamed this task from Make type hints for function parameters and return compulsory after moving MediaWiki to PHP 7 to Rncougare type hints for function parameters and return after moving MediaWiki to PHP 7.Feb 24 2018, 6:05 AM
alex-mashin updated the task description. (Show Details)
EBernhardson renamed this task from Rncougare type hints for function parameters and return after moving MediaWiki to PHP 7 to Encourage type hints for function parameters and return after moving MediaWiki to PHP 7.Feb 27 2018, 11:27 PM
Krinkle changed the task status from Open to Stalled.
Krinkle triaged this task as Normal priority.
Krinkle updated the task description. (Show Details)

I would appreciate a sniff checking if type hints from the documentation are actually set in code as well, if possible. A sniff like that would take a PHP version as parameter, as possibilities for type hinting have grown in multiple PHP versions.

I would appreciate a sniff checking if type hints from the documentation are actually set in code as well, if possible. A sniff like that would take a PHP version as parameter, as possibilities for type hinting have grown in multiple PHP versions.

I'm pretty sure our phan setup for CI can do this, although it may need to be bumped to a newer version and configured to enable the check.

Tgr added a subscriber: Tgr.Feb 14 2019, 1:33 AM

I would support making it compulsory for new code. Old code has all kinds of compatibility / deprecation considerations of course; it would be nice to convert it over time but a lot of work for limited benefit. (Where public interfaces are not affected, it should probably be done sooner.)

Of course there are plenty of situations where declaring the return type is not possible (ClassOne|ClassTwo, mixed, also void, iterable and nullables until PHP 7.1) so "compulsory" would mostly mean adding it to the coding styles and enforcing during code review.

I'm not sure I understand the idea presented in this tickets description. Is the suggestion to somehow force people to type hint all code? How would we set this up? Who would be in charge to update the code? Are we in a position to assign budget on a task as massive as updating multiple tens of thousand function headers in a thousand codebases? How do we communicate this new requirement to all the dev teams? How can we check if code conforms to this new standard? I mean, it's not like PHP 7 suddenly forbids to have function parameters that can be, for example int|string, string[]|string, or even mixed, just to name the most prominent examples that don't involve null.

Don't get me wrong: I freaking love the new type hinting capabilities we will get with PHP 7.0, 7.1 (adds iterable and nullables), and 7.2 (adds object). I will heavily make use of it the moment I can, and encourage everybody to do the same.

But I think it's a bad idea to somehow "force" this on people. Sometimes it's just a good idea to make a function, let's say, accept a @param Title|string $title.

Furthermore, I don't think strict_types does what you might think. According to http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration.strict PHP still happily converts values when, for example, a method expecting an integer is called with a string. strict_types only blocks this if the calling code is marked to be strict. The ticket here is about the code being called. That's another file, and often another codebase.

All this said I support closing this ticket, possibly as invalid. Not because I don't think the PHP 7 type hints are a good idea (they are!), but because there is nothing we can do to "encourage" people other then talking to them,