Part of #1971384: [META] Convert page callbacks to controllers

Convert this page callback to a new-style Form object, using the instructions on http://drupal.org/node/1800686

Files: 
CommentFileSizeAuthor
#72 user-reset-1998198-72.patch17.54 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,490 pass(es).
[ View ]
#72 interdiff.txt1.91 KBtim.plunkett

Comments

Albert Volkman’s picture

Status:Active» Needs review
StatusFileSize
new11.4 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/UserPasswordController.php.
[ View ]

First pass.

tim.plunkett’s picture

+++ 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

Status:Needs review» Needs work

The last submitted patch, 1998198-user_pass_reset_conversion-1.patch, failed testing.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new5.75 KB
new12.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,754 pass(es), 28 fail(s), and 3 exception(s).
[ View ]

Addresses issues raised in #2 (I think).

Status:Needs review» Needs work

The last submitted patch, 1998198-user_pass_reset_conversion-4.patch, failed testing.

dutchyoda’s picture

Assigned:Unassigned» dutchyoda
splatio’s picture

StatusFileSize
new13.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,124 pass(es), 11 fail(s), and 1 exception(s).
[ View ]

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.

fubhy’s picture

Status:Needs work» Needs review

Go bot.

fubhy’s picture

Status:Needs review» Needs work
+++ 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.

splatio’s picture

Assigned:dutchyoda» splatio

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

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new14.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998198-user-pass-reset-conversion-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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!

Status:Needs review» Needs work

The last submitted patch, 1998198-user-pass-reset-conversion-11.patch, failed testing.

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new16.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]

Let's try that again!

dawehner’s picture

Status:Needs review» Needs work
+++ 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.

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new14.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,895 pass(es).
[ View ]

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.

dawehner’s picture

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.

dawehner’s picture

Status:Needs review» Needs work
splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
new15.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,557 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, 1998198-user-pass-reset-conversion-17.patch, failed testing.

splatio’s picture

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion
dawehner’s picture

Status:Needs review» Needs work

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

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new10.74 KB
new14.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,523 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, 1998198-user-pass-reset-conversion-21.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion
jibran’s picture

Status:Needs review» Needs work
  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.

xjm’s picture

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.

disasm’s picture

+++ 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.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new14.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,884 pass(es), 19 fail(s), and 7 exception(s).
[ View ]

@jibran:

  1. Which docblock needs an update?
  2. Yes, it's part of the interface.
  3. Added.

@disasm:

Done!

tim.plunkett’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 1998198-user-pass-reset-conversion-28.patch, failed testing.

jibran’s picture

+++ 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’s picture

Status:Needs work» Needs review
StatusFileSize
new15.56 KB
FAILED: [[SimpleTest]]: [MySQL] 59,064 pass(es), 13 fail(s), and 1 exception(s).
[ View ]

So, something like this?

Status:Needs review» Needs work

The last submitted patch, 1998198-user-pass-reset-conversion-32.patch, failed testing.

disasm’s picture

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

disasm’s picture

Status:Needs work» Needs review
Issue tags:+needs security review
StatusFileSize
new12.44 KB
new15.85 KB
FAILED: [[SimpleTest]]: [MySQL] 58,747 pass(es), 4 fail(s), and 8 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.user-module.1998198-35.patch, failed testing.

jibran’s picture

+++ 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.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
new15.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,095 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

fixes ConfigFactory type hint.

Status:Needs review» Needs work

The last submitted patch, drupal8.user-module.1998198-37.patch, failed testing.

willieseabrook’s picture

Assigned:splatio» willieseabrook

Proceeding as part of drupal mentoring prague

willieseabrook’s picture

Assigned:willieseabrook» Unassigned

Didnt get time to make much progress

willieseabrook’s picture

Issue summary:View changes

Linking back to meta issue

pwolanin’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new16.45 KB
FAILED: [[SimpleTest]]: [MySQL] 60,170 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

tim.plunkett’s picture

  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

Status:Needs review» Needs work

The last submitted patch, 42: user-pass-reset-1998198-42.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new14.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,339 pass(es).
[ View ]
new17.16 KB

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).

pwolanin’s picture

StatusFileSize
new15.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,335 pass(es).
[ View ]
new2.8 KB

A little more cleanup of the form builder parameters and logic

dawehner’s picture

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

So how did it worked before?

pwolanin’s picture

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

pwolanin’s picture

StatusFileSize
new14.93 KB
PASSED: [[SimpleTest]]: [MySQL] 59,503 pass(es).
[ View ]
new817 bytes

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.

pwolanin’s picture

StatusFileSize
new14.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,534 pass(es).
[ View ]
new1.64 KB

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

dawehner’s picture

  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.

pwolanin’s picture

Assigned:Unassigned» pwolanin
StatusFileSize
new6.51 KB
new17.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,307 pass(es).
[ View ]

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

Les Lim’s picture

StatusFileSize
new16.86 KB
PASSED: [[SimpleTest]]: [MySQL] 63,197 pass(es).
[ View ]
new2.99 KB

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.

Les Lim’s picture

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.

andypost’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new16.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,389 pass(es), 11 fail(s), and 5 exception(s).
[ View ]

Simple re-roll, patch looks great now!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 55: core-1998198-55-user-pass-reset.patch, failed testing.

andypost’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.08 KB
new1.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-1998198-57-user-pass-reset.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Missed in re-roll changed crypt

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 57: core-1998198-57-user-pass-reset.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new16.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,490 pass(es).
[ View ]

proper patch

tim.plunkett’s picture

StatusFileSize
new16.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,557 pass(es), 23 fail(s), and 7 exception(s).
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, 60: user-pass-1998198-60.patch, failed testing.

tkuldeep17’s picture

Status:Needs work» Needs review
StatusFileSize
new16.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch user-pass-1998198-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled patch..

tkuldeep17’s picture

@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 ?

xjm’s picture

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

Status:Needs review» Needs work

The last submitted patch, 62: user-pass-1998198-62.patch, failed testing.

xjm’s picture

Issue tags:+Needs reroll
Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new15.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,729 pass(es).
[ View ]

PSR-4 re-roll.

rodrigoaguilera’s picture

Issue tags:-Needs reroll
andypost’s picture

  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

tim.plunkett’s picture

Issue tags:-needs security review
StatusFileSize
new17.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,556 pass(es).
[ View ]
new3.86 KB

1, yes
2, no need

Fixed watchdog usage.

jibran’s picture

  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.

tim.plunkett’s picture

StatusFileSize
new1.91 KB
new17.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,490 pass(es).
[ View ]

Sure.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the fixes.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed f475171 and pushed to 8.x. Thanks!

  • alexpott committed f475171 on 8.x
    Issue #1998198 by pwolanin, splatio, Albert Volkman, tim.plunkett,...
damiankloip’s picture

+++ 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?

Status:Fixed» Closed (fixed)

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