Part of #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

Files that need converting are:

  • core/modules/user/lib/Drupal/user/AccountFormController.php
  • core/modules/user/user.admin.inc
  • core/modules/user/user.pages.inc
CommentFileSizeAuthor
#17 1999448-user-request-17.patch1.63 KBkim.pepper
#17 interdiff.txt954 byteskim.pepper
#13 1999448-interdiff-13.txt964 bytesaaronott
#13 1999448-13-replace-raw-variables-user.patch1.65 KBaaronott
#10 1999448-10-replace-raw-variables-user.patch1.68 KBaaronott
#10 1999448-interdiff.txt1.83 KBaaronott
#8 1999448-8-replace-raw-variables-user.patch1.64 KBAnonymous (not verified)
#5 reroll-1999448-3.patch1.68 KBchrisjlee
#3 1999448_3-replace-raw-variables-user.patch2.77 KBchertzog
#1 1999448_1-replace-raw-variables-user.patch2.67 KBchertzog

Comments

chertzog’s picture

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
commented
Assigned: Unassigned » chertzog
Status: Active » Needs review
StatusFileSize
new 1999448_1-replace-raw-variables-user.patch2.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

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 1999448_3-replace-raw-variables-user.patch2.77 KB

fixes

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
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

chrisjlee commented
Status: Needs work » Needs review
StatusFileSize
new reroll-1999448-3.patch1.68 KB

attempt a reroll.

chrisjlee’s picture

chrisjlee commented

...

Status: Needs review » Needs work

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

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 1999448-8-replace-raw-variables-user.patch1.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

aaronott commented
Status: Needs work » Needs review
StatusFileSize
new 1999448-interdiff.txt1.83 KB
new 1999448-10-replace-raw-variables-user.patch1.68 KB

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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

aaronott commented
Status: Needs work » Needs review
StatusFileSize
new 1999448-13-replace-raw-variables-user.patch1.65 KB
new 1999448-interdiff-13.txt964 bytes

Good point!

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

Anonymous’s picture

Anonymous (not verified) commented

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

Anonymous’s picture

Anonymous (not verified) commented

#13: 1999448-13-replace-raw-variables-user.patch queued for re-testing.

Status: Needs review » Needs work

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt954 bytes
new 1999448-user-request-17.patch1.63 KB

Removed an unnecessary $query variable.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Looks perfect!

dries’s picture

dries commented
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.