Rename menu path 'logout' to 'user/logout' for consistency
Dave Reid - November 22, 2008 - 18:54
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed |
Description
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.
#1
Patch ready for review.
#2
Makes total sense, and thanks for cleaning up tests while you were at it. Waiting for results from the test bot to RTBC.
#3
+1 from me too.
#4
Look good and all tests passes.
#5
Yeah this makes sense.
#6
We'll need an update function for any manually built menus that are pointing to this URL, no?
#7
Zer we go. Tested the upgrade and it worked.
#8
There should also be an update path for the logout change, not?
#9
Oh man...I completely blanked and put 'login' instead of 'logout'
#10
Looks great.
#11
I 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...
#12
The last submitted patch failed testing.
#13
Yes...it failed because part of it was accidentally committed. :/
#14
Ok, I did:
cvs update -j 1.283 -j 1.282 modules/system/system.install(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.
#15
Testbot came back okay, and manual testing of both the patch and upgrade path confirms this is ready to go.
Committed to HEAD. Thanks!
#16
I 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...
#17
My fault. :( Reverted the hunks from the other issue. :(
#18
Hm. 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...
#19
Incidentally, I made a bug about this against PIFR over at #338292: Correct regex for singular assertions.
#20
Wow...that result is weird. I'll look into this shortly.
#21
Trying to resubmit the same patch.
#22
I 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.
#23
Removed unnecessary line removal.
#24
Not sure how that last test passed...I forgot to change the 'user/login' to 'user/logout' again...
#25
Committed to CVS HEAD. Thanks.
#26
Please 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).
#27
I 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.
#28
Hum. 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.
#29
Ok, 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. ;)
#30
Ok 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 = 0to 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.
#31
Reposting to get picked up by testing bot.
#32
Confirmed that tests pass with this new version. The patch itself looks good. RTBC, one last time.
#33
Committed to CVS HEAD. Thanks!
--project followup subject--
Automatically closed -- issue fixed for two weeks with no activity.
#34
Automatically closed -- issue fixed for two weeks with no activity.