Problem/Motivation

\Drupal\Tests\user\Traits\UserCreationTrait::createUser returns User|false. If you use phpstan level 7 or higher that means you need to always check if it was successful or not. It would be better to simply throw an exception instead.

In fact, it can never even return false. The first place it should return false is if it failed to create a role, but there is an assertion in the createRole method that will fail before we can return false. The second place is if $valid_user is FALSE then assertTrue will throw an exception and we'll never reach the return.

Steps to reproduce

Proposed resolution

Remove |false from the signature of the following:

  • UserCreationTrait::createUser
  • UserCreationTrait::createAdminRole
  • UserCreationTrait::createRole
  • WorkspacesMediaLibraryIntegrationTest::drupalCreateUser

Remove any unreachable returns

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3560202

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Adding the BC layer seems like a lot of busy work for not much gain, as this is tests only and in 99% of cases (maybe 100% in contrib/custom tests?) the creation will be expected to succeed then perhaps we just start throwing the exception?

mstrelan’s picture

mstrelan commented

Sounds good to me.

ritarshi_chakraborty’s picture

ritarshi_chakraborty commented

Working on it.

ritarshi_chakraborty opened merge request !13982

ritarshi_chakraborty’s picture

ritarshi_chakraborty commented

Hey @longwave, I implemented the changes you requested but tests are failing. Shall we go with the previous logic?

mstrelan’s picture

mstrelan commented

createRole returns false not null, assertNotNull will pass if it returns false

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Stop returning FALSE from UserCreationTrait::createUser » UserCreationTrait::create* methods can never return false
Category: Feature request » Bug report
Status: Active » Needs work

createRole() also will never fail, because this assertion will stop the test if it does:

    $result = $role->save();

    $this->assertSame(SAVED_NEW, $result, "Created role ID {$role->id()} with name {$role->label()}.");

Reclassifying this as a bug, as far as I can see it's never worked since the migration from SimpleTest to PHPUnit.

I wonder if there is a PHPStan extension that can catch dead code following assertions?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs issue summary update

The IS is out of date compared to the actual problem.

mstrelan’s picture

mstrelan commented
Category: Bug report » Feature request
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Updated the IS. Left one comment on the MR.

mstrelan’s picture

mstrelan commented
Category: Feature request » Bug report

Didn't mean to change the category

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Thanks for the review, fixed that type in the docs.

mstrelan’s picture

mstrelan commented

The changes look good to me.

I suspect there are a few projects that are handling this already that may be affected by this, but I haven't thought through exactly how this works with method signatures in traits, base classes etc. A couple links below:

https://git.drupalcode.org/project/autocomplete_mixed_matching/-/blob/1....

https://git.drupalcode.org/project/content_entity_base/-/blob/8.x-1.x/te...

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems pretty easy enough. I did a search in core to see if there were other instances and all seem good.

I looked at the 2 examples in #14 and don't believe this change will break this? autcomplete_mixed_matching even has an assert that it's not false, so guess that would be dead code for them.

  • catch committed 261f34a6 on 11.3.x 
    fix: #3560202 UserCreationTrait::create* methods can never return false...

  • catch committed 58f87572 on 11.x 
    fix: #3560202 UserCreationTrait::create* methods can never return false...

  • catch committed 14487099 on main 
    fix: #3560202 UserCreationTrait::create* methods can never return false...

  • catch committed e0035420 on 11.3.x 
    Revert "fix: #3560202 UserCreationTrait::create* methods can never...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !13982

Status: Fixed » Closed (fixed)

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