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.
Is there a reason we don't use "user/logout" for the logout path? I couldn't find any other issues and it seems to make sense for consistency to use "user/logout" since we already have user/login, user/register, user/password.
Comment | File | Size | Author |
---|---|---|---|
#31 | 337820-user-logout-D7.patch | 6.67 KB | Dave Reid |
#30 | 337820-user-logout-D7.patch | 6.67 KB | Dave Reid |
#24 | 337820-user-logout-D7.patch | 5.57 KB | Dave Reid |
#23 | 337820-user-logout-D7.patch | 5.57 KB | Dave Reid |
#22 | 337820-user-logout-D7.patch | 5.69 KB | Dave Reid |
Comments
Comment #1
Dave ReidPatch ready for review.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedMakes total sense, and thanks for cleaning up tests while you were at it. Waiting for results from the test bot to RTBC.
Comment #3
catch+1 from me too.
Comment #4
lilou CreditAttribution: lilou commentedLook good and all tests passes.
Comment #5
chx CreditAttribution: chx commentedYeah this makes sense.
Comment #6
webchickWe'll need an update function for any manually built menus that are pointing to this URL, no?
Comment #7
Dave ReidZer we go. Tested the upgrade and it worked.
Comment #8
Dries CreditAttribution: Dries commentedThere should also be an update path for the logout change, not?
Comment #9
Dave ReidOh man...I completely blanked and put 'login' instead of 'logout'
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks great.
Comment #11
Dave ReidI think part of this patch was accidentally committed.
http://drupal.org/cvs?commit=154623
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.inst...
Comment #13
Dave ReidYes...it failed because part of it was accidentally committed. :/
Comment #14
webchickOk, I did:
(hat tip, dww!)
And then committed so the conflicts should be gone now. Thanks for the heads-up. Marked the patch back for re-testing.
Comment #15
webchickTestbot came back okay, and manual testing of both the patch and upgrade path confirms this is ready to go.
Committed to HEAD. Thanks!
Comment #16
Dave ReidI think there was even more extras that were committed with this. :)
http://drupal.org/cvs?commit=154659
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.admi...
http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/form.inc?r1=1.305...
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.modu...
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/upload/upload.modu...
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/user/user.pages.in...
Looks like stuff from #137932: Automatic enctype on adding a file field...
Comment #17
webchickMy fault. :( Reverted the hunks from the other issue. :(
Comment #18
webchickHm. Well this is interesting.
Justin Randell noticed tonight that the Menu item creation/deletion tests had a failure in them this evening. So I spent a few minutes rolling back patches until it stopped failing. It turns out, this patch causes it (no idea why).
The failure is:
"Menu item was reset" in menu.test line 317 (MenuTestCase->resetMenuItem())
What sucks though is as you can plainly see in #14 and #15, I marked the patch back for testing and it came through all greens http://drupal.org/node/337820#comment-1122283. :( I marked it for retesting at :30, it passed at :57, and it was committed at :18...
So... I guess something is causing the testbot to not catch this error. :(
At any rate, we need to figure out why that test fails with this patch applied, so I've reverted it for now to get tests passing again...
Comment #19
webchickIncidentally, I made a bug about this against PIFR over at #338292: Correct regex for singular assertions.
Comment #20
Dave ReidWow...that result is weird. I'll look into this shortly.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedTrying to resubmit the same patch.
Comment #22
Dave ReidI cannot duplicate that error at all... So I'll post a new patch that includes a necessary change to robots.txt and the update function since there is now an existing system_update_7014.
Comment #23
Dave ReidRemoved unnecessary line removal.
Comment #24
Dave ReidNot sure how that last test passed...I forgot to change the 'user/login' to 'user/logout' again...
Comment #25
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease revert. The actual failure that made webchick revert that in the first place is still not fixed. The test bot is "near-sighted" on some tests, and fails to report back failures in some cases (investigation in progress in #338292: Correct regex for singular assertions).
Comment #27
Dave ReidI still have no idea why this patch would cause the menu item/creation test to fail. I need to do some debugging into the test to figure out what's going on.
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedHum. I *made* the debugging, but somehow failed to post the results back here.
The issue is that getStandardMenuItem() picks a menu item that is not displayed on the front page (Help in my case). That happens consistently on all my tests (upgrading an existing 7.x installation, or starting from scratch). You will have to fix that function, for example by making it pick Logout explicitly.
Comment #29
webchickOk, reverted this once more, and started the testing bot again. WHEW.
Let's leave this 'active' until we fix whatever the problem is with the test/bot/whatever, so we don't accidentally commit it prematurely any more. ;)
Comment #30
Dave ReidOk I fixed the error in the getStandardMenuItem(). As Damien said in #28, before this patch, the function uses
SELECT MIN(mlid) FROM {menu_links} WHERE module = 'system' AND hidden = 0 AND has_children = 0
to get the mlid of a front page menu item, assumed the logout item since before the patch, it has an mlid of 4. With the patch, the logout path is no longer has the minimum mlid (17) that meets those conditions, it's the admin/help menu item (mlid = 16), which does NOT display on the front page. So when the test performs $this->drupalGet('') it cannot find that menu item on the front page.Revised patch changes the SQL logic to explicitly find the mlid of only the logout path:
SELECT mlid FROM {menu_links} WHERE module = 'system' AND router_path = 'user/logout'
The menu item/creation test passes on my install and is ready for review.
Comment #31
Dave ReidReposting to get picked up by testing bot.
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedConfirmed that tests pass with this new version. The patch itself looks good. RTBC, one last time.
Comment #33
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!