Bug: Assigning null to ?array $identities property without proper handling

[WCrash7]

PHP Version

8.2.7

CodeIgniter4 Version

4.6.3

Shield Version

1.2.0

Which operating systems have you tested for this bug?

Windows

Which server did you use?

cli-server (PHP built-in webserver)

Database

mysqlite(memory unittest)

Did you customize Shield?

yes:

class User extends \CodeIgniter\Shield\Entities\User
.
.
.
public function saveUsernameIdentity(): bool
	{
		if (empty($this->username) && empty($this->password) && empty($this->password_hash)) {
			return true;
		}

		/** @var UserIdentityModel $identityModel */
		$identityModel = model(UserIdentityModel::class);

		$identity = $this->getIdentity('username_password');
		if ($identity === null && !empty($this->username)) {
			$this->identities = null;

			$this->createUsernameIdentity( [
				'username' => $this->username,
				'password' => '',
			]);


			$identity = $this->getUsernameIdentity();
		}

		if ($identity !== null) {
			if (!empty($this->username)) {
				$identity->secret = $this->username;
			}

			if (!empty($this->password)) {
				$identity->secret2 = service('passwords')->hash($this->password);
			}

			if (!empty($this->password_hash) && empty($this->password)) {
				$identity->secret2 = $this->password_hash;
			}

			try {
				$identityModel->save($identity);
			} catch (\CodeIgniter\Database\Exceptions\DataException $e) {
				$messages = [
					lang('Database.emptyDataset', ['insert']),
					lang('Database.emptyDataset', ['update']),
				];
				if (in_array($e->getMessage(), $messages, true)) {
					return true;
				}

				throw $e;
			}
		}

		return true;
	}

What happened?

the $identities property is correctly declared as nullable array: https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L50

However, in several places, we directly assign null like these:

https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L130

https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L149

TypeError: CodeIgniter\Shield\Entities\User::setIdentities(): Argument 1 ($identities) must be of type array, null given,

Steps to Reproduce

try to reload all identities

Expected Output

reload identities without error

Anything else?

No response

[datamweb]

I’m not able to understand the issue you’re describing. While your point about method setIdentities might be valid in theory, we actually don’t use that method anywhere in our codebase.

You should try to provide at least the minimal required code along with clear steps to reproduce the issue.

As for loading, I have no problem doing it in the following way.

try to reload all identities

dd( auth()->user()->getIdentities());

[WCrash7]

in my code: . . . $identity = $this->getIdentity('username_password'); if ($identity === null && !empty($this->username)) { $this->identities = null; . . . This line is used to set identities to null in order to force reloading of identities. $this->identities = null; The statement calls this function: https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L130

We set identities to null because the getIdentities() method: https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L111 calls $this->populateIdentities() https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L113 For reloading identities after changes, $this->identities must be null:

https://github.com/codeigniter4/shield/blob/d07c0f9442dd0712b3fa39bfba677838ff1b8e26/src/Entities/User.php#L93C2-L101C6

This behavior is necessary when you update a user's identities (e.g. username, email, or password) and need the updated identities to be reloaded from the database on the next access.

[datamweb]

So, based on this, you are suggesting that the method signature should be changed to:

public function setIdentities(?array $identities): void

Is that correct?

[WCrash7]

Yes