Deprecate user_load_by_mail & user_load_by_name

As I can see, the pipeline has failed.

I also found a comment with a link to the current @see https://www.drupal.org/node/3555670 page. As I understand, we use links for CR and not for issues.

+ @deprecated in drupal:11.1.0 - should be 11.3

It also requires replacing all calls in core modules (without removing functions for now).

Hi @astonvictor i have to create the CR? in drupal.org??

it should be possible to use https://www.drupal.org/node/add/changenotice?field_project=3060

Hi @astonvictor can you help me to solve the phpunit pipeline problem??

It's not getting passed phpstan, may need to update the baseline.

I have solve the phpstan error.

For get the active user.

Hi @santanu mondal, I left a review comment.

Added more review coments

I have created a new PR against 11.x and incorporated all the PR review suggestions.

Please review.
https://git.drupalcode.org/project/drupal/-/merge_requests/14455

I probably edited this one too much to be eligible to review it now. During the process of creating the new branch I stripped out all of the unrelated and unnecessary changes. I also undid a line wrap to a ternary statement. As far as I can tell, we don't have coding standards for line-wrapping ternary statements, but we almost universally indent wrapped statements after the first line. In this case there was no indentation. I decided to remove the line wrap entirely because I didn't think it was necessary.

Sorry there is a mismatch in deprecation versions on one.

Did we get confirmation of deprecation without replacement from user module maintainers? @kristiaanvandeneynde or @moshe weitzman

Yes, I've been pondering about that too. There are hundreds of usages of those two functions in contrib, and and loadByProperties() isn't the greatest API IMHO:

https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22user_...

Either we'd add something like User::loadByName(), but we probably want to not promote more static methods, the other option is UserRepository service like we're adding in #2536594: Add a FilterFormatRepository providing methods to load filter formats and have other examples too, I think that's worth considering.

I prefer removal of the functions without replacement. I personally dont think any new service is needed.

If we were doing something special in user_load_by_mail() and user_load_by_name() regarding optimization, then this wouldn't be as clear cut. But, as it stands, it's a simple wrapper around loadByProperties() already. Removing the functions will also make testing and juggling dependencies easier as the classes that used to call it can now use their own copy of the entity type manager service to get the user storage.

So +1 on removal without replacement. It's time to rip the band-aid off for this one.

Maybe we can fix this by introducing #3569814: Introduce EntityRepositoryInterface::loadUniqueByProperties to reduce boilerplate, increase stability and please phpstan. and then changing the deprecation notice to point to that instead?

We should maybe wait until that other issue gets in, so that the CR can tell people about the new API.

Not sure that's necessary, especially for contrib. The good thing about the current "replacement" is that it's not new. It's easy to just convert the calls and it works in any supported and non-supported core version 8.0+. Having a replacement, either here or the proposed API in #33 would require for contrib to do the usual DeprecationHelper/method_exists/version_compare BC dance, which results, at least for now, in more code/complexity than the existing API that's used here.

True, if you already update your module to take care of this deprecation and use the new method, then that's an implicit core version bump. If you use the loadByProperties() and reset() combo, then you're good to go. We could try and add a rule later on that detects said combo and advises to use the new method.

Then again, we have full reign over core. So if we do wait for the other issue to land, at least we could already clean up the calls in core with the new method.

We could update the CR to mention both options, clearly stating the implication of using the newer approach.

I think given the widespread usage of user_load_by_ functions in contrib we're too late to deprecate this for 12.0.0 even though the replacement is already possible. Discussed with @catch and we agreed that the deprecation should be for Drupal 13. See https://git.drupalcode.org/search?group_id=2&scope=blobs&search=user_loa...

Updated MR to address #38

Updating search links:

#30: user_load_by_name($
#38: user_load_by_.*($

The only change since this was set back from RTBC is the deprecation version (and merges), still looks OK.

The MR has git conflict issue it couldn't be rebased from UI.

Needs rebase.

I was able to update the fork in the browser UI. I don't know why it still reports that the MR still needs to be rebased. It doesn't. I pulled it to my local and tried to merge main, but Git said that it's already up-to-date. So I think everything is fine and GitLab is just crazy.

Committed 9a9ada0 and pushed to main, and committed 9151865 and pushed to 11.x. Thanks!

Realizing I never linked the conversation in slack https://drupal.slack.com/archives/C079NQPQUEN/p1769449447565259

I think it meets the bar for credit, but I forgot to mention it:
@lleber and @joachim also participated in slack (@moshe, @berdir, and @kristiaan already received credit for direct contribution)

Credit updated per #50.

Thanks!

Just tried to commit a different issue and ran into a phpstan error on the deprecation.

I've committed https://git.drupalcode.org/project/drupal/-/commit/2d88383f7e08c89a07a56... to workaround this, but I'm wondering if that got added between the last pipeline run here and the commit. We might need a quick follow-up to remove the usage and baseline entry?

@catch's commit broke main, I think maybe due to a rogue file in his local checkout? The job was failing so I reverted that commit and now it's green again, tentatively marking this fixed again.

Yep I had a rogue file in my local checkout (the only rogue file), and it previously hadn't caused any issues, but due to having user_load_by_name() in it, messed up phpstan. Mystery solved at least (and deleted the file).