resolve differing createUser() test methods [#3324384]

Problem/Motivation

This is a follow-on from #3224276: Move useful helper methods for working with entities from EntityKernelTestBase to a trait, in the interest of keeping that issue simple.

There is a confusing mishmash of test methods called createUser():

- Drupal\Tests\user\Traits\UserCreationTrait has createUser() with 4 params, which we'll refer to as createUser4: createUser(array $permissions = [], $name = NULL, $admin = FALSE, array $values = [])
- EntityKernelTestBase has createUser() with 2 params, which we'll refer to as createUser2: createUser($values = [], $permissions = [])
- EntityKernelTestBase imports UserCreationTrait, aliasing createUser as drupalCreateUser
- some child classes of KernelTestBase import UserCreationTrait, and some of them do the same aliasing, and some don't

Proposed resolution

Figure out how to end up in a situation where KernelTestBase imports UserCreationTrait, including createUser() without aliasing.

This is going to be difficult because the methods are incompatible. See https://www.drupal.org/project/drupal/issues/3224276#comment-14681959 for detatils.

Remaining tasks

Figure out the minimum of breakage that can happen.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3324384

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3324384-resolve-differing-createuser-methods changes, plain diff MR !3046
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

29 November 2022 at 14:55

joachim created an issue. See original summary.

Comment #2

longwave

he/him

English

commented 29 November 2022 at 15:07

This is also confusing PHPStan, see #3319582-19: Fix calls to methods with too many parameters passed in

Comment #3

longwave

he/him

English

commented 29 November 2022 at 15:09

Wow, this is confusing. If only we hadn't done this!

  protected function createUser($values = [], $permissions = []) {
    return $this->drupalCreateUser($permissions ?: [], NULL, FALSE, $values ?: []);
  }

Comment #4

joachim commented 29 November 2022 at 15:32

Issue summary:

View changes

Is there a way could remove createUser2, and have createUser4 sniff its passed parameter values to figure out which one the caller means, and trigger a deprecation if it detected that the caller passed in createUser2 parameters?

I'm not sure whether that's possible, based on what the caller might be assuming the default values of parameters do.

Comment #5

joachim commented 29 November 2022 at 15:35

Ok, we'd basically have to detect whether param1 is an array of entity value or an array of permissions:

  protected function createUser($values = [], $permissions = []) {

  protected function createUser(array $permissions = [], $name = NULL, $admin = FALSE, array $values = []) {

We can PROBABLY assume that $permissions is a numeric array? If so, then this would maybe work:

function createUser(array $permissions = [], $name = NULL, $admin = FALSE, array $values = []) {
  // if $permissions has string keys -> it's actually $values!

  // if $name is an array, it's actually $permissions!

  // now carry on as before!

Comment #6

joachim commented 29 November 2022 at 16:11

Status:

Active

» Needs review

Let's see if this works...

Comment #7

29 November 2022 at 16:13

joachim opened merge request !3046

Comment #8

joachim commented 30 November 2022 at 10:40

Status:	Needs review	» Needs work
Issue tags:		+Novice

Tagging as novice in case someone can get to this before I do :)

The MR needs all the calls to createUser() that are failing in the test to be changed to the new parameter order.

Comment #9

elber

Portuguese, Brazil

Brazil

commented 30 November 2022 at 12:42

ok, I wil try to work on it

Comment #10

elber

Portuguese, Brazil

Brazil

commented 1 December 2022 at 17:32

Status:

Needs work

» Needs review

Comment #11

mondrake

🇮🇹

commented 1 December 2022 at 17:39

Issue tags:

+PHPStan-1

Comment #12

joachim commented 2 December 2022 at 10:07

What's the purpose of these lines:

> use phpDocumentor\Reflection\Types\Null_;

Comment #13

elber

Portuguese, Brazil

Brazil

commented 2 December 2022 at 11:03

ok sorry it was a mistake. I fixed it in the next commit

Comment #14

3 December 2022 at 10:55

Spokje made their first commit to this issue’s fork.

Comment #15

spokje

Dutch

commented 3 December 2022 at 12:53

Fixed test failures.

Comment #16

elber

Portuguese, Brazil

Brazil

commented 5 December 2022 at 10:27

Status:

Needs review

» Reviewed & tested by the community

I revised the last commit. Tests are passing, coding standards are ok, scope of the issue was fixed and MR is applied without issues. Moving to RTBC

Comment #17

znerol commented 22 December 2022 at 18:05

Status:

Reviewed & tested by the community

» Needs work

Comment #18

spokje

Dutch

commented 29 December 2022 at 14:21

Status:

Needs work

» Needs review

(Tried to) Address(ed) all threads brought up by @znerol.

Comment #19

znerol commented 2 January 2023 at 11:01

Status:

Needs review

» Needs work

Comment #20

spokje

Dutch

commented 3 January 2023 at 11:39

Status:

Needs work

» Needs review

Comment #21

longwave

he/him

English

commented 3 January 2023 at 16:05

Status:

Needs review

» Needs work

Three more minor comments on the MR.

Is this a good place to start using PHP 8 named arguments? Then we can say

$this->createUser(values: ['uid' => 1, 'name' => 'user1'])->save();

\Drupal::currentUser()->setAccount($this->createUser(['view test entity'], values: ['uid' => 2]));

instead of repeating NULL, FALSE everywhere.

Comment #22

spokje

Dutch

commented 3 January 2023 at 16:10

Is this a good place to start using PHP 8 named arguments?

For me it is, but I would like a wider consensus, since I've seen other code style changes being rolled back because of the lack of it.

Comment #23

znerol commented 4 January 2023 at 06:12

Named arguments would simplify many calls here.

Comment #24

mondrake

🇮🇹

commented 4 January 2023 at 07:04

-1 on named arguments in THIS issue. It’s a coding standard issue first, and once approved we will be able to use that.

That will require time (the coding standard queue is pretty slow atm), there’s not even an issue for named arguments yet AFAICS.

Let’s not slow down this issue on that.

Comment #25

mondrake

🇮🇹

commented 4 January 2023 at 07:09

see #3323994-13: Fix PHPStan L1 errors "Constructor of class Foo has an unused parameter $bar" and following comments for a similar case where coding standards are slowing down an issue.

Comment #26

joachim commented 4 January 2023 at 11:53

> Is this a good place to start using PHP 8 named arguments? Then we can say

I have been a bit concerned that calls to createUser() that only care about entity values are a bit ugly. The order of parameters makes sense if you are creating test users to test access and permissions, which I assume is the most common case.

But switching to named function arguments in the future would help that.

Another possibility for the future would be add a createEntity($entity_type_id, $values) method to KernelTestBase, and then tests that only care about the entity values of a user can call that instead.

Comment #27

joachim commented 4 January 2023 at 13:05

Status:

Needs work

» Needs review

Dealt with @longwave's comments on the MR.

Comment #28

znerol commented 4 January 2023 at 13:31

Status:

Needs review

» Reviewed & tested by the community

Thanks @joachim. This is ready if tests pass.

Comment #29

6 January 2023 at 11:27

catch committed c6babbb6 on 10.1.x

Issue #3324384 by Spokje, joachim, elber, znerol, longwave, mondrake:...

Comment #30

catch

he/him

English

commented 6 January 2023 at 11:27

Status:

Reviewed & tested by the community

» Fixed

Committed c6babbb and pushed to 10.1.x. Thanks!

Comment #31

joachim commented 6 January 2023 at 13:13

Should we file an 11.x follow-up to remove the deprecation handling?

Comment #32

catch

he/him

English

commented 6 January 2023 at 14:00

It should be covered by the generic 'remove deprecated code from core' issues when they're opened, we generally remove deprecations at a larger scale than they're added.