🇬🇧United Kingdom @pbattino

OK the comment #18 is probably a red herring. Unfortunately I'm not able to run local test right now so I can't debug this.

sorry I changed back the version to 2 by mistake.

Is it possible that it is a problem of defaults?, i.e. the update hook works if you already have one Generic Client enabled, If you don't, it fails.

OK, I see now than when I upload I can select which version I want to test against. Let's see what happens.
Thanks @joseph.olstad

OK, I'll dig it out then. But my question stands: do I need to open a different ticket, since my patch is for 3.x ? (I'm not very familiar with issues spanning different versions developed in parallel.)

Bumping. Can we merge this?

Can somebody please review?

What about changing this part of submitForm() in openid_connect/src/Form/OpenIDConnectSettingsForm.php?

  public function submitForm(array &$form, FormStateInterface $form_state) {
    parent::submitForm($form, $form_state);

    $role_mappings = [];
    foreach ($form_state->getValue('role_mappings') as $role => $mapping) {
      $values = array_values(array_filter(str_getcsv($mapping, ' ')));
      if (!empty($values)) {
        $role_mappings[$role] = $values;
      }
    }

It seems to work for me, I can submit a patch in a couple of hours.

If the role mapping is empty, it should do nothing.

This is what I expected to happen, but unfortunately it seems that if you fill in at least one role, you can't go back. That is, resetting that fieldset with all empty fields, will not remove the mapping but rather map all roles to an empty string, meaning no role will be assigned and all roles will be removed.

In short, it seems that there is no way to "empty" the configuration of role_mappings. Even if you empty all fields in https://www.efas.local/en/admin/config/people/openid-connect/settings#ed... , the configuration will be:

role_mappings:
  editor: {  }
  role1: {  }
  role2: {  }
  role3: {  }
  role4: {  }
  role5: {  }

etc..
To cross-check that this was the problem, I edited the configuration removing altogether the "role_mappings" key and this restored the expected behaviour, i.e. no mapping at all / no removal of already assigned roles.

So perhaps we should just better handle the case of when all mapping fields are empty, regardless of whether they were never used or used but then emptied.
(I stumbled upon this edge-case because errors after fresh install when no mapping is specified: 🐛 Trying to access array offset on value of type null in Drupal\openid_connect\Form\OpenIDConnectSettingsForm->buildForm() Fixed )

All good to me!

The quote was from the explanation in https://www.drupal.org/project/pathauto/releases/8.x-1.11 → , linked from your comment.

AH Yes! Now I understand, it's because the no-longer-needed dependency is a Drupal module, if it were a non-drupal package we would not have this problem, I get it.
Thanks!

Thanks for the explanation Bedrir.
At this point I don't understand what the reason is for using a tool like Composer. Shouldn't each package track its dependencies only?
If it's to avoid this:
" This might cause problems if other modules implicitly use without depending on it."
then I would argue we are defeating the purpose of tracking dependencies. It's the other module that is not doing its job.

Not to mention that we may make the life harder to users who have a conflict with the no-longer-needed dependency. See the case of the BEF module above. Yes, the * allows for some flexibility but it's still an unnecessary packages added on a fresh install.

I'm not a Composer expert, perhaps I am missing something, please correct me if I'm wrong! Thanks!

Why has patch #14 this line:
"drupal/jquery_ui_slider": "*",
Isn't the aim to remove the dependency altogether?

pbattino → created an issue.

Re: #6
I'm not sure it makes sense to populate $userinfo['groups'] in \Drupal\openid_connect\Plugin\OpenIDConnectClientInterface::retrieveUserInfo.
The point is that the claim "groups" may be populated but with other info related to other stuff, while the info to do the roles mapping may be in another claim, for example 'entitlements' or 'roles' (yes, that's a standard OIDC claim too!). It all depends on the IdP configuration.

I developed a patch that allows to set which claim you want to use for the mapping, or none to skip the mapping, but it's for versin 3.x. I don't know how to proceed: should I open a different ticket?

I think it works!
I still get:
Warning: foreach() argument must be of type array|object, null given in Drupal\openid_connect\OpenIDConnect->saveUserinfo() (line 711 of modules/contrib/openid_connect/src/OpenIDConnect.php).
after login, probably because I haven't specified a single role mapping, so the foreach() still gets a 'null'.

Loosely related to this issue: possibility to turn off role mapping. It's related in the sense that if there's such a setting, it would impact all places like this where it's assumed the role_mappings is populated.

oops.. sorry, cross-posting, I did not see the "Please review message"

Is it possible that this error is caused by a missing default value for those fields? If I simply save the form, the problem persists, but if I edit even just one of those fields, then the error disappears. I can see that the openid_connect.settings now has one extra attribute:

role_mappings:
  ...
 <one line for each role>
 ...

pbattino → created an issue.

pbattino → created an issue.