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

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#80 user-1987896-80.patch1.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,851 pass(es).
[ View ]
#76 user-1987896-76.patch27.68 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es).
[ View ]
#76 interdiff.txt511 bytestim.plunkett
#74 user-1987896-74.patch27.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#74 interdiff.txt802 bytestim.plunkett
#71 user-1987896-71.patch27.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,055 pass(es).
[ View ]
#71 interdiff.txt3.87 KBtim.plunkett
#69 user-1987896-69.patch25.4 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,828 pass(es).
[ View ]
#69 interdiff.txt4.53 KBtim.plunkett
#66 user-page-198789664-64.patch25.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#62 interdiff.txt590 bytestim.plunkett
#62 user-1987896-62.patch27.45 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,104 pass(es).
[ View ]
#60 user-1987896-60.patch27.45 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#60 interdiff.txt7.9 KBtim.plunkett
#57 user-1987896-57.patch28.66 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,984 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#57 interdiff.txt1.35 KBtim.plunkett
#55 user-1987896-55.patch27.7 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,427 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt3.21 KBtim.plunkett
#52 user-1987896-52.patch24.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,208 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#46 user-1987896-46.patch24.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1987896-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#46 interdiff.txt913 bytestim.plunkett
#44 user-1987896-44.patch24.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,802 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#44 interdiff.txt936 bytestim.plunkett
#40 user-1987896-40.patch24.64 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#40 interdiff.txt2.88 KBtim.plunkett
#37 user-page-1987896-37.patch24.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,701 pass(es).
[ View ]
#37 interdiff.txt1.18 KBtim.plunkett
#35 user-page-1987896-35.patch24.03 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,695 pass(es).
[ View ]
#35 interdiff.txt1.22 KBtim.plunkett
#31 user-page-1987896-31.patch24.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,652 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
#31 interdiff.txt5.58 KBtim.plunkett
#29 user-1987896-29.patch20.74 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,647 pass(es), 24 fail(s), and 3 exception(s).
[ View ]
#27 user-1987896-27.patch20.79 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1987896-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 interdiff.txt19.6 KBtim.plunkett
#19 user-page-1987896-19.patch3.1 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-page-1987896-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 interdiff.txt590 byteskgoel
#13 user-page-1987896-13.patch3.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,136 pass(es).
[ View ]
#13 interdiff.txt2.41 KBtim.plunkett
#11 1987896-user-page-conversion-11.patch2.73 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#11 interdiff.txt608 byteskgoel
#9 1987896-user-page-conversion-9.patch2.73 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 interdiff.txt639 byteskgoel
#7 1987896-user-page-conversion-7.patch2.72 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 56,416 pass(es), 42 fail(s), and 0 exception(s).
[ View ]
#3 1987896-user-page-route-3.patch2.53 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,652 pass(es), 42 fail(s), and 0 exception(s).
[ View ]

Comments

Assigned:Unassigned» marcingy

Assigned:marcingy» Unassigned

Status:Active» Needs review
StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] 55,652 pass(es), 42 fail(s), and 0 exception(s).
[ View ]

Initial patch...

Status:Needs review» Needs work

Assigned:Unassigned» kgoel

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] 56,416 pass(es), 42 fail(s), and 0 exception(s).
[ View ]

Patch added.

StatusFileSize
new639 bytes
new2.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

StatusFileSize
new608 bytes
new2.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Fixed router name in hook_menu (user.module)

StatusFileSize
new2.41 KB
new3.1 KB
PASSED: [[SimpleTest]]: [MySQL] 56,136 pass(es).
[ View ]

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

StatusFileSize
new590 bytes
new3.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-page-1987896-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added weight back in the hook_menu.

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

Issue tags:-WSCCI-conversion

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

Issue tags:+WSCCI-conversion
StatusFileSize
new19.6 KB
new20.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1987896-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new20.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,647 pass(es), 24 fail(s), and 3 exception(s).
[ View ]

Whoops, forgot to pull first.

StatusFileSize
new5.58 KB
new24.02 KB
FAILED: [[SimpleTest]]: [MySQL] 57,652 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

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!

Assigned:kgoel» Unassigned
Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new24.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,695 pass(es).
[ View ]

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

StatusFileSize
new1.18 KB
new24.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,701 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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.

StatusFileSize
new2.88 KB
new24.64 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

Status:Reviewed & tested by the community» Needs review
Issue tags:-WSCCI-conversion

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

Issue tags:+WSCCI-conversion
StatusFileSize
new936 bytes
new24.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,802 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

@dawehner suggested this.

StatusFileSize
new913 bytes
new24.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1987896-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, user-1987896-46.patch, failed testing.

Assigned:Unassigned» tim.plunkett

That's it.

Status:Needs work» Postponed

Status:Postponed» Needs review
Issue tags:-WSCCI-conversion

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

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, user-1987896-46.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.61 KB
FAILED: [[SimpleTest]]: [MySQL] 58,208 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Conflicted with the user_role_delete issue.

Status:Needs review» Needs work

The last submitted patch, user-1987896-52.patch, failed testing.

Issue tags:+Stalking Crell

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

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
new27.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,427 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, user-1987896-55.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.35 KB
new28.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,984 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, user-1987896-57.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.9 KB
new27.45 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, user-1987896-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,104 pass(es).
[ View ]
new590 bytes

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

StatusFileSize
new25.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, user-page-198789664-64.patch, failed testing.

Status:Needs work» Needs review

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

StatusFileSize
new4.53 KB
new25.4 KB
PASSED: [[SimpleTest]]: [MySQL] 56,828 pass(es).
[ View ]

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.

StatusFileSize
new3.87 KB
new27.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,055 pass(es).
[ View ]

Ookay

Status:Needs review» Reviewed & tested by the community

Whew!

Status:Reviewed & tested by the community» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new802 bytes
new27.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new511 bytes
new27.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es).
[ View ]

Fair enough.

Status:Needs review» Reviewed & tested by the community

ahoy, thanks

Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

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

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

Title:Convert user_page() to a new style controllerChange notice: Convert user_page() to a new style controller
Issue tags:+Needs change record

Status:Active» Needs review
Issue tags:-Stalking Crell
StatusFileSize
new1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 56,851 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

good catch.

Status:Reviewed & tested by the community» Active

Committed/pushed to 8.x, thanks!

Assigned:tim.plunkett» Unassigned

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.

Issue summary:View changes
Issue tags:+Missing change record

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.

Status:Active» Fixed

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

Title:Change notice: Convert user_page() to a new style controllerConvert user_page() to a new style controller
Issue tags:-Missing change record

Published the change notice.

Status:Fixed» Needs review
Issue tags:+Needs change record

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.