Optimize user logins by avoiding duplicate entity queries

Another one:

UserLoginForm::validateName() does exactly the same checks as UserLoginForm::validateAuthentication() except that the latter doesn't set the same error message if it can't find an account. If we move the validation message around, we save yet another identical entity query.

Applied the MR and verified I could still login. Test updates show the coverage so think this is good.

Pushed an additional commit to deprecate ::validateName() instead of removing it, and removed the call to user_is_blocked() by just checking status on the entity, this removes an extra query on a failed login too.

Also opened 📌 Optimize UserLoginController by remove duplicate entity queries RTBC as a follow-up, doing it here would double the MR size since UserLoginController more or less copies the form logic 1-1, so we need to make all the same changes there more or less.

Back to needs review for those changes.

Deprecation seems fine, still can login at least.

Leaving at RTBC after docs fix.

As we add a new public method let’s strict type all params and the return

Prashant.c → made their first commit to this issue’s fork.

Attempting to make a contribution here, I've modified aspects concerning strict types in both function parameters and return values. After local testing, I can confirm that the login functionality is functioning as expected following these adjustments. Updating the status to NR.

Thanks

We need a change record for the UserAuthInterface method addition too, should probably be a separate change record to the form validation deprecation.

Set to NW because of "Needs change record" tag

I attempted to create a change record https://www.drupal.org/node/3411040 → (Draft) as mentioned in #17 📌 Optimize user logins by avoiding duplicate entity queries Needs work . It may require a lot of changes.

Kindly review, thankyou.

MR has a test failure.

Have not re-reviewed.

Do we need to add the interface and the string strict typing here? Given the test failures, that feels like more disruptive then this was originally scoped.

The existing method on the interface definitely shouldn't have type hints added in this issue, that's out of scope (and would have to be implemented completely differently to how it is in the MR due to bc).

Reverted the out of scope changes to the methods we're not adding here, tightened up type hinting on the new method, also made the new method return bool instead of bool/int since we already have the uid available before we call it.

I fixed one small issue with a doc block. But that was so small I doubt it blocks from re-RTBCing. LGTM.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased. Just a couple of merge conflicts in the assertions. 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed will allow all the assertions in these tests to use assertSame() which will make it much easier to see the impact of changes like this.

Rebased again after 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed which now makes it much easier to see what the effect on the database query count is.

Rebased again now that[#3396196] has landed. Easier to see the impact now.

I'm triaging RTBC issues → . I read the IS, the comments and the MR. I didn't find any unanswered questions or other work to do.

The diff no longer applies, setting to needs work. Also, I left some comments in the MR.

Rebased, accepted the suggestions, and updated the comment. Since all those changes were minor, moving back to RTBC.

Also removing the 'needs change record' tag since that's done now.

Nice work on this optimization, greatly appreciated!

Rebased after 📌 Log every individual query in performance tests Needs work

The change records need a bit of work.

https://www.drupal.org/node/3411040 → says we are adding a new interface, but the interface already exists; we are only adding a new method to it.

https://www.drupal.org/node/3410706 → doesn't really have enough information. We should explain briefly why ::validateName() is deprecated - the code is merged into ::validateAuthentication(), which isn't clear - and what to do if you were overriding the login form and calling this before.

Saving issue credits.

Updated both CRs.

Committed and pushed 589b8fe870 to 11.x and 95cc37fbfa to 10.3.x. Thanks!

Also opened 📌 Optimize \Drupal\basic_auth\Authentication\Provider\BasicAuth::authenticate() Needs review as a follow-up.

alexpott → changed the visibility of the branch 3410582-optimize-user-logins to hidden.

alexpott → changed the visibility of the branch 3410582-optimize-user-logins to active.

This seems problematic.

It break https://git.drupalcode.org/project/mail_login/-/blob/8.x-2.x/src/AuthDec... completely because that implements the interface directly as a decorator.

It also breaks the intent of that API because that and other modules (including our custom implementation) used that to allow logging in with the user e-mail and not only as the username. The new method doesn't allow that, because the user entity was already loaded.

Seems like maybe the whole thing should go the other way and maybe this API should check blocked status?

Based on feedback from @berdir
something overlooked here.

I had a quick look at putting the blocked logic on UserAuth::authenticate() but that runs into problems with the $uid/FALSE return value.

There are three reasons the method could return FALSE:
1. The username doesn't exist
2. The password is wrong
3. The user is blocked.

For the login form, we currently tell people if the user is blocked - so to get this information we need to have a loaded user, but we can't get that information from a uid/FALSE return value. If we wanted to load the user outside this method and check if they're blocked, we'd need the uid, but you only get that if the auth was successful, so it would have to run the entity query again.

This makes me think we maybe need a new interface, it could be called UserAuthenticationInterface, where the equivalent of ::authenticate() gives you TRUE/FALSE instead of a user ID, but it has extra methods to get the account object, whether it was found at all, whether it was blocked, so that calling code can find that information out without running entity queries again.

I don't have a full picture yet, but having a new interface with new methods makes sense to me, then we can check for that and trigger a deprecation + keep the old logic for those sites.

This doesn't cleanly revert so I've made a revert MR. Moving direct to RTBC because it's only a couple of small merge conflicts in StandardPerformanceTest so should be good as long as gitlab comes back green.

#2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController → is doing pretty much what I think we need for #3410582-43: Optimize user logins by avoiding duplicate entity queries → so I may just try to revive that issue.

Committed/pushed the revert MR. Back to needs work, but we might end up in #2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController → instead.

catch → changed the visibility of the branch 3410582-revert to hidden.

I started to look at re-rolling #2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController → but found a couple of issues - it's very far behind and much, much bigger scope than this issue, but more importantly, it would make the mail_login bc break potentially worse by ignoring the old service and using a brand new one, meaning logging in with e-mail would silently fail to work.

However thinking about the various ways the bc break could be ameliorated resulted in an idea - new interface UserAuthenticationInterface with two new methods: ::lookupAccount() and ::authenticateAccount(). mail_login then would only need to override ::lookupAccount() and ::authenticateAccount() could fall through to the interface.

Pushed up changes to the original MR - reverting the revert, then refactoring a bit to make the deprecation more graceful per the above.

Because we're deprecating the old interface we now need to incorporate 📌 Optimize UserLoginController by remove duplicate entity queries RTBC and 📌 Optimize UserLoginController by remove duplicate entity queries RTBC here to avoid triggering deprecation errors from those classes.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

Updated the CR as well.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Pretty sure the MR is mergable, it might be the needs review bot not liking the revert of the revert.

Opened 📌 [11.1] Deprecate UserAuthInterface Active .

Circled back on this one and appears all threads and feedback have been addressed.

@catch that's a really neat way of addressing the problem. We still support the old interface till Drupal 12 but we allow us to optimise using the new interface. Great idea. We can work out how to clean this up during the 11.x cycle.

I think we could have a follow-up to deprecate UserAuthInterface and remove it from UserAuth.

Committed and pushed 31edb2aa23 to 11.x and 16082d0849 to 10.3.x. Thanks!

Oops I didn't spend enough time reviewing \Drupal\user\Controller\UserAuthenticationController. This did not implement the BC correctly. It still needs to call $this->userAuth->authenticate() when the injected $this->userAuth is the old interface - otherwise any of the existing decorations are going to be broken.

Good spot, that would have been annoying. Added the extra interface check - kept it in the one if clause so that it's easier to remove.

I think we still need to change things to prevent double authenticating on failure.

Applied the changes, although they didn't quite work, but close enough after some tidying.

Left a comment on the MR.

Checking the Change records https://www.drupal.org/node/3410706 → when is this deprecated to be removed? May answer a question on the MR.

Applied the suggestion on the MR and replied to the other thread.

Thanks, sorry for not catching that before.

Committed and pushed 1916b7863e to 11.x and 2076c3d9fe to 10.3.x. Thanks!

Tagging this for release highlights - it's not much by itself but combined with other performance improvements could make up part of a decent paragraph.

Automatically closed - issue fixed for 2 weeks with no activity.

Seems like we have a bug and BC break here, please see 🐛 BC break in login auth changes from #3444978 Fixed
Updating to Drupal 10.3.0 currently breaks logins with certain login related contrib modules.

Thank you.