[Change record update] Convert user_page() to a new style controller

Patch added.

Talked with timPlunkett and daweher, adding /login parameter in router which will solve this issue.

Fixed router name in hook_menu (user.module)

This is a victim of #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK. I think this needs to be exempt from that rule.

I cleaned up a couple bits. The bug was the missing route for user/login

#13: user-page-1987896-13.patch queued for re-testing.

Haven't we talked about redirect to either /user/login or /user/{uid}?

Hi

#13 works for me. The patch has been well applied and the controller is invoked.

What about #16? Should we add more stuff in this patch ?

I'm not really sure whether it is right to have both forms and normal forms on the same controller, but I don't have a better answer for that at the moment.

+++ b/core/modules/user/user.moduleundefined
@@ -863,10 +863,7 @@ function user_menu() {
-    'weight' => -10,

we should not get rid of the weight ...

Added weight back in the hook_menu.

As a follow up we could convert the user login form to an entity controller.

#19: user-page-1987896-19.patch queued for re-testing.

#19: user-page-1987896-19.patch queued for re-testing.

#19: user-page-1987896-19.patch queued for re-testing.

I'd rather not introduce a drupal_get_form call directly into the new controller.
And since it's not a true page callback, it's not even on the list for #1971384: [META] Convert page callbacks to controllers.

Whoops, forgot to pull first.

This is quite the issue.

Turns out we can't redirect when the user is anonymous, it breaks too many things.

However, I had to clean up user_login_finalize().

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,221 @@
+/**
+ * @todo.
+ */
+class UserLoginForm implements FormInterface, ControllerInterface {

lets fill this in, and well, fix the rest docs of this class

+++ b/core/modules/user/lib/Drupal/user/Plugin/Block/UserLoginBlock.phpundefined
@@ -33,7 +34,7 @@ public function access() {
+    $form = drupal_get_form(new UserLoginForm(\Drupal::service('config.factory'), \Drupal::service('flood')));

lets use the create() method here too instead

but overall i love the user_login_finalize cleanup, great stuff, thanks!

Thanks! That last form part was the last bug too (forgot the request).

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,221 @@
+  /**
+   * @var \Symfony\Component\HttpFoundation\Request
+   */
...
+  /**
+   * @var \Drupal\Core\Flood\FloodInterface
+   */

they need the typical boring boring details

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,221 @@
+  /**
+   * @param \Drupal\Core\Config\ConfigFactory $config_factory
+   *   The config factory.
+   * @param \Drupal\Core\Flood\FloodInterface $flood
+   *   The flood service.
+   */
+  function __construct(ConfigFactory $config_factory, FloodInterface $flood) {

needs the boring as well
Constructs a ..
line
also visibikity

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,221 @@
+  /**
+   * A FAPI validate handler. Sets an error if supplied username has been blocked.
+   */
+  public function validateName(array &$form, array &$form_state) {
...
+   * A validate handler on the login form. Check supplied username/password
+   * against local users table. If successful, $form_state['uid']
+   * is set to the matching user ID.
+   */
+  public function validateAuthentication(array &$form, array &$form_state) {
...
+  /**
+   * The final validation handler on the login form.
+   *
+   * Sets a form error if user has not been authenticated, or if too many
+   * logins have been attempted. This validation function should always
+   * be the last one.
+   */
+  public function validateFinal(array &$form, array &$form_state) {

lets make those a bit better too

otherwise if bot agress its ready to go

Added the boilerplate.
Those docblocks are exactly whats in HEAD now, so I don't think it's a problem to leave it.

agreed, thanks!

+++ b/core/modules/user/lib/Drupal/user/Controller/UserController.phpundefined
@@ -7,27 +7,37 @@
+    else {
+      $response = drupal_get_form(UserLoginForm::create(\Drupal::getContainer()), $request);
+    }

So couldn't we just do a subrequest for /user/login?

Our HttpKernel has helper methods for that.

Turns out we couldn't use a subrequest, but we can still use new requests without the redirect.

Many thanks to @dawehner for this fix.

#40: user-1987896-40.patch queued for re-testing.

@dawehner suggested this.

Ugh. I was looking at how D7 user_page() worked. Apparently during the originally WSCCI Routing merge, the entire behavior of this function was changed from a subrequest to a redirect.

See
http://drupalcode.org/project/drupal.git/commitdiff/ea8d2911
http://drupalcode.org/project/drupal.git/blobdiff/2922920..d93bbc9:/core...
http://drupalcode.org/project/drupal.git/commitdiff/26b46f8

If this doesn't work I'm reverting all of those tests to restore to D7 sanity.

That's it.

This needs #1668866: Replace drupal_goto() with RedirectResponse :(
But that's passing!

#46: user-1987896-46.patch queued for re-testing.

Conflicted with the user_role_delete issue.

Talking to Crell, I have no idea how to best resolve this.

This is a reroll of #44.
I'm going to restore D7's behavior. This means reverting some test coverage to the way it was before.

Fixed one bug.

+++ b/core/modules/user/lib/Drupal/user/Controller/UserController.php
@@ -18,16 +20,51 @@
+    $url = ($user->uid) ? '/user/' . $user->uid : '/user/login';

Shouldn't these come from the generator? We're converting them to routes, so we can reference them as routes.

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
@@ -0,0 +1,229 @@
+  /**
+   * A FAPI validate handler. Sets an error if supplied username has been blocked.
+   */

{@inheritdoc}

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
@@ -0,0 +1,229 @@
+      $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $form_state['values']['name']))->fetchObject();

I cry every time I see this in a form object. :-( I thought we fixed it?

This looks reasonable otherwise, assuming the testbot doesn't hate it.

Okay, screw it. The /user/$uid needs to be _controller but the /user/login really wants to be _form or _content. I either get inception-style pages or no themed markup.

So I'm just giving up, and putting the subrequest inside the form. This only works now because we changed forms to allow responses being returned mid-buildForm().

Shouldn't these come from the generator? We're converting them to routes, so we can reference them as routes.

No idea how to do this.

I injected the DB connection, and fixed the docblocks (none of them are inheritdoc).

Ugh.

So it's this or #37. I prefer #62 but only barely.

I prefer #37 because #62 would make it hard to reuse the form. It contains information which simply does not really belong there and so causes some relative random behavior

yeah, i prefer to #37 as well, having the kernel available to the form doesnt look good..also the destination check feels a bit wrong..should be in the controller..:/ sorry tim

Okay, talked to @dawehner and @Crell, and they preferred the approach in #37.

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,239 @@
+    $account = user_load($form_state['uid']);

We could inject the user storage

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.phpundefined
@@ -0,0 +1,239 @@
+      $account = $this->connection->query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $form_state['values']['name']))->fetchObject();

Make a follow up to convert this to a EQ

We don't want eq because we actually need the account.

+++ b/core/modules/user/lib/Drupal/user/Controller/UserController.php
@@ -7,27 +7,37 @@
+      $response = drupal_get_form(UserLoginForm::create(\Drupal::getContainer()), $request);

To be tidy, make this class ContainerAware, then pass in $this->container.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Block/UserLoginBlock.php
@@ -33,7 +34,7 @@ public function access() {
+    $form = drupal_get_form(UserLoginForm::create(\Drupal::getContainer()), \Drupal::request());

Can these not be injected?

Aside from those nitpicks I think this is good.

Ookay

Whew!

Looks great... minor nit.

+++ b/core/modules/user/user.moduleundefined
@@ -1355,13 +1211,14 @@ function user_authenticate($name, $password) {
-function user_login_finalize(&$edit = array()) {
+function user_login_finalize(AccountInterface $account) {
   global $user;
+  $user = $account;

So now the global $user is being set here I think this important fact needs to be reflected in it's documentation.

Fair point, it used to say "Must be called when logging in a user." which was confusing and easy to do wrong.

+++ b/core/modules/user/user.moduleundefined
@@ -1345,18 +1201,20 @@ function user_authenticate($name, $password) {
+ * Finalizes the login process and logs in a user.
...
+ * The function logs in the user, records a watchdog message about the new
+ * session, saves the login timestamp, calls hook_user_login(), and generates a
+ * new session.

I don't see where it describes that this replaced the global user ...

Fair enough.

ahoy, thanks

Committed 0b8c586 and pushed to 8.x. Thanks!

I think we need to change notice based on the changes to the user login process...

While attempting to write a change notice for this, I saw two leftover references to user_login_final_validate().

good catch.

Committed/pushed to 8.x, thanks!

#2083415: [META] Write up all outstanding change notices before release

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.

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on June 27, 2013, which means that a change record has been missing for more than six months.

Sounds like there's one self-contained change record needed here, and we can also stick another row in https://drupal.org/node/2119699.

The amount of time to write those change notice is not that much more than posting a comment about the need to write one

https://drupal.org/node/2185941

Published the change notice.

Actually, the filed change record doesn't seem to cover:

I think we need to change notice based on the changes to the user login process...

I don't know what that means, but presumably someone who understands this patch does.

The existing change record's contents can just be added to https://drupal.org/node/2119699.

Well, my point is that we already had a metric to determine which issue needs change notices ...
I did not wanted to be rude here, it is just odd how you have to spent your time.

Came across this again; can anyone clarify/check:

I think we need to change notice based on the changes to the user login process...

From @alexpott in #78.

I have updated the change notice with only change to the login process (not already mentioned) I could find:

user_login_finalize(&$edit = array()) => user_login_finalize(AccountInterface $account)

Issue is already at needs review.