CommentFileSizeAuthor
#7 ldap-SharedemailSupport-2883852-7-D8.patch7.71 KBboromino

Issue fork ldap-2883852

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

grahl created an issue. See original summary.

grahl’s picture

grahl
he/him
Location Zürich
commented
Status: Active » Postponed
Related issues: +#2717567: Drupal 8 version of Shared eMail

We're waiting on the port of sharedemail.

grahl’s picture

grahl
he/him
Location Zürich
commented
Assigned: grahl » Unassigned
grahl’s picture

grahl
he/him
Location Zürich
commented
Category: Task » Feature request
baikho’s picture

baikho commented

@grahl, D8 Port is done. Let me know if you experience any issues

baikho’s picture

baikho commented
Status: Postponed » Active
boromino’s picture

boromino commented
Status: Active » Needs review
StatusFileSize
new ldap-SharedemailSupport-2883852-7-D8.patch7.71 KB
dravenk’s picture

dravenk
He/Him
Primary language English
commented
Version: 8.x-3.x-dev » 8.x-4.x-dev
dravenk’s picture

dravenk
He/Him
Primary language English
commented
Issue tags: +Needs reroll
grahl’s picture

grahl
he/him
Location Zürich
commented
Status: Needs review » Needs work

Thanks everyone for their work on this.

I don't feel comfortable adding this in, knowing how complex the interaction with email can be in all setup variants and what potential side-effects this could introduce.

Ideally this would be a separate module but at the moment the two cases in LoginValidator and DrupalUserProcessor could probably be fixed more easily for users of the sharedemail module by a simpler patch just changing those lines.

If someone wanted to try going the separate module route I'd be happy to commit a patch to ldap which provides an appropriate alter hook to allow changing the behavior of conflict resolution, but I'd like to avoid coupling ldap directly to sharedemail (which is a module one should avoid using with Drupal, from my perspective).

luongosb’s picture

luongosb commented

I'd be willing to write something if you made the hook available.

grahl’s picture

grahl
he/him
Location Zürich
commented

Thanks for the initiative vetchneons, could you maybe also write a quick patch for the necessary hooks and add it here? I don't have much time at the moment for ldap.

boromino’s picture

boromino commented

Is there any progress on this? I need the feature and would be willing to create a module. @vetchneons Did you already start? Can we join forces?

boromino opened merge request !14

boromino’s picture

boromino commented
Status: Needs work » Needs review

I have added a hook to alter $account_with_same_email. I have also created a module ldap_sharedemail. I will publish the module as soon as the hook is available in the ldap module.

grahl’s picture

grahl
he/him
Location Zürich
commented
Status: Needs review » Needs work
Issue tags: -Needs reroll

Hi boromino

Thanks for the MR. Could you take a look at the two following things?

  • The hook ldap_account_with_same_email isn't documented in ldap_user.api.php, could you add that? The other hooks start with ldap_user so that might be good for consistency.
  • I'm unsure why the $account_with_same_email instanceof UserInterface check should be necessary. From my perspective it should not change its type when coming back from the alter hook and from the database it's basically guaranteed to be a user entity or false. Could you remove that or explain its necessity? Maybe the api documentation would also already answer that.

boromino opened merge request !15

boromino closed merge request !15

boromino’s picture

boromino commented
Status: Needs work » Needs review

I have added documentation in ldap_user.api.php with similar name to other hooks.

I had added $account_with_same_email instanceof UserInterface check, because $account_with_same_email was passed as reference to the hook. Hence a hook implementation could set it to anything other than false and UserInterface. However I have now opted for a different approach in order to be consistent on the return value and on the value to be passed to the hook implementations.

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

bluegeek9’s picture

bluegeek9 commented
Title: Port sharedemail support to D8 » Handle duplicate emails
Status: Needs review » Active

There are two other issues related to non-unique emails. I will use this issue as the parent, it is the oldest.

#3279539: Duplicate email addresses during LDAP sync cause GroupUserUpdateProcessor to abort
#3304327: Fix LDAP Query does not continue all users if the user already exists in the system with the same email address