Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#80 | user-1987896-80.patch | 1.95 KB | tim.plunkett |
#76 | user-1987896-76.patch | 27.68 KB | tim.plunkett |
#76 | interdiff.txt | 511 bytes | tim.plunkett |
#74 | user-1987896-74.patch | 27.61 KB | tim.plunkett |
#74 | interdiff.txt | 802 bytes | tim.plunkett |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedComment #2
marcingy CreditAttribution: marcingy commentedComment #3
vijaycs85Initial patch...
Comment #5
vijaycs85Failures are partially because of #1981644: Figure out how to deal with 'title/title callback'
Comment #6
kgoel CreditAttribution: kgoel commentedComment #7
kgoel CreditAttribution: kgoel commentedPatch added.
Comment #9
kgoel CreditAttribution: kgoel commentedTalked with timPlunkett and daweher, adding /login parameter in router which will solve this issue.
Comment #11
kgoel CreditAttribution: kgoel commentedFixed router name in hook_menu (user.module)
Comment #13
tim.plunkettThis 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
Comment #15
attiks CreditAttribution: attiks commented#13: user-page-1987896-13.patch queued for re-testing.
Comment #16
dawehnerHaven't we talked about redirect to either /user/login or /user/{uid}?
Comment #17
JulienD CreditAttribution: JulienD commentedHi
#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 ?
Comment #18
dawehnerI'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.
we should not get rid of the weight ...
Comment #19
kgoel CreditAttribution: kgoel commentedAdded weight back in the hook_menu.
Comment #20
dawehnerAs a follow up we could convert the user login form to an entity controller.
Comment #22
kgoel CreditAttribution: kgoel commented#19: user-page-1987896-19.patch queued for re-testing.
Comment #23
tim.plunkett#19: user-page-1987896-19.patch queued for re-testing.
Comment #25
kgoel CreditAttribution: kgoel commented#19: user-page-1987896-19.patch queued for re-testing.
Comment #27
tim.plunkettI'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.
Comment #29
tim.plunkettWhoops, forgot to pull first.
Comment #31
tim.plunkettThis 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().
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedlets fill this in, and well, fix the rest docs of this class
lets use the create() method here too instead
but overall i love the user_login_finalize cleanup, great stuff, thanks!
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedComment #35
tim.plunkettThanks! That last form part was the last bug too (forgot the request).
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedthey need the typical boring boring details
needs the boring as well
Constructs a ..
line
also visibikity
lets make those a bit better too
otherwise if bot agress its ready to go
Comment #37
tim.plunkettAdded the boilerplate.
Those docblocks are exactly whats in HEAD now, so I don't think it's a problem to leave it.
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedagreed, thanks!
Comment #39
dawehnerSo couldn't we just do a subrequest for /user/login?
Our HttpKernel has helper methods for that.
Comment #40
tim.plunkettTurns out we couldn't use a subrequest, but we can still use new requests without the redirect.
Many thanks to @dawehner for this fix.
Comment #42
tim.plunkett#40: user-1987896-40.patch queued for re-testing.
Comment #44
tim.plunkett@dawehner suggested this.
Comment #46
tim.plunkettUgh. 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.
Comment #48
tim.plunkettThat's it.
Comment #49
tim.plunkettThis needs #1668866: Replace drupal_goto() with RedirectResponse :(
But that's passing!
Comment #50
tim.plunkett#46: user-1987896-46.patch queued for re-testing.
Comment #52
tim.plunkettConflicted with the user_role_delete issue.
Comment #54
tim.plunkettTalking to Crell, I have no idea how to best resolve this.
Comment #55
tim.plunkettThis 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.
Comment #57
tim.plunkettFixed one bug.
Comment #58
Crell CreditAttribution: Crell commentedShouldn't these come from the generator? We're converting them to routes, so we can reference them as routes.
{@inheritdoc}
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.
Comment #60
tim.plunkettOkay, 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().
No idea how to do this.
I injected the DB connection, and fixed the docblocks (none of them are inheritdoc).
Comment #62
tim.plunkettUgh.
Comment #63
tim.plunkettSo it's this or #37. I prefer #62 but only barely.
Comment #64
dawehnerI 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
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, 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
Comment #66
tim.plunkettOkay, talked to @dawehner and @Crell, and they preferred the approach in #37.
Comment #68
dawehnerWe could inject the user storage
Make a follow up to convert this to a EQ
Comment #69
tim.plunkettWe don't want eq because we actually need the account.
Comment #70
Crell CreditAttribution: Crell commentedTo be tidy, make this class ContainerAware, then pass in $this->container.
Can these not be injected?
Aside from those nitpicks I think this is good.
Comment #71
tim.plunkettOokay
Comment #72
Crell CreditAttribution: Crell commentedWhew!
Comment #73
alexpottLooks great... minor nit.
So now the global $user is being set here I think this important fact needs to be reflected in it's documentation.
Comment #74
tim.plunkettFair point, it used to say "Must be called when logging in a user." which was confusing and easy to do wrong.
Comment #75
dawehnerI don't see where it describes that this replaced the global user ...
Comment #76
tim.plunkettFair enough.
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedahoy, thanks
Comment #78
alexpottCommitted 0b8c586 and pushed to 8.x. Thanks!
I think we need to change notice based on the changes to the user login process...
Comment #79
alexpottComment #80
tim.plunkettWhile attempting to write a change notice for this, I saw two leftover references to user_login_final_validate().
Comment #81
steveoliver CreditAttribution: steveoliver commentedgood catch.
Comment #82
catchCommitted/pushed to 8.x, thanks!
Comment #83
tim.plunkettComment #84
catch#2083415: [META] Write up all outstanding change notices before release
Comment #85
xjmThanks 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.
Comment #86
xjmTagging "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.
Comment #87
dawehnerThe 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
Comment #88
tim.plunkettPublished the change notice.
Comment #90
xjmActually, the filed change record doesn't seem to cover:
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.
Comment #91
dawehnerWell, 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.
Comment #92
xjmCame across this again; can anyone clarify/check:
From @alexpott in #78.
Comment #93
jhedstromI 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.
Comment #94
vijaycs85I 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.