Skip to content

Remember previous state of remember login checkbox#22271

Merged
DeepDiver1975 merged 2 commits intomasterfrom
remember-login-state
Feb 10, 2016
Merged

Remember previous state of remember login checkbox#22271
DeepDiver1975 merged 2 commits intomasterfrom
remember-login-state

Conversation

@vincchan
Copy link
Copy Markdown
Member

fixes #22205

@mention-bot
Copy link
Copy Markdown

By analyzing the blame information on this pull request, we identified @jancborchardt, @DeepDiver1975 and @LukasReschke to be potential reviewers

Comment thread core/templates/login.php Outdated
<div class="remember-login-container">
<input type="checkbox" name="remember_login" value="1" id="remember_login" class="checkbox checkbox--white">
<label for="remember_login"><?php p($l->t('Stay logged in')); ?></label>
<?php if ($_POST['remember_login'] !== '1') { ?>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a little PHP annoyance here that would lead to some error logging here. If you use $_POST (or one of the other superglobals) on an not existing key this will throw an error in the error log:

➜ master git:(remember-login-state) ✗ tail -f data/owncloud.log
{"reqId":"0z0JSUoucxFosykwVUj+","remoteAddr":"::1","app":"PHP","message":"Undefined index: remember_login at /Users/lukasreschke/Documents/Programming/master/core/templates/login.php#71","level":3,"time":"2016-02-10T11:50:06+00:00"}

To fix this we usually use a ternary operator like isset($_POST['remember_login']) ? $_POST['remember_login'] : 0. Also it would make sense to move this piece into \OC_Util::displayLoginPage to move it out of the template. Something like:

$parameters['rememberLoginState'] = isset($_POST['remember_login']) ? $_POST['remember_login'] : 0;

You can then access the value in this template using $_['rememberLoginState'].

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But besides my comment: 🚀 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you set checked="checked" on the checkbox directly instead of doing the roundtrip with the attribute?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz Yeah, you are right. I don't know what went through my head. I'm gonna fix that :)

@vincchan
Copy link
Copy Markdown
Member Author

@LukasReschke @blizzz

Thanks for the input, please have another look :)

@blizzz
Copy link
Copy Markdown
Contributor

blizzz commented Feb 10, 2016

looks good and works 👍

@blizzz blizzz added this to the 9.0-current milestone Feb 10, 2016
@karlitschek
Copy link
Copy Markdown
Contributor

👍

@LukasReschke
Copy link
Copy Markdown
Member

Thanks again @vincchan for the contribution 🚀 🍻

DeepDiver1975 added a commit that referenced this pull request Feb 10, 2016
Remember previous state of remember login checkbox
@DeepDiver1975 DeepDiver1975 merged commit 39e6a18 into master Feb 10, 2016
@DeepDiver1975 DeepDiver1975 deleted the remember-login-state branch February 10, 2016 16:25
@jancborchardt
Copy link
Copy Markdown
Member

Nice @vincchan, thanks for fixing the issue! :)

@lock
Copy link
Copy Markdown

lock Bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

»Stay logged in« checkbox resets to unchecked when wrong password was put in

7 participants