[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 TobiasBg):

 Thanks for your follow-up, I have to object to some points however.

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

 I don't really see where you get those 12 references from. I'll try to
 break it down:
 - WP_INSTALLING is an internal constant that is only defined by WP, not by
 a user (in e.g. wp-config.php). Therefore the check for existence is
 enough.
 - E_DEPRECATED and E_RECOVERABLE_ERROR are PHP constants, if I'm not
 mistaken. Again nothing to be dealt with by the user. (The actual check
 for there value is most probably only needed by PHP internally.)
 - WP_LANG_DIR is not a boolean, but a string. Nothing to check there. And
 the defined() check is only to find out, if it still has to be set.
 - Same with LANG_DIR, WP_SITEURL, and WPMU_PLUGIN_DIR. The defined() are
 there to know that it is safe to try to use their values.
 - WP_ADMIN: The actual value is in the "return" clause. That's the same as
 checking for true/false and then returning that accordingly.
 - VHOST and SUNRISE are constants from WPMU that were left in the code for
 backward-compatibility to those installations of WPMU from before the
 merge in WP 3.0.
 (I don't have any background on them, but it's likely possible that they
 aren't booleans either, so that checking the value doesn't make sense.)
 To add on: MULTISITE is not fresh, unreleased code. It is being merged
 from an existing project (WPMU) that was not started by core devs (well,
 ok, now they are). That merge is not yet complete, so there might still be
 places where coding standards and common practices are not yet completely
 implemented.

 I hope these counter examples show enough how WP distincts between
 constants. To clarify my initial statement: WP checks for defined() &&
 value of boolean constants.
 (Actually, please see [http://core.trac.wordpress.org/ticket/12375
 #12375], which deals with the problem basically.)

 For your second argument: I know that there might be problems with already
 defined() constants in my example, but again: This would actually not
 affect anybody who is not trying to work with this. And mainly it is a
 disadvantage of using "constants" and not just of this special case.
 Nobody, who doesn't want to be affected, won't even notice the change.
 The patch just adds more possibilities. I know that there are ways for
 achieving my initial goal without setting the constant to false (like your
 remove_action() code), but this is not the issue here.
 The issue is that I have submitted a patch that enhances the current
 behavior by adding more possibilities and actually respecting the constant
 without bringing disadvantages.

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

 Right, your solution works, and so does my current. But again, the issue
 or goal of this ticket is not to find a workaround for this, but to
 "attack the base", i.e. the code that makes workarounds necessary, and
 enhance it so that workarounds are not needed. (BTW: All those workarounds
 add much more overhead, code lines, and headaches) than the additional
 check in the patch.)
 I'm also not trying to be a jerk and won't fight forever for this to go
 in. I just thought that it can enhance a developer's experience with BP.
 Also the main issue (missing checks for actual boolean values) is probably
 not only there with this constant, but it's where I first stumbled upon
 it, and it might be a good start to check for the same thing with other
 constants.

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