Fatal error on PHP 7.2: Method signature compatibility strictly enforced

Wow, that's an odd breakage.

I tested locally with RC6 and got this:

/usr/local/Cellar/php72/7.2.0RC6_9/bin/php -r 'class A { public function f($x = NULL) {} } class B extends A { public function f() {} }'
PHP Warning:  Declaration of B::f() should be compatible with A::f($x = NULL) in Command line code on line 1

Warning: Declaration of B::f() should be compatible with A::f($x = NULL) in Command line code on line 1

And with 7.1:

/usr/local/Cellar/php71/7.1.11_22/bin/php -r 'class A { public function f($x = NULL) {} } class B extends A { public function f() {} }'
PHP Warning:  Declaration of B::f() should be compatible with A::f($x = NULL) in Command line code on line 1

Warning: Declaration of B::f() should be compatible with A::f($x = NULL) in Command line code on line 1

So this is a warning for me locally, rather than a fatal error, but on both 7.1 and 7.2.

(Edit: I didn't add a type hint on the arg here, but doing so doesn't change the result.)

$ /usr/local/Cellar/php72/7.2.0RC6_9/bin/php -r 'class A { public function f(A $x = NULL) {} } class B extends A { public function f() {} }'
PHP Warning:  Declaration of B::f() should be compatible with A::f(?A $x = NULL) in Command line code on line 1

Warning: Declaration of B::f() should be compatible with A::f(?A $x = NULL) in Command line code on line 1

However, I can confirm the fatal error in Drupal on PHP72, and no warning in PHP71, unlike with the sample code above.

The difference is the interface.

This is a warning on PHP71 and PHP72:

class A {
  public function f(A $x = NULL) {}
}

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

This is fine on PHP71 and fatal on PHP72:

interface A {
  public function f();
}

class B implements A {
  public function f(A $x = NULL) {}
}

class C extends B {
  public function f() {}
}

I'm not sure, but I get the feeling this is a PHP bug - even if it's intended, this introduces a BC-breaking fatal error rather than a deprecation.

I went ahead and wrote a PHP bug report (https://bugs.php.net/bug.php?id=75590), but I'm not sure the result won't be "this code was always incorrect, and should never have worked in the first place". :P

By the way, I think I have an idea why PHP wants to allow extending the argument signature with optional arguments, but not reducing it. Each change on their own is transitive in fulfilling all superclasses contracts, but both together allow us to violate that transitivity, because subclasses can remove and then re-add differently-typed arguments to the signature.

If A::f(X $x = NULL) is extended by B::f() which is extended by C::f(Y $x = NULL), then C::f(?Y) is incompatible with A::f(?X)) even though each method is compatible with its immediate parent. To safely allow this, PHP would have to search the entire inheritance chain for type mismatches, and developers would have to be told: "Yes, you can extend B and override B::f, and you can add an optional argument to it, but it has to be of type X because some superclass of B already used that."

Edit:

This is obviously not ideal, but I suspect our only good option (even if PHP7.2 removes the fatal error) is to go through core and ensure strict compatibility in all method declarations, even those that are silent in PHP7.1.

In 8.5.x, IntelliJ currently recognizes 35 violations, which fall into these categories:

- \Drupal\Core\Database\Statement::fetchAll(?, ?, ?) double-implements \PDOStatement::fetchAll(?, ?, ?Array) and \Drupal\Core\Database\StatementInterface::fetchAll(?, ?, ?). Fixed by adding array hint both in the interface and the class.
- 22 subclasses of \Drupal\views\Tests\ViewTestBase override ::setup(), to which ViewTestBase adds an optional flag $import_test_views. Quick fix: Add the argument to all subclasses, but the flag should really be refactored to a separate importTestViews() method.
- \Drupal\forum\Form\Overview::buildForm(array, FormStateInterface) overrides Drupal\taxonomy\Form\OverviewTerms::buildForm(array, FormStateInterface, ?VocabularyInterface). Probably no way around taking the VocabularyInterface argument there too, and then simply overwriting it with the configured forum vocabulary.
- 9 classes extend \Drupal\Core\Menu\LocalTaskDefault or LocalActionDefault or ContextualLinkDefault, overriding ::getTitle(?Request) with ::getTitle. (Includes the original crash.) Fixed by adding the missing argument.
- 2 subclasses extend \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::count(?) which implements \Countable::count() and adds a $refresh argument. Same as ViewTestBase: This shouldn't be a flag, especially because this method overloads PHP's count(). Just use a separate ::refresh() method.
- \Drupal\layout_builder\Form\UpdateBlockForm::buildForm(array, FormStateInterface, EntityInterface, ?, ?, ?) overrides \Drupal\layout_builder\Form\ConfigureBlockFormBase::buildForm(array, FormStateInterface, EntityInterface, ?, ?, ?, ?array). The extra $configuration argument should be added to the subclass signature.

This patch fixes all the inspection errors (based on 8.5.x) and should therefore fix all the fatals, without doing any additional refactoring.

Actual refactoring is probably out of scope, but the following are a bit iffy:

- The ::setup($import_test_views = TRUE). The only case where it's set to FALSE when a test needs to set up other stuff (content types, taxonomy) before creating the views. I suspect that better ways to do this is to split the setup function:

-- Either move the views import to an extra createTestViews() method and call that from the child's setup after doing the pre-views setup.
-- Or move the pre-views-import steps to an extra preSetup() method and call that from the parent's setup before creating views.

- The ::count($refresh = FALSE). Use a ::refreshCount() or something.

- The LocalActionDefault, LocalTaskDefault and ContextualLinkDefault ::getTitle(Request $request). The argument was added by #2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings, but I don't see any class in core which actually uses it (it was briefly used while the patch evolved, but seemingly not in the final product), and the respective interfaces don't define it at all. Are we sure this isn't detritus?

Probably jumped the gun on the PDO stuff. It wasn't actually a fatal error before, and it seems that (unlike my local environment) the CI is using a version of PDOStatement where the $constructor_argument isn't type-hinted. Reverted.

The FALSE in FieldApiDataTest was never doing anything lol. PHP 7.2 is certainly going to make our code better and more consistent!

This needs to be fixed in 8.5.x first...

This sounds critical.

Yeah, especially now that 7.2.0 is out. Aside from the comment moderation queue, enabling config_translation seemingly breaks almost all config forms because of the local-task thing...

As per #14 we need this in 8.5.x first - doesn't apply to 8.4.x so that's why testbot set back to needs work...

@catch, @effulgentsia, @larowlan, @xjm and I discussed this issue today. We agreed it should be considered a duplicate of #2923015: [PHP 7.2] Incompatible method declarations.

To fix the issue in drupal 8.4.x, edit the file core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php

on line 10 add:

use Symfony\Component\HttpFoundation\Request;

near line 55 replace :

public function getTitle() {

with this :

public function getTitle(Request $request = NULL) {

#19 fixed the issue for me.