Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Albert Volkman’s picture

Tagging

Albert Volkman’s picture

This is my first conversion, please be gentle :) I haven't converted the validate or submit functions yet.

Albert Volkman’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-1.patch, failed testing.

dawehner’s picture

Thanks for helping on all that conversion issues.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,66 @@
+ * Definition of Drupal\user\UserPasswordController.

The current standard for @file is "Contains \...". just have a look at http://drupal.org/node/1354 for general informationl.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,66 @@
+use Drupal\Core\Entity\EntityFormController;
...
+class UserPasswordController extends EntityFormController {

he, this controller is another controller :) Just extend no class by default. This works as well.

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

all that nitpicking is just to make it as perfect as possible. ... there should be an empty line after the first {

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,66 @@
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a PasswordController object.
+   */
+  public function __construct() {

If you have nothing injected into the class, there is no reason to put that code here.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,66 @@
+  public function passwordUser(Request $request) {

This method should be documented. In general: It's really great that you can just get the request object very easy here.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,66 @@
+  ¶
...
+  ¶

empty spaces.

+++ b/core/modules/user/user.routing.ymlundefined
@@ -25,3 +25,10 @@ user_account_settings:
+    _controller: '\Drupal\user\UserPasswordController::passwordUser'

Just wondering whether it makes more sense in a general UserController.

Feel free to ping me on IRC if you need help with fixing this failures.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
4.52 KB

Second try.

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-6.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.49 KB

Erm, let's try that again.

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-8.patch, failed testing.

dawehner’s picture

This looks great so far, beside of the failing tests ^^

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,62 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();

If you don't implement the ControllerInterface you also don't need the create method.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordController.phpundefined
@@ -0,0 +1,62 @@
+   * @param Request $request

We use the full namespace on documentation.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
7.54 KB
7.43 KB

Since this is returning a form, I think I was approaching this incorrectly. Consider this take 2 :)

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-11.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
697 bytes
7.44 KB

Added the Request object \Drupal::request(). Although, I'm assuming that's a hard dependency which shouldn't be added that way.

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-13.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Re-roll.

Albert Volkman’s picture

Status: Needs review » Needs work

The last submitted patch, 1984610-user_pass_conversion-16.patch, failed testing.

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/UserPasswordForm.phpundefined
@@ -0,0 +1,102 @@
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().

You could implement {@inheritdoc} ... and nothing else, because this is partially incorrect.

+++ b/core/modules/user/lib/Drupal/user/UserPasswordForm.phpundefined
@@ -0,0 +1,102 @@
+      $form['name']['#default_value'] = \Drupal::request()->query->get('name');

Have you tried to inject the request into the form?

+++ b/core/modules/user/lib/Drupal/user/UserPasswordForm.phpundefined
@@ -0,0 +1,102 @@
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

{@inheritdoc}

+++ b/core/modules/user/lib/Drupal/user/UserPasswordForm.phpundefined
@@ -0,0 +1,102 @@
+    $users = entity_load_multiple_by_properties('user', array('mail' => $name, 'status' => '1'));
...
+      $users = entity_load_multiple_by_properties('user', array('name' => $name, 'status' => '1'));

What about injecting the user storage controller as well?

dawehner’s picture

Assigned: Unassigned » dawehner

Assigning to myself to inject the request into the form controller method.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
5.74 KB
10.52 KB

I think the proper way would be to use the controller resolver to pass in values instead of using custom reflection code.

Sadly this requires some hacks, but maybe we could get this fixed by extend our controller resolver.

ParisLiakos’s picture

+++ b/core/lib/Drupal/Core/Controller/HtmlFormController.phpundefined
@@ -80,7 +81,7 @@ public function content(Request $request, $_form) {
-      if (in_array('Drupal\Core\ControllerInterface', class_implements($form_arg))) {
+      if (in_array('Drupal\Core\Controller\ControllerInterface', class_implements($form_arg))) {

i cant believe how this forgotten piece havent bite us yet?

Besides the hacks, that i am not sure i can think of a better alternative, this should maybe _entity_form like i said to dawehner in irc

Status: Needs review » Needs work

The last submitted patch, drupal-1984610-20.patch, failed testing.

dawehner’s picture

Added an issue to remove ControllerInterface: #1998140: Remove backward compatible ControllerInterface

i cant believe how this forgotten piece havent bite us yet?

There is a BC-Interface for that ...

dawehner’s picture

ParisLiakos’s picture

Status: Needs work » Postponed

ah..now i found out that crell kept a bc interface, so no conversions break during that
postponing this issue per #24

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
10.11 KB

Was too out of date for an interdiff.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

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