[patch] BP_DISABLE_ADMIN_BAR should be checked for true and not just existence

[TobiasBg]

When playing around with the constant BP_DISABLE_ADMIN_BAR (I want to add a more fine grained control on who sees the admin bar), I noticed that in all places where it is used, it is only checked whether the constant is defined, but not if it is actually defined as true.
This means that

define( 'BP_DISABLE_ADMIN_BAR', false );

does not have the expected effect.

The attached patch changes this behavior.

[TobiasBg]

Patch do add better constant checks

[cnorris23]

The current implementation, defined( 'BP_DISABLE_ADMIN_BAR' ), is correct. The BP_DISABLE_ADMIN_BAR constant isn't define in the core BuddyPress code, and is only later defined by the user. Either in a plugin, their bp-custom.php, or their wp-config.php, so it's existence is all that needs to be checked. Whether you define it as true, false, or 'ninjas' :), all that matters is that it's defined. The recommended way to define it is set as define( 'BP_DISABLE_ADMIN_BAR', true );, because that makes the most logical sense. Unless you can give a good use case as to why this should be changed, this ticket will likely be closed.

[TobiasBg]

I don't really agree.

I know that it is not pre-defined, but user defined - so it may exist, but does not always. That's what the defined() check is for. However, I believe that the actual value should also be checked:
For one: It is meant to be a boolean constant ("disable": yes/no).
Second: It's consistency: I have not seen a line in WP core, where only the existence but not the value of a constant is checked. This is just common code convention, in my eyes.

My use case is this:
I want to develop a more fine grained control of who can see the Admin Bar and who can not.
For that I would like to use something like this (just a short code fragment):

if ( is_user_logged_in() && current_user_can( 'manage_options' ) {
  define( 'BP_DISABLE_ADMIN_BAR', true );
} else {
  define( 'BP_DISABLE_ADMIN_BAR', false );
}

However, this does not work with just the checks for defined(), which I learned the hard way, as it took me forever to find the actual cause.

And if the recommended way actually is to set it to true, then the checks should also check for that.
(Keep in mind, that adding the checks is also backwards compatible, in case someone actually has set the constant to 'ninjas', as that will evaluate to true, also.)

[TobiasBg]

Chatlog of a discussion about this:
​https://irclogs.wordpress.org/chanlog.php?channel=buddypress-dev&day=2010-02-24#m56082

Summary/list of arguments to follow tomorrow.

[TobiasBg]

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. ​MULTISITE, ​SCRIPT_DEBUG, ​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.

[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. ​MULTISITE, ​SCRIPT_DEBUG, ​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.

[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 ​#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.

[cnorris23]

I could continue with the point-counterpoint, but I don't feel it's just not worth it to me. IMHO, and I know you may disagree as you have already stated in the dev chat, that both sides have presented valid arguments. Ultimately, however, the fate of this ticket is not up to us. Is it going to create some big bottleneck, or drastically change a behavior... no. Is it a necessary or valuable change... myself, and a couple of others seem to think not. It was fun debating, which is always helps me learn more, but in the words of Kenny Rogers, 'you gotta know when to fold 'em." :)

[TobiasBg]

Refreshed patch to add better logic checks

[TobiasBg]

Just added a patch to fix this for the 1.2 branch.

Patch brings better coding standards and more possibilities at no measurable additional cost.

One occurance of BP_DISABLE_ADMIN_BAR in bp_core_override_adminbar_css() left out, as that should be removed, see #2024.

[DJPaul]

We've decided to not patch this one. The admin bar will be re-developed for BP 1.3 to use the admin bar which will be present in WP 1.3, so there may be a WP core constant to change such behaviour.