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.

CommentFileSizeAuthor
#80 user-1987896-80.patch1.95 KBtim.plunkett
#76 user-1987896-76.patch27.68 KBtim.plunkett
#76 interdiff.txt511 bytestim.plunkett
#74 user-1987896-74.patch27.61 KBtim.plunkett
#74 interdiff.txt802 bytestim.plunkett
#71 user-1987896-71.patch27.3 KBtim.plunkett
#71 interdiff.txt3.87 KBtim.plunkett
#69 user-1987896-69.patch25.4 KBtim.plunkett
#69 interdiff.txt4.53 KBtim.plunkett
#66 user-page-198789664-64.patch25.07 KBtim.plunkett
#62 interdiff.txt590 bytestim.plunkett
#62 user-1987896-62.patch27.45 KBtim.plunkett
#60 user-1987896-60.patch27.45 KBtim.plunkett
#60 interdiff.txt7.9 KBtim.plunkett
#57 user-1987896-57.patch28.66 KBtim.plunkett
#57 interdiff.txt1.35 KBtim.plunkett
#55 user-1987896-55.patch27.7 KBtim.plunkett
#55 interdiff.txt3.21 KBtim.plunkett
#52 user-1987896-52.patch24.61 KBtim.plunkett
#46 user-1987896-46.patch24.62 KBtim.plunkett
#46 interdiff.txt913 bytestim.plunkett
#44 user-1987896-44.patch24.51 KBtim.plunkett
#44 interdiff.txt936 bytestim.plunkett
#40 user-1987896-40.patch24.64 KBtim.plunkett
#40 interdiff.txt2.88 KBtim.plunkett
#37 user-page-1987896-37.patch24.17 KBtim.plunkett
#37 interdiff.txt1.18 KBtim.plunkett
#35 user-page-1987896-35.patch24.03 KBtim.plunkett
#35 interdiff.txt1.22 KBtim.plunkett
#31 user-page-1987896-31.patch24.02 KBtim.plunkett
#31 interdiff.txt5.58 KBtim.plunkett
#29 user-1987896-29.patch20.74 KBtim.plunkett
#27 user-1987896-27.patch20.79 KBtim.plunkett
#27 interdiff.txt19.6 KBtim.plunkett
#19 user-page-1987896-19.patch3.1 KBkgoel
#19 interdiff.txt590 byteskgoel
#13 user-page-1987896-13.patch3.1 KBtim.plunkett
#13 interdiff.txt2.41 KBtim.plunkett
#11 1987896-user-page-conversion-11.patch2.73 KBkgoel
#11 interdiff.txt608 byteskgoel
#9 1987896-user-page-conversion-9.patch2.73 KBkgoel
#9 interdiff.txt639 byteskgoel
#7 1987896-user-page-conversion-7.patch2.72 KBkgoel
#3 1987896-user-page-route-3.patch2.53 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Assigned: Unassigned » marcingy
marcingy’s picture

Assigned: marcingy » Unassigned
vijaycs85’s picture

Status: Active » Needs review
FileSize
2.53 KB

Initial patch...

vijaycs85’s picture

Status: Needs review » Needs work
kgoel’s picture

Assigned: Unassigned » kgoel
kgoel’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Patch added.

kgoel’s picture

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

kgoel’s picture

Fixed router name in hook_menu (user.module)

tim.plunkett’s picture

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

attiks’s picture

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

dawehner’s picture

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

JulienD’s picture

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 ?

dawehner’s picture

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

kgoel’s picture

FileSize
590 bytes
3.1 KB

Added weight back in the hook_menu.

dawehner’s picture

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

kgoel’s picture

Issue tags: -WSCCI-conversion

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

tim.plunkett’s picture

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

kgoel’s picture

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

tim.plunkett’s picture

Issue tags: +WSCCI-conversion
FileSize
19.6 KB
20.79 KB

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.

tim.plunkett’s picture

FileSize
20.74 KB

Whoops, forgot to pull first.

tim.plunkett’s picture

FileSize
5.58 KB
24.02 KB

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Assigned: kgoel » Unassigned
Status: Needs review » Needs work
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
24.03 KB

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

ParisLiakos’s picture

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

tim.plunkett’s picture

FileSize
1.18 KB
24.17 KB

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

agreed, thanks!

dawehner’s picture

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

tim.plunkett’s picture

FileSize
2.88 KB
24.64 KB

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

Issue tags: +WSCCI-conversion
FileSize
936 bytes
24.51 KB

@dawehner suggested this.

tim.plunkett’s picture

FileSize
913 bytes
24.62 KB

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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

That's it.

tim.plunkett’s picture

Status: Needs work » Postponed
tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
24.61 KB

Conflicted with the user_role_delete issue.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Issue tags: +Stalking Crell

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
27.7 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
28.66 KB

Fixed one bug.

Crell’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
27.45 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
27.45 KB
590 bytes

Ugh.

tim.plunkett’s picture

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

dawehner’s picture

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

ParisLiakos’s picture

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

tim.plunkett’s picture

FileSize
25.07 KB

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.

dawehner’s picture

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

tim.plunkett’s picture

FileSize
4.53 KB
25.4 KB

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

Crell’s picture

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

tim.plunkett’s picture

FileSize
3.87 KB
27.3 KB

Ookay

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Whew!

alexpott’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
802 bytes
27.61 KB

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

dawehner’s picture

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

tim.plunkett’s picture

FileSize
511 bytes
27.68 KB

Fair enough.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

ahoy, thanks

alexpott’s picture

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

alexpott’s picture

Title: Convert user_page() to a new style controller » Change notice: Convert user_page() to a new style controller
Issue tags: +Needs change record
tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Stalking Crell
FileSize
1.95 KB

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

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community

good catch.

catch’s picture

Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
catch’s picture

xjm’s picture

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.

xjm’s picture

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.

dawehner’s picture

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

tim.plunkett’s picture

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

Published the change notice.

xjm’s picture

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.

dawehner’s picture

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.

xjm’s picture

Title: Convert user_page() to a new style controller » [Change record update] Convert user_page() to a new style controller
Issue tags: -Needs change record +Needs change record updates

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.

jhedstrom’s picture

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.

vijaycs85’s picture

Status: Needs review » Fixed
Issue tags: -Needs change record updates

I have added few more functions to the CR and IMHO, we have covered all the changes we did in this issue. We might need to add some code block for D7 vs D8, but I don't think it is strictly part of this issue.

Status: Fixed » Closed (fixed)

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