Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Assigned: Unassigned » chertzog
Status: Active » Needs review
FileSize
2.67 KB

I think i got all of them.

Status: Needs review » Needs work

The last submitted patch, 1999448_1-replace-raw-variables-user.patch, failed testing.

chertzog’s picture

Status: Needs work » Needs review
FileSize
2.77 KB

fixes

kim.pepper’s picture

Status: Needs review » Needs work

Looking good. Needs a bit of clean up.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -68,7 +68,8 @@ public function form(array $form, array &$form_state) {
+      $request = \Drupal::request()->request->get('pass-reset-token');

Should probably call this $token or $reset_token instead of $request

+++ b/core/modules/user/user.admin.incundefined
@@ -21,7 +21,8 @@
+  $request = Drupal::request()->request->get('op');

Again, variable name $request doesn't quite match.

+++ b/core/modules/user/user.admin.incundefined
@@ -21,7 +21,8 @@
+  $op = !empty($request) ? $request : $callback_arg;

You turn this into an if statement and it would be more readable.

+++ b/core/modules/user/user.admin.incundefined
@@ -30,7 +31,9 @@ function user_admin($callback_arg = '') {
+      $accounts = Drupal::request()->request->get('accounts');
+      $operation = Drupal::request()->request->get('operation');

Extract the $request variable here to make it less verbose.

+++ b/core/modules/user/user.pages.incundefined
@@ -203,9 +203,10 @@ function template_preprocess_user(&$variables) {
+  $request = Drupal::request()->query->get('destination');

The variable should probably be called $destination

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

attempt a reroll.

chrisjlee’s picture

...

Status: Needs review » Needs work

The last submitted patch, reroll-1999448-3.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

Reroll this one Go testbot go !

Status: Needs review » Needs work

The last submitted patch, 1999448-8-replace-raw-variables-user.patch, failed testing.

aaronott’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
1.68 KB

Another re-roll with small changes such as using ->has() instead of isset().

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -70,7 +70,8 @@ public function form(array $form, array &$form_state) {
+      $pass_reset = isset($_SESSION['pass_reset_' . $account->uid]) && $query->has('pass-reset-token') && ($query->get('pass-reset-token') == $_SESSION['pass_reset_' . $account->uid]);

We don't need the ->has() here... ->get() will return NULL. Due to the isset($_SESSION['pass_reset_' . $account->uid]) the value we test against can never be NULL

aaronott’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
964 bytes

Good point!

I've removed the ->has() from AccountFormController.

Anonymous’s picture

looks good to me.I think it should go to RTBC +1

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch, 1999448-13-replace-raw-variables-user.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
954 bytes
1.63 KB

Removed an unnecessary $query variable.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks perfect!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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