New User Logout confirm Form is misleading

Prashant.c → made their first commit to this issue’s fork.

I agree with this, this message is kind of irrelevant here because we already have in bolds Are you sure you want to log out? and this is not something that cannot be undone, the user can always log in again.

I would say a better thing would be to just not have this description at all.

Prashant.c → changed the visibility of the branch 3469116-new-user-logout to hidden.

In the original issue 🐛 User logout is vulnerable to CSRF Fixed also the description is not shown https://www.drupal.org/files/issues/2024-03-26/Screenshot%202024-03-26%2... → .

Tests are failing because the getDescription() {} method is expecting a translatable string, which in case we have to provide. Looking for input here.

This will need a title update as just the word "misleading" is vague and not sure what the problem is.

Probably should have test coverage as well.

Before adding tests we need to decide on what would be the "description" for this as requested in #8 🐛 New User Logout confirm Form is misleading Needs work

sahana _n → made their first commit to this issue’s fork.

Hi Prashant.c

I created an MR Please review it.
If there are any inaccuracies, I would greatly appreciate your suggestions for improvement.

Thank you!!

@sahana _n

Thanks for your efforts but in the existing MR we are already using the getDescription() {}new MR was not required. Please see #8 🐛 New User Logout confirm Form is misleading Needs work

prashant.c → changed the visibility of the branch 3469116-Logout-confirmation-form-shows-inappropriate-confirmation-description to hidden.

@smustgrave The Title of the issue has already been updated removing the tag. Please re-add if you still see some issues with the current title.

TBD. Suggesting a couple of descriptions we can use:

• Logging out will end your current session, and you will need to log in again to continue.
• Once you log out, you’ll need to sign in again to access your account.

I like the second option, for brevity.

Hi,

Even I liked the second option and I updated the MR Please review.
I verified that in Drupal version 11.x.

I would greatly appreciate your suggestions for improvement. Please let me know.

Thank you!!

Just wanted to confirm this one because as noted above, this text wasn't there when the change was committed. Added a screenshot to the IS.

I also agree that it's better without a description, I think at this point logging out is pretty self explanatory!

@pameeela wrong screenshot attached :). Without description is good but see #8 🐛 New User Logout confirm Form is misleading Needs work , we have to provide the description.

Oops! Fixed.

I realise that the tests failed without a description, but that doesn't mean it's not possible :)

I guess it is better to update it to something more accurate for now than to have it be confusing.

Yes, and this is added in #19 🐛 New User Logout confirm Form is misleading Needs work

Yes, I know that :)

I have made a minor change to the wording for review.

Looks good to me. In my opinion, we can move it to RTBC.

Updated screenshot. Still needs a test.

Added the check to test the existence of the description and that should be sufficient I guess.

Hi, I have verified this issue in our D11 version with the latest MR- 3469116-11.x-new-user-logout and it applies cleanly. This isssue is resolved now.

Testing steps:
As a logged in user, directly visit user/logout on a site (to trigger the CSRF protection and use the confirm form). The misleading text will be visible above the confirm/cancel buttons.

Test Results:
Logout confirmation form shows the appropriate confirmation description now.

Adding before and after screenshots.

Still putting this issue in needs review for the further code reviews.

Thanks!

Tests Failed.

Hi @dishakatariya we already have screenshots for this so we don't need any new ones.

Thanks, @pameeela for the rebase, pipelines passed, moving to NR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

Hello,
I have verified this issue on D11 version.
MR applies cleanly.

Testing steps:
As a logged in user, directly visit user/logout on a site (to trigger the CSRF protection and use the confirm form).
The misleading text will be visible above the confirm/cancel buttons.

Test Results:
Logout confirmation form shows the appropriate confirmation description now.

Refer screenshots-

Everything looks good, I'm sure the messaging will always be of debate, but this could always turn to a config option at a later date.

Text needs some fixing, "you'll" should be replaced with "you will"

