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.

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

epriestley added a revision: Restricted Differential Revision.Feb 2 2017, 2:03 PM

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

EDIT: sorry, wrong tab :/

epriestley added a revision: Restricted Differential Revision.Oct 23 2017, 5:14 PM
epriestley added a revision: Restricted Differential Revision.Oct 23 2017, 5:17 PM
epriestley added a commit: Restricted Diffusion Commit.Oct 23 2017, 5:33 PM
epriestley closed this task as Resolved by committing Restricted Diffusion Commit.Oct 23 2017, 5:54 PM
epriestley added a commit: Restricted Diffusion Commit.
epriestley added a commit: Restricted Diffusion Commit.
epriestley added a revision: Restricted Differential Revision.Nov 28 2017, 2:05 PM
epriestley added a commit: Restricted Diffusion Commit.Nov 28 2017, 6:54 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.

epriestley added a revision: Restricted Differential Revision.Aug 24 2018, 5:47 PM
epriestley added a commit: Restricted Diffusion Commit.Aug 24 2018, 5:58 PM