Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back deleting of user preferences #278

Closed

Conversation

mnoorenberghe
Copy link
Contributor

This addresses 7b541d9#commitcomment-8211326 and brings back the old behaviour where setting a value to NULL deletes it.

@mnoorenberghe
Copy link
Contributor Author

@mnoorenberghe
Copy link
Contributor Author

I think this also breaks registration at the email verification stage: https://github.com/fisharebest/webtrees/blob/master/login.php#L553-L556

Could we have a 1.6.1 for this?

@fisharebest
Copy link
Owner

Yes we need a 1.6.1 release to fix this.

Previously we had a combined read/write/delete function, which was split up into separate read/write functions to create a cleaner/clearer API. (I think I'd been using too much jQuery when I initially wrote it...)

Instinctively it would seem preferable to have a separate delete function - instead of a combined write/delete function.

The combined function is clearly more "convenient", but perhaps not so clear as

if ($theme === null) {
    $user->deletePreference('theme');
} else {
   $user->setPreference('theme', $theme);
}

What are your views?

@mnoorenberghe
Copy link
Contributor Author

I would be fine with a separate deletePreference method and I kinda guessed you would suggest it.
The main downsides are that it's more work to fix code using the old API (4 more lines) and we would have to audit all existing callers to make sure NULL is never passed as the 2nd argument to setPreference.

@fisharebest
Copy link
Owner

I think I prefer the simplicity of the separate API function, and the clarity of the explicit call to deletePreference().

My IDE tells me there are only 42 calls to User::setPrefernence(), so reviewing the code won't be too onerous. There are only a few user settings that we would want to be able to delete.

If we try to set other values to null, it will highlight any deficiencies in our input validation / defaults.

fisharebest added a commit that referenced this pull request Oct 20, 2014
@fisharebest
Copy link
Owner

User::deletePreference() has been added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants