Problem/Motivation

Adding UserCreationTrait to a test is currently very frustrating as the phpstan baseline has to change - see https://www.drupal.org/pift-ci-job/2516459

The same goes for AssertMailTrait.

Proposed resolution

Fix the traits and baseline

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
64.7 KB
mondrake’s picture

Version: 10.0.x-dev » 10.1.x-dev
Status: Needs review » Reviewed & tested by the community

Good for 10.1.x, will need porting to 10.0

mondrake’s picture

Issue tags: +PHPStan-1
alexpott’s picture

Title: Fix PHPStan errors from UserCreationTrait » Fix PHPStan errors from UserCreationTrait and AssertMailTrait
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Related issues: +#3319582: Fix calls to methods with too many parameters passed in
FileSize
76.77 KB
77.46 KB

Widening the scope slightly to incorporate the other trait that causes issues like this. Crediting @mondrake since this is similar to the work in #3319582: Fix calls to methods with too many parameters passed in

And have patches for both 10.0.x and 10.1.x

alexpott’s picture

  1. +++ b/core/lib/Drupal/Core/Test/AssertMailTrait.php
    @@ -82,13 +82,8 @@ protected function assertMail($name, $value = '', $message = '', $group = 'Email
    -   * @param string $group
    -   *   (optional) The group this message is in, which is displayed in a column
    -   *   in test output. Use 'Debug' to indicate this is debugging output. Do not
    -   *   translate this string. Defaults to 'Other'; most tests do not override
    -   *   this default.
        */
    -  protected function assertMailString($field_name, $string, $email_depth, $message = '', $group = 'Other') {
    +  protected function assertMailString($field_name, $string, $email_depth, $message = '') {
    

    Now that we have PHPStan level 1 any calls to assertMailString that have 4 arguments will produce an error - so we don't need to trigger a deprecation here.

  2. +++ b/core/lib/Drupal/Core/Test/AssertMailTrait.php
    @@ -122,20 +117,15 @@ protected function assertMailString($field_name, $string, $email_depth, $message
    -   * @param string $group
    -   *   (optional) The group this message is in, which is displayed in a column
    -   *   in test output. Use 'Debug' to indicate this is debugging output. Do not
    -   *   translate this string. Defaults to 'Other'; most tests do not override
    -   *   this default.
        */
    -  protected function assertMailPattern($field_name, $regex, $message = '', $group = 'Other') {
    +  protected function assertMailPattern($field_name, $regex, $message = '') {
    

    As above.

alexpott’s picture

This patch shrinks core/phpstan-baseline.neon from 199K to 150K... not bad.

mallezie’s picture

Status: Needs review » Reviewed & tested by the community

Both patches look good to me. (Awaiting tests offcourse).
Both code and comments updated, Baseline removals are all in scope, nothing unexpected here. So RTBC.

Changes are small (and same on both patches). And they clean up a nice chunk in the baseline.

mallezie’s picture

Status: Reviewed & tested by the community » Needs work

Don't we need the deprecation for contrib / custom testing using this trait?

mallezie’s picture

Status: Needs work » Reviewed & tested by the community

Since PHP allows you to pass in as many extra params as you like this will not break any other contrib / custom code using this parameter. (Thanks @alexpott for explaining).
Solves my remark.

  • catch committed 19ee0de on 10.0.x
    Issue #3319683 by alexpott, mallezie, mondrake: Fix PHPStan errors from...
  • catch committed 0dc1693 on 10.1.x
    Issue #3319683 by alexpott, mallezie, mondrake: Fix PHPStan errors from...
catch’s picture

Version: 10.1.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed the respective patches to 10.1.x and 10.0.x, thanks!

mondrake’s picture

Status: Fixed » Closed (fixed)

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