Convert user_pass_reset to a new-style Form object

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+ * Contains Drupal\user\UserPasswordController.

Contains \Drupal

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException

Missing ;

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+ * Form controller for the user password forms.                               ¶
...
+    global $user;
+  ¶
+    // When processing the one-time login link, we have to make sure that a user

Whitespace

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+class UserPasswordController implements ControllerInterface {

Missing create() and __construct

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+        $reset_link_account = user_load($uid);
...
+      $account = user_load($uid);

Should inject the entity manager and load from that

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+      $timeout = config('user.settings')->get('password_reset_timeout');

Inject config as well

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,99 @@
+          drupal_goto('user/password');

return new RedirectResponse

+++ b/core/modules/user/user.moduleundefined
@@ -895,13 +895,10 @@ function user_menu() {
-  $items['user/reset/%/%/%'] = array(
+  $items['user/reset/%user/%timestamp/%hashed_pass'] = array(

This can stay as it was

The following patch makes the controller implement FormInterface as well as ControllerInterface. Hopefully should get a few more tests passing however it still needs work getting things injected into the controller.

Go bot.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+    $this->entityManager = $entity_manager;

No need to store the entity manager. All we need is the storage controller.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+    return new static($container->get('plugin.manager.entity'));

You can directly inject the storage controller here. We don't need the entity manager.

$container->get('plugin.manager.entity')->getStorageController('user'));

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+   * Menu callback; process one time login link and redirects to the user page on success.

80 characters. Just use {@inheritdoc} please.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+    $user = $request->attributes->get('account');

That variable should be named $account.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+        drupal_set_message(t('You are logged in as %user. <a href="!user_edit">Change your password.</a>', array('%user' => $user->name, '!user_edit' => url("user/" . $user->id() . "/edit"))));

We can now inject t() (@see ModuleListForm for an example). Same applies to all other occurrences of t() in here.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+      return new RedirectResponse(url('<front>', array('absolute' => TRUE)));

Should use UrlGenerator::generateFromPath(). Same applies to all other occurrences of url() in here.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+      $account = $this->userStorageController->load($uid);

Should be called $user then ($account should always be the currently logged in account from the request imho).

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+      if ($timestamp <= $current && $account && $account->status) {

Should use UserInterface::isActive()

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+        if ($account->login && $current - $timestamp > $timeout) {

Should use UserInterface::getLastLoginTime(). Same applies to all other occurrences of $account->login here. In general: Always use getter methods instead of accessing the properties directly.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+        elseif ($account->isAuthenticated() && $timestamp >= $account->login && $timestamp <= $current && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login)) {

Should use UserInterface::getPassword() and UserInterface::getLastLoginTime().

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+            watchdog('user', 'User %name used one-time login link at time %timestamp.', array('%name' => $account->name, '%timestamp' => $timestamp));

Should use $account->getUsername(). Same applies to all other occurrences of $account->name.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,156 @@
+        throw new AccessDeniedHttpException();

Instead of that you should probably implement a custom access check service.

+++ b/core/modules/user/user.routing.ymlundefined
@@ -75,6 +75,14 @@ user_pass:
+  pattern: '/user/reset/{uid}/{timestamp}/{hashed_pass}/{op}'

We always try to avoid shortcuts. Let's use {hash} and {operation} for example.

Thanks @fubhy, I will hopefully take a look this week!

This implements the changes suggested by @fubhy (apart from the access checker for now) - plus I'm also injecting and using the user settings config service.

Hopefully this should be closer to passing!

Let's try that again!

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,180 @@
+              $form['message'] = array('#markup' => $this->translationManager->translate('<p>This is a one-time login for %user_name and will expire on %expiration_date.</p><p>Click on this button to log in to the site and change your password.</p>', array('%user_name' => $user->getUsername(), '%expiration_date' => format_date($timestamp + $timeout))));

format_time() can now be replaced by the date service.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,180 @@
+  public function validateForm(array &$form, array &$form_state) {}
...
+  public function submitForm(array &$form, array &$form_state) {}

Afaik the proper codestyle for empty brackets are using a newline between them.

+++ b/core/modules/user/user.moduleundefined
@@ -830,11 +830,8 @@ function user_menu() {
   $items['user/reset/%/%/%'] = array(
     'title' => 'Reset password',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('user_pass_reset', 2, 3, 4),
-    'access callback' => TRUE,
+    'route_name' => 'user_pass_reset',
     'type' => MENU_CALLBACK,
-    'file' => 'user.pages.inc',
   );

No need to keep this menu callback entry any longer.

+++ b/core/modules/user/user.pages.incundefined
@@ -179,7 +96,7 @@ function user_cancel_confirm_form($form, &$form_state, $account) {
-    $question = t('Are you sure you want to cancel the account %name?', array('%name' => $account->getUsername()));
+    $question = t('Are you sure you want to cancel the account %name?', array('%name' => $account->name));

@@ -227,7 +144,7 @@ function user_cancel_confirm_form_submit($form, &$form_state) {
-    watchdog('user', 'Sent account cancellation request to %name %email.', array('%name' => $account->getUsername(), '%email' => '<' . $account->getEmail() . '>'), WATCHDOG_NOTICE);
+    watchdog('user', 'Sent account cancellation request to %name %email.', array('%name' => $account->name, '%email' => '<' . $account->mail . '>'), WATCHDOG_NOTICE);

@@ -306,7 +223,7 @@ function user_cancel_confirm($account, $timestamp = 0, $hashed_pass = '') {
-    if ($timestamp <= $current && $current - $timestamp < $timeout && $account->id() && $timestamp >= $account->getLastLoginTime() && $hashed_pass == user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())) {
+    if ($timestamp <= $current && $current - $timestamp < $timeout && $account->id() && $timestamp >= $account->login && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login)) {

Oh this change seems to be a in the wrong direction, as it uses the BC decorator instead of the proper methods on the account object.

This patch should clear up everything in #14, on the last point - that just ended up in there from the reroll and I've now made sure it's out of this patch.

It would be nice if you provide an interdiff, see https://drupal.org/documentation/git/interdiff
so it is easier to review again.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,194 @@
+   * @var \Drupal\Core\Routing\UrlGenerator
...
+  public function __construct(UserStorageControllerInterface $storage_controller, TranslationManager $translation_manager, UrlGenerator $url_generator, Config $user_config, Date $date) {

Instead of using the class directly you should better document the interface, which is Drupal\Core\Routing\PathBasedGeneratorInterface

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.phpundefined
@@ -0,0 +1,194 @@
+  /**
+   * Constructs a UserPasswordResetForm object.
+   *
+   * @param \Drupal\user\UserStorageControllerInterface $storage_controller
+   *   The user storage controller.
+   */

You have to document all variables.

This should clear up some of the documentation issues. Should I convert the documentation for other classes to use the interface?

#18: 1998198-user-pass-reset-conversion-17.patch queued for re-testing.

This could reuse the FormBase in some places (getting the request/using t() instead of the translation manager directly)

Rerolled, using FormBase, switched PathBasedGeneratorInterface for UrlGeneratorInterface, as well as using generateFromRoute where possible.

#22: 1998198-user-pass-reset-conversion-21.patch queued for re-testing.

1. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
   @@ -0,0 +1,182 @@
   +   * {@inheritdoc}
   

   Doc block needs update.

2. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
   @@ -0,0 +1,182 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function submitForm(array &$form, array &$form_state) {
   +
   +  }
   

   Do we need this?

3. +++ b/core/modules/user/user.module
   @@ -817,14 +817,6 @@ function user_menu() {
   -    'title' => 'Reset password',
   

   This should be added to yml.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
@@ -0,0 +1,182 @@
+  /**
+   * The user settings config object.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $userConfig;
...
+   * @param \Drupal\Core\Config\Config $user_config
+   *   The user settings config object.
...
+    $this->userConfig = $user_config;
...
+      $container->get('config.factory')->get('user.settings'),

I'm partial to storing configFactory, and getting the particular config in the method. However; calling the config userConfig is better than some I've seen.

So, this isn't a standard drupal form. There are six different code paths, 4 of which are redirects, one is a 403, and the sixth is a "form" with a button that does redirect.

So I think that this needs to be cleaned up in this issue, because nowhere else that I know of do we have an empty submit button.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
@@ -0,0 +1,182 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, $uid = NULL, $timestamp = NULL, $hash = NULL, $operation = 'confirm') {

This doc block. :)

@Albert Volkman please attach interdiff's to any changes you make. Makes it really hard to review without them.

Interdiff is to #28. I don't think the approach in #32 was correct. Splitting buildForm into buildForm and submitForm. This negates the need of the $operation param, but leaving it to not break BC. This was tested and working with drush uli. Adding needs security review because, well this is the reset password link and changing something probably warrants a security review.

+++ w/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
@@ -107,17 +113,18 @@ public function buildForm(array $form, array &$form_state, $uid = NULL, $timesta
+      return new RedirectResponse($this->urlGenerator()->generateFromRoute('<front>', array(), array('absolute' => TRUE)));

Why not use $this->url('') here.

fixes ConfigFactory type hint.

Proceeding as part of drupal mentoring prague

Didnt get time to make much progress

Linking back to meta issue

Here's a re-roll and fixes to align it with the existing code better. Need ot decide if we still want a GET request to work as it used to.

1. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
   @@ -0,0 +1,224 @@
   +      $container->get('config.factory'),
   ...
   +    $this->configFactory = $config_factory;
   

   You don't need this, FormBase comes with a config() method

2. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordResetForm.php
   @@ -0,0 +1,224 @@
   +      return new RedirectResponse($this->urlGenerator()->generateFromRoute('<front>', array(), array('absolute' => TRUE)));
   

   FormBase doesn't have a redirect() method, but this should use the FormBase::url() method

Ok, discussed with Crell. The form builder cannot return redirects, so we need to wrap this in a normal route controller and then render the form within that. Makes for a much cleaner separation of logic anyhow, rather than having all that crap in the form (that code was carried over from ancient Drupal versions).

A little more cleanup of the form builder parameters and logic

Ok, discussed with Crell. The form builder cannot return redirects

So how did it worked before?

@dawehner - before it was also a _content controller wrapping drupal_get_form(), so it's just somehow when using _form it doesn't work.

This just removes the #action being hard-coded which is not required since we don't need to append the operation.

Note also that this patch changes the behavior in a subtle way. Previously, you could make a GET request to the URL with /login appended and get logged in.

Now, it actually must run through the whole form submit code path. I think this is a big improvement, since now modules could add additional validate or submit steps if desired.

Feedback from dawehner in IRC: based on #2110643: ControllerBase::container() and FormBase::container() needs to be private we shouldn't invoke the container() method like:

$form_builder = $this->container()->get('form_builder');

This fixes that call using \Drupal, but opened an issue here to make it part of ControllerBase: #2166863: Add a formBuilder() method to ControllerBase

1. +++ b/core/modules/user/lib/Drupal/user/Controller/UserController.php
   @@ -59,6 +60,75 @@ public function userTitle(UserInterface $user = NULL) {
   +   * @return array
   +   *   The form structure.
   

   Technically this does also return redirect response objects

2. +++ b/core/modules/user/lib/Drupal/user/Controller/UserController.php
   @@ -59,6 +60,75 @@ public function userTitle(UserInterface $user = NULL) {
   +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   +   */
   

   It would be helpful if we would explain here why the exception is thrown.

This patch fixes up the dependency injection, adds comments, and adds a test case for the reset link for a blocked user.

Re-rolled after #2166863: Add a formBuilder() method to ControllerBase. Instead of injecting \Drupal\Core\Form\FormBuilderInterface, we can use the formBuilder() method on ControllerBase.

After this initial conversion is resolved/committed, I'd like to hack on it some more in #75924: Include fields for resetting password on the one-time password reset page.

Simple re-roll, patch looks great now!

Missed in re-roll changed crypt

proper patch

Reroll.

Re-rolled patch..

@tim.plunket
user_cancel_form in user.pages.inc has to be converted into new style controller. It's issue has been closed https://drupal.org/node/1946466. So can here I submit patch for it ?

62: user-pass-1998198-62.patch queued for re-testing.

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -18,6 +22,110 @@
   +   * @param \Drupal\Core\Datetime\Date $date
   +   *   The date formatting service.
   +   */
   +  public function __construct(Date $date, UserStorageInterface $user_storage) {
   

   User storage missed in docs

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -18,6 +22,110 @@
   +  public function resetPass($uid, $timestamp, $hash) {
   +    $account = $this->currentUser();
   +    $config = $this->config('user.settings');
   

   not sure about policy, but better to inject this services too

1, yes
2, no need

Fixed watchdog usage.

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -18,6 +22,112 @@
   +        $reset_link_user = $this->userStorage->load($uid);
   +        if ($reset_link_user) {
   

   We can make this one statement.

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -18,6 +22,112 @@
   +        if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {
   ...
   +        elseif ($user->isAuthenticated() && $timestamp >= $user->getLastLoginTime() && $timestamp <= $current && $hash === user_pass_rehash($user->getPassword(), $timestamp, $user->getLastLoginTime())) {
   

   Could we wrap each bool statement in ()? Just for sanity. No one will ever touch this code again so this is the only chance to make it more readable.

Sure.

Thanks for the fixes.

Committed f475171 and pushed to 8.x. Thanks!

+++ b/core/modules/user/src/Controller/UserController.php
@@ -18,6 +22,111 @@
+          return $this->formBuilder()->getForm('Drupal\user\Form\UserPasswordResetForm', $user, $expiration_date, $timestamp, $hash);

Isn't that a bit hacky? Shouldn't we redirect instead in this case?