Page MenuHomePhabricator

In PHP7, include-time warnings from source files are ignored
Closed, ResolvedPublic

Description

If you write code like this:

class A {
  public function m(X $x) {}
}

class B extends A {
  public function m() {}
}

...it generates an include time error because the declarations of method m() disagree, like this:

[Thu Feb 02 05:53:22.399241 2017] [php7:notice] [pid 38535] [client 127.0.0.1:53398] [2017-02-02 07:53:22] ERROR 2: Declaration of PhabricatorHomeProfileMenuItem::newPageContent() should be compatible with PhabricatorProfileMenuItem::newPageContent(PhabricatorProfileMenuItemConfiguration $config) at [/Users/epriestley/dev/core/lib/phabricator/src/applications/home/menuitem/PhabricatorHomeProfileMenuItem.php:4]

Prior to PHP7, this was a E_STRICT error. After PHP7, E_STRICT has been deprecated and this is an E_WARNING instead.

Currently, in PhutilBootloader::executeInclude(), we silence E_WARNING warnings because they only indicated that a file did not exist or failed to load prior to PHP7, and we test for that by checking the return value of include_once.

// Suppress warning spew if the file does not exist; we'll throw an
// exception instead. We still emit error text in the case of syntax errors.
$old = error_reporting(E_ALL & ~E_WARNING);
$okay = include_once $path;
error_reporting($old);

However, under PHP7, this now causes some warnings (including "Declaration .. should be compatible") to be silenced.

This is somewhat complicated to navigate because ideally we want to continue handling "file missing" warnings gracefully.

Event Timeline

kugel- added a subscriber: kugel-.EditedFeb 3 2017, 9:55 AM

@epriestley can you have a look at this, please?

EDIT: sorry, wrong tab :/

johnclyde added a subscriber: johnclyde.EditedMay 12 2018, 9:55 PM

I added a linter and I accidentally included this line which references a different already-existing linter

final class ArcanistChmodLinter extends ArcanistLinter {

This impact of this is that arc lint silently fails, and I traced it down to PhutilBootloader::executeInclude() which still has a reference to T12190 even though this task is now closed.