Problem/Motivation

Discovered via PHPStan in #3178534: Start running PHPStan on Drupal core (level 0), there is invalid PHP syntax in \Drupal\Tests\node\Functional\AssertButtonsTrait.

$ php -l ./core/modules/node/tests/src/Functional/AssertButtonsTrait.php
PHP Parse error:  syntax error, unexpected '+', expecting :: (T_PAAMAYIM_NEKUDOTAYIM) in ./core/modules/node/tests/src/Functional/AssertButtonsTrait.php on line 30

The line in question is

$this->assertSession()->elementTextEquals('xpath', "(//div[@class='dropbutton-wrapper']//input[@type='submit'])[{$i + 1}]", $buttons[$i]);

Only variables, array elements or object properties can be accessed using the {} syntax inside strings; other expressions are not allowed.

This was last changed in #3203476: Convert assertions involving use of xpath on divs to WebAssert, and although we don't use this trait in core (two uses in contrib), I don't know how this slipped through as I thought we had a PHP lint step in DrupalCI?

Steps to reproduce

Proposed resolution

Fix the syntax.
Add a basic test that uses this trait.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
StatusFileSize
new1.25 KB

This trait doesn't seem worth maintaining in core.

It was only ever used in NodeFormButtonsTest. It was added in #2511584: Move NodeFormButtonsTest::assertButtons() to a trait and then the original test was removed from core in #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button

I propose fixing the syntax but deprecating it with no replacement.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
+++ b/core/modules/node/tests/src/Functional/AssertButtonsTrait.php
@@ -2,6 +2,8 @@
+@trigger_error(__NAMESPACE__ . '\AssertButtonsTrait is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/xxx', E_USER_DEPRECATED);

Needs a CR and its number reflected in the deprecation message. Other than that, this looks good.

longwave’s picture

longwave’s picture

Status: Needs work » Needs review
StatusFileSize
new1.25 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Slightly edited the CR. Thanks!

grimreaper’s picture

+1 for this, big thanks!

  • catch committed ee6a1a7 on 9.3.x
    Issue #3213734 by longwave, mondrake: AssertButtonsTrait has invalid PHP...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed ee6a1a7 and pushed to 9.3.x. Thanks!

mondrake’s picture

Status: Fixed » Patch (to be ported)

I would suggest to backport the fix to the PHP syntax (not the deprecation) to 9.2.x to avoid static analysis complaining

mondrake’s picture

Issue tags: +Novice
longwave’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new959 bytes
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed 9da6062 on 9.2.x
    Issue #3213734 by longwave, mondrake: AssertButtonsTrait has invalid PHP...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9da6062 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

Published change record