Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct user variable in hook_user_insert #74

Merged
merged 1 commit into from
May 24, 2022

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented May 24, 2022

$user is never defined anywhere.

It ends up not making a difference when creating drupal users via UI because the only thing it changes in synchronizeUFMatch is that it reassigns $uniqId (the email address) to $params['email'] a second time before calling Contact.create. i.e. once here and then again here.

The Drupal vs Drupal8 also doesn't make a difference because that param is only used when it's wordpress or joomla.

This surfaced because of civicrm/civicrm-core#23515. There aren't any tests in this repo but there are tests elsewhere that failed, e.g. https://github.com/SemperIT/CiviCARROT/runs/6566995948?check_suite_focus=true#step:19:44, where the drupal user isn't created in the UI.

@KarinG
Copy link
Contributor

KarinG commented May 24, 2022

Also seeing it in WFC ->
image

@KarinG
Copy link
Contributor

KarinG commented May 24, 2022

Drupal\Core\Entity\EntityStorageException: Mandatory key(s) missing from params array: one of (first_name, last_name, email, display_name)

@demeritcowboy
Copy link
Contributor Author

Noting you can also reproduce the fail with drush:

drush user-create abc --password=12345 --mail=a@b.org

@eileenmcnaughton eileenmcnaughton merged commit 33f94cc into civicrm:master May 24, 2022
@demeritcowboy demeritcowboy deleted the user branch May 24, 2022 19:10
@demeritcowboy
Copy link
Contributor Author

Thanks @eileenmcnaughton @KarinG

@KarinG
Copy link
Contributor

KarinG commented May 24, 2022

Thank you Dave! Re-running WFC tests now to tripple confirm.

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy @KarinG does this only affect d8 - i saw a comment on chat that it might not?

@KarinG
Copy link
Contributor

KarinG commented May 24, 2022

Sorry - comment was about the Student vs student and the excessive use of strtolower in WFC -> that dates back to D7.

I’m not sure about this PR/issue affecting D7 - Dave?

@demeritcowboy
Copy link
Contributor Author

It would have come up in core tests since drush creates the webtest user during those runs, but no the drupal 7 code looks ok to me.

@eileenmcnaughton
Copy link
Contributor

is it surprising that this was wrong for so long & not found until the change I made?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants