Wrong type for \Drupal\Tests\UiHelperTrait::$loggedInUser

Hi — I’m Srushti Kondhalkar (new to Drupal contributions).

I looked at this issue; the PHPDoc says @var \Drupal\user\UserInterface, but the property is initialized as FALSE.
I plan to reproduce and prepare a patch that changes the initial value to NULL so that the type matches.

Could someone please confirm if this approach is acceptable, or point me to the exact file path to update?

Thanks — I’ll follow up with a branch/merge request once I have the code.
— Srushti (srushti_k)

Would it be better to use null instead of false? There might be some BC concerns doing that though.

Discussed briefly with @mondrake, I think it would be much better if we used typed properties:

  /**
   * The current user logged in using the Mink controlled browser.
   */
  protected UserInterface|false $loggedInUser = FALSE;

or even better if we could use

  /**
   * The current user logged in using the Mink controlled browser.
   */
  protected ?UserInterface $loggedInUser;

Well, exactly - that's probably worth doing in bulk in a followup.

For #9

As discussed in #10/11 not sure we should do that here, maybe let's just fix up the docs and then bulk-fix the properties later.

Not sure it's worth changing this isolated case here.

My suggestion would be to do, for core/tests:

• #3551596: Add return types to core/tests code via Rector - round 1 adding some return types
• Round 2 (issue tbd) - complete return types and add argument types via Rector
• Round 3 (issue tbd) - add types to class properties via Rector
• Round 4 (issue tbd) - convert anything that is left out

NULL would indeed be better. For example, in \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::assertPageLoadComplete, we could update $this->loggedInUser && $this->loggedInUser->hasPermission() to just $this->loggedInUser?->hasPermission().

Core doesn't seem to have any cases where we are checking if the property is FALSE, only the loose empty check, but of course contrib would be the issue. I guess the same way would be to deprecate the $loggedInUser property and make a new $authenticatedUser property.

This search reports 22 pages for this->loggedInUser. Some of those are on 7.x branches and I don't see a way to exclude those from the search. So, not really all that helpful.

Backported down to 10.6.x as a documentation fix.

Committed and pushed 422ac9c2aa6 to 11.x and ac5feee6fc6 to 11.3.x and 74bda5e0635 to 10.6.x. Thanks!