Problem/Motivation

The return type for \Drupal\openid_connect\OpenIDConnect::createUser is UserInterface|null but it can never return null. This means that external code using this function needs to handle the null case, even though it doesn't exist.

Currently ::createUser calls \Drupal\externalauth\ExternalAuthInterface::register which only returns UserInterface. Following through to the implementation of that function confirms that null is never returned. Instead, if there is an issue creating a user then an exception is thrown.

Steps to reproduce

Proposed resolution

Drop null from the return type and documented typehint.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork openid_connect-3504426

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.

mstrelan opened merge request !137

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
pfrilling’s picture

pfrilling
Location Minster, OH
commented

The code looks fine to me. I left a comment on the MR about handling the exception. What do you think?

mstrelan’s picture

mstrelan commented

That's a tricky one to answer. The only place this module is calling createUser is in OpenIDConnect::completeAuthorization, and that already has checks to see if the user already exists. In that case it would be fine for the exception to bubble up. The doc block for \Drupal\externalauth\ExternalAuthInterface::register doesn't mention that it throws that exception either, so in theory any concrete class implementing that interface doesn't need to throw that exception either. So I think bubbling up would be fine.

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the validation @mstrelan. I think this all looks good.

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Reviewed & tested by the community » Fixed

  • pfrilling committed 6a6f708a on 3.x authored by mstrelan 
    Issue #3504426 by mstrelan, pfrilling: OpenIDConnect::createUser return...
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: +#3506932: non-existent service "openid_connect.openid_connect" after update

Alpha6 causes an issue in keycloak reported by two others

I'm not sure which change in alpha6 is causing this.

mstrelan’s picture

mstrelan commented
Related issues: -#3506932: non-existent service "openid_connect.openid_connect" after update

Removing (un)-related issue

Status: Fixed » Closed (fixed)

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