Hello,
I have verified this issue on D11 version.
MR applies cleanly.

Testing steps:
As a logged in user, directly visit user/logout on a site.
The misleading text will be visible above the confirm/cancel buttons.

Test Results:
Logout confirmation form shows the appropriate confirmation description.

Refer attachments -

Hi, I've reviewed the code from branch 3469116-11.x-new-user-logout, and it works for me. I have attached a screenshot as evidence that it is working.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-15 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, and @simohell.

Our recommendation is to remove the text, "This action cannot be undone". Do not replace it with anything. The purpose of the form is clear without this additional text.

In other words, we strongly agree with Comment #3. And Comment #20.

I will add a separate comment with some technical notes.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

From Comment #20:

Just wanted to confirm this one because as noted above, this text wasn’t there when the change was committed. Added a screenshot to the IS.

I tested by checking out Commit 62c4d2d2fa6 (the commit for 🐛 User logout is vulnerable to CSRF Fixed on the 10.3.x branch). I installed Drupal with the Standard profile and visited /user/logout/confirm. The text was there: "This action cannot be undone." It is true that the screenshot on #144538 does not show that text, but the text was there when the issue was committed.

I was curious, so I investigated.

The problem is this commit on the MR for #144538: b6134f16, which removed this function (and added return-type declarations to 5 other functions):

  public function getDescription() {
    return '';
  }

The same day, the author of the commit made this comment #144538-186: User logout is vulnerable to CSRF → :

I pushed a change to add some typehint returns. If we aren’t adding a description I removed that function.

Actually found an issue that return ’’ doesn’t match the docs of the inherited docs.

That is a mistake. The text, "This action cannot be undone", comes from the base class. Removing that function means we are not overriding the base class.

Later that day (Comment #188), the author of the commit (and Comment #186) marked the issue RTBC. It is not good practice to review your own work.

The second line of Comment #186 makes the same observation as Comment #8 on this issue. Because the form class implements an interface, getDescription() has to return a TranslatableMarkup object, not a string. But it can return the object equivalent to an empty string:

  public function getDescription() {
    return $this->t('');
  }

phpcs fails in the MR

It is usually a bad idea to suppress phpcs warnings, but I think it is the right thing to do in this case.

There is a failure in Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder. That test is already listed on 🌱 [meta] Known intermittent, random, and environment-specific test failures Active , so I am re-running the tests. (I do not see a way to re-run a single test suite, so I am re-running the entire pipeline.)

I have not tested, but the code change looks correct. I am setting the status to NR for testing and further review.

Comment #9 asked for automated tests, but I am not sure that is needed.

The issue summary needs an update (proposed resolution and screenshots), so I am adding the tag for that.

Well, I tried to re-run the pipeline. I am not sure it is working.

If we do want automated tests, then we could test that the default text, "This action cannot be undone", does not appear on the page.

Benji, I agree that it is usually a bad idea to suppress phpcs warnings, but in this case there are valid reasons to do it.

I re-run the tests and now they are green.

I am replacing the screenshot in the issue summary and rearranging a bit to match the usual template. (Screenshots go under "User interface changes".) I am replacing "more appropriate text" with "empty text" in the Proposed resolution section.

My screenshot looks a lot like the one in 🐛 User logout is vulnerable to CSRF Fixed .

Previous comments and the Remaining tasks suggest adding an automated test, but I do not think that is needed.

Code looks good, the confirm form is what I show in the screenshot, and the confirm form works. RTBC.

I read the comments and the MR. All questions have been answered and this look fine. However, there is a question about a test, asked for in #9 and #27 but the tags, 'needs tests' was removed in #32 without comment. So, the question about a test needs to be answered.

I do think that this should have a test to prevent the re-introduction of a description on the form.

I have been able to replicate and review all the comments. I am working on the test.

I have added test coverage for this, following the approach mentioned in #48. Moving this to NR for review. Kindly review the changes.