[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  |  
-----------------------------------+----------------------------------------

Comment(by cnorris23):

 I have three things to add other than what's already been discussed here
 and the dev chat.

 First a response:
 > I have not seen a line in WP core, where only the existence but not the
 value of a constant is checked.

 > - 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]).

 Your first quote is negated by your second quote. The code linked for the
 MULTISITE constant is fine, but the file in which your example is located
 contains 12 references to checks to {{{defined()}}} only checks. Two of
 these references are in the same line of code as your MULTISITE example.
 In fact, the line you referenced is the only line where the MULTISITE
 constant is checked to be true in addition to whether it's defined. It
 might be one thing if {{{defined()}}} only checks were only found in older
 functions, from, say, WP 0.71, but the MULTISITE code is fresh, unreleased
 code. To say that a {{{defined()}}} only check is outside the scope of WP
 coding standards, is therefore incorrect.

 Second, is another argument against. Let's say the patch did go through,
 for your use case, you're almost certainly not going to do a check solely
 on the {{{is_user_logged_in()}}} or {{{current_user_can( 'manage_options'
 )}}} (I know this was just your example code, so just hang with me ;) ).
 For your use case, you're almost certainly going to have multiple
 scenarios for when a user can and can't see the admin bar. If there is any
 sort of complexity to the rules, you will almost certainly reach a point
 where you have a conflict. Since, a constant cannot be redefined (because
 if it could it would, by definition, be a variable), you are likely to run
 into a situation where a user is/isn't shown the admin bar, when they
 should/should not, simply because BP_DISABLE_ADMIN_BAR was was previously
 defined an earlier rule.

 Third, is that BP_DISABLE_ADMIN_BAR, as already discussed, was meant to be
 a one time deal. You either want the admin bar shown or you don't. There's
 no need to change core code, especially when it adds overhead, when you
 can already do the same thing with existing code. All you need to do is
 this:
 {{{
 if ( is_user_logged_in() && current_user_can( 'manage_options' ) {
      remove_action( 'wp_footer', 'bp_core_admin_bar', 8 ); // removes
 admin bar from frontend
      remove_action( 'admin_footer', 'bp_core_admin_bar' ); // removes
 admin bar from backend
 }
 }}}

 I just found that code today, as kept thinking there had to be some other
 way to do this without the need of the constant, or core code changes. I
 promise at no time was I trying to be a jerk, and I wasn't holding out on
 you with the {{{remove_action}}} code just for the sake of arguing. Hope
 this is solution does what you need.

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