-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Hide the # tab if registrations are turned off #460
Conversation
@@ -193,6 +193,10 @@ public function get_lock_options() { | |||
|
|||
$options_obj = array_replace_recursive( $extraOptions, $options_obj, $extended_settings ); | |||
|
|||
if ( ! $this->wp_options->is_wp_registration_enabled() && ! isset( $options_obj["allow#"] )) { | |||
$options_obj["allow#"] = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if is_wp_registration_enabled
is false so that the first condition passes; but then the allow#
is true. You would keep allow#=true
instead of turning it off (since is_wp_registration_enabled
is false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended. That would mean someone has {"allow#":true}
in their extra Lock settings and we want to respect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but won't that fail due to wp_reg being disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the main pathway, yes, but some of that is hook-able so it can be overridden. These extra settings JSON fields say they will override all other settings and I want them to be able to do that, even if it's edge case
return users_can_register_#_filter(); | ||
} | ||
return get_site_option( 'users_can_register', 0 ) == 1; | ||
return is_multisite() ? users_can_register_#_filter() : get_site_option( 'users_can_register' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would that second value be the same to get_site_option( 'users_can_register', 0 ) == 1
(previous)? Is this tested somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common practice. get_site_option( 'users_can_register', 0 ) == 1
means "get the option users_can_register
and if it doesn't exist use 0
and then loosely compare that to 1
."
If users_can_register
is on, it will be a "1"
(true), if it's off it will be a "0"
(false), and if it's not set, get_site_options()
will return an explicit false
. Codex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for that link ⚡️
2f7557c
to
5b08e93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌮
No description provided.