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

Override the DrupalBase getUserObject function with a Drupal8/9 compa… #19517

Merged

Conversation

seamuslee001
Copy link
Contributor

…tible version

Overview

This fixes an error when trying to use the User v3 API on Drupal 9 as per https://civicrm.stackexchange.com/questions/38766/call-to-undefined-function-user-load-using-drupal-9-and-api3

Before

Undefined function user_load

After

no undefined function

ping @adixon @KarinG @MikeyMJCO @eileenmcnaughton

@civibot
Copy link

civibot bot commented Feb 3, 2021

(Standard links)

@civibot civibot bot added the master label Feb 3, 2021
@homotechsual
Copy link
Contributor

This looks good to me, we've been using the same approach locally in testing this out anyway.

@eileenmcnaughton
Copy link
Contributor

Merge on pass based on @MikeyMJCO approving

@adixon
Copy link
Contributor

adixon commented Feb 3, 2021

What!!! I just went to lunch and you fixed it? I can't even find my hand-clapping emojis before you're done here.

Also, any clues about why this fixes it? is it because user_load only gets called from getUserObject and we can just redefine that and ignore user_load? If so, that sounds like a better solution than mine, though I wonder if there are other instances of code out there that call user_load within the civicrm.module.

@homotechsual
Copy link
Contributor

What!!! I just went to lunch and you fixed it? I can't even find my hand-clapping emojis before you're done here.

Also, any clues about why this fixes it? is it because user_load only gets called from getUserObject and we can just redefine that and ignore user_load? If so, that sounds like a better solution than mine, though I wonder if there are other instances of code out there that call user_load within the civicrm.module.

It looks like anywhere else D8 or 9 relevant has already been refactored and this was missed (or it worked and there was no urgency :-) )

@homotechsual
Copy link
Contributor

@seamuslee001 I'm curious why our automated testing didn't pick up the deprecation - do we just not have test coverage for this?

@seamuslee001
Copy link
Contributor Author

@MikeyMJCO I would say 1) we don't run the standard api_x tests on D8/D9 instances 2) I'm not sure if there are unit tests covering this specific API

@MikeyMJCO @adixon in regards to user load

There are none in the d8 module it seems https://github.com/civicrm/civicrm-drupal-8/search?q=user_load

These are the ones it is finding in core https://github.com/civicrm/civicrm-core/search?q=user_load and for all but the OG bridge (which is not set up for d8 AFAIK) there are replacement Drupal8 files that override those calls found AFAIK

@eileenmcnaughton eileenmcnaughton merged commit 4ccc62d into civicrm:master Feb 3, 2021
@eileenmcnaughton eileenmcnaughton deleted the fix_user_load_deprecation branch February 3, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants