Introduce a OneTimeAuthentication service and deprecate user_pass_rehash

Does user_pass_reset_url() also fit into this? Not sure about user_cancel_url(), it's canceling, but it also calls user_pass_rehash() and it's also about one-time-authentication, not for login, but for something else. Especially since you mention the e-mail verification issue, which I think is a very similar use case.

thinking out loud: Less sure about user_mail_tokens(), that's a token helper and not really an API. It's used in UserHooks, specifically the user mail hook, so that will eventually go into a mail plugin or something? The user notification service is probably not a good fit? What's slightly awkward is that we'd need to make this public as it's called indirectly through the token service. that also doesn't yet support service-callbacks yet, but since this is not persisted (like form and batch callbacks), I'm fine with a [$this, 'someMethod'] callback here, which should work fine, despite it being references as $function in Token::replace().

> This has some usage in contrib. I'd probably add an interface here.

To me, if we add an interface isn't determined by whether there are calls but if we want to support alternative implementations. Which I think we do not. So I'd say non-internal but not interface.

The advantage of that it is that it's much easier to add a new method to a service/class without interface. So the password change confirmation could be added without having to worry about BC/something else implementing that interface.

Like this a lot, had a few questions/comments.

Still a couple of open threads.

I'm not sure the @api is necessary, it's a service so it's already included as api.

I think this is nearly ready, it should not be final, other than that I think this covers what we need.

Meant needs work

Removing final from the core/modules/user/src/OneTimeAuthentication.php class, for some reason my suggestion didn't reopen the thread.

I know the server command is internal, I'm not sure we can just remove the method without deprecation though.

Other than the the open comments I think this is ready, I've been I've this a few times and don't see anything else.

I'll leave the threads open for visibility to committers.

Edit: I updated the CR title too.

I made a few comment on the MR. Back to NW for the coding standards (variable names).

There are several open threads on the MR about BC policy. Instead of setting the issue to RTBC in order to get a committer's opinion, I am adding the tag for review from a release manager.

Another BC question: since Drupal\user\Controller\UserController::validatePathParameters is protected, is it OK to deprecate it in 11.4 and remove it in 12.0? Even if it is OK, is it feasible? That is, the current MR targets the main branch: if the deprecation message targets 12.0, then when will we actually remove it? I think it should be removed in Drupal 13.0.

Related: why is that function not simply added to the table (in the issue description here and in the change record)? Again, is that because it is protected, so it only matters if someone extends the controller class?

Finally, I am adding the tag for change record (CR) updates. I find it really helpful when CR has explicit before-and-after code snippets.

Posted some comments on the MR. Hopefully none of it derails things. This is looking nice.

I am satisfied with the code changes made in response to my MR comments. Thanks!

There are two new open threads on the MR from @heddn, both related to the change record. I am re-adding the issue tag for that, and back to NW.

I think all that is left is RM review. But we can still mark RTBC, right?

Right. The RM tag is just to get an opinion on the BC implications, and we can also get that from the committer's review. Besides, we seem to have reached consensus.

There is also the question of whether to make the new class final. I am reopening one thread to call attention to that question.

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.

@benjifisher tagged this for release manager review in #21.

Consulted with @longwave about the protected methods:
Protected methods are considered @internal and do not have a BC promise. There's also time for removal by D12, so this deprecation is OK.

Sometimes there can be considerations if the protected method is used a lot, but neither one in this MR is.
getOneTimeLoginUrl has no usages in contrib:
https://search.tresbien.tech/search?q=getOneTimeLoginUrl&num=450&ctx=0
validatePathParameters has two, but neither are extending UserController
https://search.tresbien.tech/search?q=validatePathParameters&num=450&ctx=0

Is the other thing for RM review about the API/not API thread on OneTimeAuthentication? https://git.drupalcode.org/project/drupal/-/merge_requests/15178/diffs?f...
If so, I'll consult the other RMs.

@godoislate:

Yes: I re-opened that thread for visibility. Should the new class be final or marked @api and related questions.

We have an existing policy issue for final: #3488476: [policy, no patch] Allow using @final when a class is not considered as an extension point, it got pretty emotional and I think nobody wants to get even close to that issue anymore, including myself. Plenty of "final class" examples in core in (previously) experimental subsystems/modules such as default content, navigataion, jsonapi that happened without having an explicit policy, some @final too. Personally I just stopped doing final class and mostly @final to avoid that issue.

Personally I like final and don't buy the it's open source and therefore we should not use final. I think being explicit that we don't support extending certain parts of our codebase is valid for many reasons that do no need to be iterated over here. I've reviewed the moved code and ensured that it's not regressed on security important things like the use of hash_equals() to compare hashes. I think we should merge this one as is and we can continue debating final / @final in the other issue. Committing this one as final does not make the situation in core anymore or less consistent as we have final classes already.

I'm not keen on adding final to everything, but I think in this specific case we really want to actively discourage and as far as possible prevent overriding the class, so it seems fine here and as @alexpott notes it's not the first usage and the policy issue is open to change things across core.

The deprecations seem fine.

I don't think the class needs the @api tag, the reasons for ever having to interact with it directly are very specific, it's not a value object or anything like that.

Removing the RM review tag.

Committed and pushed a38f1d17600 to main and f8884040145 to 11.x and 70276131688 to 11.4.x. Thanks!

I merged this to 11.x and 11.4.x so contrib and custom gets the whole of the 12.x cycle to replace deprecated usages.

I think there might be something wrong with this change. For me, both locally and in CI, this broke the use of $this->drupalLogin() in tests – see, e.g., here:

ArgumentCountError: Too few arguments to function Drupal\user\OneTimeAuthentication::__construct(), 0 passed in /builds/project/search_api_autocomplete/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 and exactly 2 expected

I also tested with a test from Core (Drupal\Tests\user\Functional\Views\AccessRoleTest::testAccessRole()), same error.
I’m unfamiliar with the Drupal\user\OneTimeAuthentication: ~ syntax in user.services.yml, but it seems that that is assumed to also enable autowiring, but doesn’t? For me, the attached patch fixes both the tests in my contrib modules as well as the Core test.

I don’t know why the pipelines passed in the MR, though. Perhaps there is something different in the CI setup for Core and contrib modules? If there is something I can/should do in my contrib modules to fix this then that would be fine, too.

Edit: It’s not just a CI problem, both drush uli and the “Reset password” form on the site are also broken.

@drunken monkey it's because in main/12.x the user.services is autowired and in 11.x / 11.4.x it is not. Should have caught this on merge. I've pushed a quick follow-up to resolve this.

@drunken monkey thanks for reporting this - I've also tested drush uli and the reset password form - it's all now working.