Problem/Motivation

The @var annotation of UserAuthenticationController::LOGGED_IN and UserAuthenticationController::LOGGED_OUT suggest these constants are strings when they are in fact ints.

Steps to reproduce

Proposed resolution

Change them from ints to strings

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 issues-3412459-Incorrect-typehint-fixes.patch734 bytesviren18febs

Issue fork drupal-3412459

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.

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

viren18febs’s picture

viren18febs commented
Status: Active » Needs review
StatusFileSize
new issues-3412459-Incorrect-typehint-fixes.patch734 bytes

Hi , i have fixed and added a patch for this , please review

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Can we please have an MR for this?

viren18febs’s picture

viren18febs commented
Assigned: Unassigned » viren18febs

viren18febS opened merge request !6037

viren18febs’s picture

viren18febs commented
Assigned: viren18febs » Unassigned

Generated MR for the same, please review.

viren18febs’s picture

viren18febs commented
Status: Needs work » Needs review
smustgrave’s picture

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

@viren18febS this was tagged for novice intended for newer users to Drupal contribution.. Looking at your post history you have 14 pages of issues and should avoid novice issues in the future.

viren18febs’s picture

viren18febs commented

Hi smustgrave,

Sure i'll take care of that.
Thank you.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

The comments above also mention strings, but the values are not strings.

viren18febs’s picture

viren18febs commented
Status: Needs work » Needs review
smustgrave’s picture

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

Feedback appears to be addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Just realised actually where this is used:

    if ($this->currentUser()->isAuthenticated()) {
      $response = new Response(self::LOGGED_IN);
    }
    else {
      $response = new Response(self::LOGGED_OUT);
    }

The Response constructor takes a string!

    public function __construct(?string $content = '', int $status = 200, array $headers = [])

So this is being cast back to string anyway; should we actually be changing them to e.g.

  const LOGGED_IN = '1';
mstrelan’s picture

mstrelan commented

Agree with #14, the doc suggests this should indeed be a string. I suspect this may be misused in userland and might break if people are checking $foo === UserAuthenticationController::LOGGED_IN.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Sounds like this needs to be re-scoped.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Issue tags: -Needs issue summary update
mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Have updated the MR with the new approach to use strings. Added a CR just in case anyone is using these constants with strict comparisons.

Apologies for picking this up with the Novice tag, it is blocking #3404508: Fix strict type errors in Functional tests: Add casts and fix types where needed so I was motivated to work on it.

smustgrave’s picture

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

@mstrelen thanks for noting that, think if it's a blocker for a larger issue that's perfectly valid.

Issue summary was updated by @larowlan in #17 and consts turned to strings.

larowlan closed merge request !6037

  • larowlan committed 442f147b on 11.x 
    Issue #3412459 by viren18febS, mstrelan, smustgrave, longwave, larowlan...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and published the change record, thanks

Status: Fixed » Closed (fixed)

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