Page MenuHomePhabricator

Should mediawiki/mediawiki-phan-config ship a _target php version to support running with newer php on the developer side?
Closed, ResolvedPublic

Description

When running phan on core or extensions with a php7.4 it can shows some suggestion for the code which are not usable, because CI is running with 7.2 as current required version.

Example:

PhanPluginDuplicateExpressionAssignmentOperation
Can simplify this assignment to $attributes['class'] ??= '' (requires php version 7.4 or newer)

Should the config have '_target_php_version' or 'minimum__target_php_version'?

// The PHP version that the codebase will be checked for compatibility against.
// For best results, the PHP binary used to run Phan should have the same PHP version.
// (Phan relies on Reflection for some types, param counts,
// and checks for undefined classes/methods/functions)
//
// Supported values: `'5.6'`, `'7.0'`, `'7.1'`, `'7.2'`, `'7.3'`, `'7.4'`, `null`.
// If this is set to `null`,
// then Phan assumes the PHP version which is closest to the minor version
// of the php executable used to execute Phan.
//
// Note that the **only** effect of choosing `'5.6'` is to infer that functions removed in php 7.0 exist.
// (See `backward_compatibility_checks` for additional options)
'_target_php_version' => null,

// The PHP version that will be used for feature/syntax compatibility warnings.
//
// Supported values: `'5.6'`, `'7.0'`, `'7.1'`, `'7.2'`, `'7.3'`, `'7.4'`, `null`.
// If this is set to `null`, Phan will first attempt to infer the value from
// the project's composer.json's `{"require": {"php": "version range"}}` if possible.
// If that could not be determined, then Phan assumes `_target_php_version`.
'minimum__target_php_version' => null,

Related Objects

StatusSubtypeAssignedTask
ResolvedNone
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
Resolved toan
ResolvedLucas_Werkmeister_WMDE
ResolvedJoe
ResolvedJdforrester-WMF
ResolvedLadsgroup
InvalidNone
ResolvedReedy
OpenNone
Resolved tstarling
ResolvedJdforrester-WMF
ResolvedPRODUCTION ERRORLegoktm
Resolved tstarling
ResolvedJoe
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
ResolvedDaimona
ResolvedDaimona

Event Timeline

I can confirm that issues like

PhanPluginDuplicateExpressionAssignmentOperation Can simplify this assignment to $attributes['class'] ??= '' (requires php version 7.4 or newer)

Are all caused by minimum__target_php_version being set to 7.4 (as inferred from the PHP executable). So I see it as:

  • _target_php_version is used to give forward-compatibility notices (I might think of warning if a function is named "match", for PHP 8 compatibility, if _target_php_version is '8.0') [1]
  • minimum_... is used to determine which features can be used

So for our case, we probably want to set minimum_... to 7.2, as that's what most repos have as a minimum requirement, and _target_... to 8.0, as that's what we're aming to.

The problem is for repos with a minimum requirement higher than 7.2. They'd need to manually override the config value, because the composer.json value is ignored in presence of a config setting (unless this can be changed upstream with an additional config param). Note, even in the worst case, these repos would just stop getting some suggestions like the one about '??=', but nothing more serious in theory.

[1] - Right now, there seems to be just an issue being emitted if it's 8.0, perhaps more compatibility notices will be added later.

Ah, drat, in my head we'd solved this issue.

So for our case, we probably want to set [minimum__target_php_version] to 7.2, as that's what most repos have as a minimum requirement, and [_target_php_version] to 8.0, as that's what we're aming to.

The problem is for repos with a minimum requirement higher than 7.2. They'd need to manually override the config value, because the composer.json value is ignored in presence of a config setting (unless this can be changed upstream with an additional config param). Note, even in the worst case, these repos would just stop getting some suggestions like the one about '??=', but nothing more serious in theory.

[1] - Right now, there seems to be just an issue being emitted if it's 8.0, perhaps more compatibility notices will be added later.

This sounds good to me (or even 8.1?). Let's do it?

Change 735368 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan@master] Set minimum and _target PHP version

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

This sounds good to me (or even 8.1?). Let's do it?

Yeah, done. The settings can still be overridden if a repo has a different minimum/_target PHP version.

Change 735368 merged by jenkins-bot:

[mediawiki/tools/phan@master] Set minimum and _target PHP version

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

  NODES
Note 4
Project 3