backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

ThemeFunctionsTestCase::assertThemeOutput(): Deprecated: Optional parameter $variables declared... #6150

Open klonos opened 1 year ago

klonos commented 1 year ago

I just want to raise this issue quickly for now, so that there's a record for it. I won't get into the details of how I have gotten to this point*, but I got this warning while troubleshooting the test I have added in the PR for #6148 :

Deprecated: Optional parameter $variables declared before required parameter $expected is implicitly treated as a required parameter in /app/docroot/core/modules/simpletest/tests/theme.test on line 411

(*tests have been failing with fatal errors, but without any meaningful message to help troubleshoot further)


Side-notes

Noting that this is the function declaration for ThemeFunctionsTestCase::assertThemeOutput():

protected function assertThemeOutput($callback, array $variables = array(), $expected, $message = '', $group = 'Other') { ... }

We also have the very similar BackdropWebTestCase::assertThemeOutput(), which has this declaration:

protected function assertThemeOutput($callback, array $variables, $expected, $message = '', $group = 'Other') { ... }

Also noting that ThemeFunctionsTestCase::assertThemeOutput() is using t() for the assertion message (it shouldn't), whereas BackdropWebTestCase::assertThemeOutput() is correctly using format_string().

Not sure why we have these two practically identical methods 🤷🏼 (?)

avpaderno commented 1 year ago

It is sufficient to change the method declaration to protected function assertThemeOutput($callback, array $variables, $expected, $message = '', $group = 'Other'). It is not possible for the methods/functions calling that method to omit the value for $variables and expect PHP uses its default value; in fact, they always provide a value for $variables.

$variables = array();
$expected = '';
$this->assertThemeOutput('links', $variables, $expected, 'Empty %callback generates no output.');
$variables = array();
$variables['heading'] = 'Some title';
$expected = '';
$this->assertThemeOutput('links', $variables, $expected, 'Empty %callback with heading generates no output.');
avpaderno commented 1 year ago

Since BackdropWebTestCase is the parent class of ThemeFunctionsTestCase, in this case the solution is simply removing ThemeFunctionsTestCase::assertThemeOutput().

avpaderno commented 11 months ago

There are conflicts between the current 1.x branch and the PR I created. I will close my PR and create another one later.