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.

CommentFileSizeAuthor
#31 337820-user-logout-D7.patch6.67 KBDave Reid
#30 337820-user-logout-D7.patch6.67 KBDave Reid
#24 337820-user-logout-D7.patch5.57 KBDave Reid
#23 337820-user-logout-D7.patch5.57 KBDave Reid
#22 337820-user-logout-D7.patch5.69 KBDave Reid
#9 337820-user-logout-D7_0.patch4.98 KBDave Reid
#7 337820-user-logout-D7.patch4.98 KBDave Reid
#1 337820-user-logout-D7.patch4.16 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
FileSize
337820-user-logout-D7.patch4.16 KB

Patch ready for review.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Makes total sense, and thanks for cleaning up tests while you were at it. Waiting for results from the test bot to RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

+1 from me too.

lilou’s picture

lilou CreditAttribution: lilou commented
Category: feature » task
Status: Needs review » Reviewed & tested by the community

Look good and all tests passes.

chx’s picture

chx CreditAttribution: chx commented

Yeah this makes sense.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

We'll need an update function for any manually built menus that are pointing to this URL, no?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
337820-user-logout-D7.patch4.98 KB

Zer we go. Tested the upgrade and it worked.

Dries’s picture

Dries CreditAttribution: Dries commented

There should also be an update path for the logout change, not?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
337820-user-logout-D7_0.patch4.98 KB

Oh man...I completely blanked and put 'login' instead of 'logout'

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Looks great.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Yes...it failed because part of it was accidentally committed. :/

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Testbot came back okay, and manual testing of both the patch and upgrade path confirms this is ready to go.

Committed to HEAD. Thanks!

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

My fault. :( Reverted the hunks from the other issue. :(

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Fixed » Needs work

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Incidentally, I made a bug about this against PIFR over at #338292: Correct regex for singular assertions.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Wow...that result is weird. I'll look into this shortly.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review

Trying to resubmit the same patch.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
337820-user-logout-D7.patch5.69 KB

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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
337820-user-logout-D7.patch5.57 KB

Removed unnecessary line removal.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
337820-user-logout-D7.patch5.57 KB

Not sure how that last test passed...I forgot to change the 'user/login' to 'user/logout' again...

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Fixed » Active

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

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

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.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

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

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
FileSize
337820-user-logout-D7.patch6.67 KB

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

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
337820-user-logout-D7.patch6.67 KB

Reposting to get picked up by testing bot.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Confirmed that tests pass with this new version. The patch itself looks good. RTBC, one last time.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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