[buddypress-trac] [BuddyPress] #2023: BP_DISABLE_ADMIN_BAR should be checked for true and not just existence

[buddypress-trac at lists.automattic.com]

#2023: BP_DISABLE_ADMIN_BAR should be checked for true and not just existence
-----------------------------------+----------------------------------------
Reporter:  TobiasBg                |       Owner:       
    Type:  enhancement             |      Status:  new  
Priority:  minor                   |   Milestone:  1.2.2
Keywords:  has-patch dev-feedback  |  
-----------------------------------+----------------------------------------
Changes (by TobiasBg):

  * keywords:  has-patch => has-patch dev-feedback


Comment:

 Here's a list of arguments for/against the patch:

 ----

 '''Pros:'''
 - Makes code actually do, what the meaning/semantics reflect, i.e define(
 'BP_DISABLE_ADMIN_BAR', false ); actually does what it says and not the
 complete opposite.
 This allows code like
 {{{
 'BP_DISABLE_ADMIN_BAR', current_user_can( 'manage_options' ) );
 }}}
 (pure demo code here! Please don't argue that this can be achieved by
 other means.)

 - The patch introduces consistency and applies accepted coding standards
 from/with WP (see e.g. [http://core.trac.wordpress.org/browser/trunk/wp-
 includes/load.php#L567 MULTISITE],
 [http://core.trac.wordpress.org/browser/trunk/wp-includes/script-
 loader.php#L59 SCRIPT_DEBUG],
 [http://core.trac.wordpress.org/browser/trunk/wp-includes/post.php#L4184
 DOING_AUTOSAVE]).

 - The patch is backward-compatible: Nothing changes for default
 installations. If the constant is not defined, no additional checks are
 applied. If the constant is defined, the code additionally checks if it is
 true (which it e.g. also is, if the constant were defined to an arbitrary
 string, like 'ninjas').
 The only differing case occurs for define( 'BP_DISABLE_ADMIN_BAR', false
 );, but that's the one we actually want to be obeyed.

 - The same argument disarms the "additional check/comparison needed"
 argument, as that is only done if the constant is defined (in which case
 the user *wants* the check/comparison).

 ----

 '''Cons:'''
 (from the chat log); my answer in ''italics''

 - No double negatives (BP_'''DISABLE'''_ADMIN_BAR', '''false''').
 ''(Nobody has to use it, if he doesn't want to/doesn't understand, but it
 will adhere to the expected behavior. Ideas for this issue were to rename
 the constant, but that's the worst option of all.)''

 - No need to check defined() && value.
 ''(As mentioned above, the value is only checked, if it is defined(),
 bringing no overhead at all to default installations, but gainining actual
 respect of the value.)''

 ----

 '''Remarks'''
 Another approach would be to move the defined() checks out of the way,
 into something like bp-default-constants.php, where all all constants that
 have not yet been set (i.e. by a plugin) are set to a default value.
 This would mean that no further defined() checks would be necessary for
 most constants, but only checks for their values.
 However, that would have to involve many more constants than just the one
 from this ticket.

-- 
Ticket URL: <http://trac.buddypress.org/ticket/2023#comment:4>
BuddyPress <http://buddypress.org/>
BuddyPress