BuddyPress Singleton

[johnjamesjacoby]

Much of the clean-up of 1.5 and 1.6 paved the way for BuddyPress to eventually move away from needing to define the $bp global we've all come to know and love. The next part of this is turning the BuddyPress class into a singleton, with magic methods for getting/setting/checking the individual variables it uses.

bbPress has already gone this route in 2.1, though it completely removed its dependency on the $bbp global (it was only around for 1 version, with single-digit plugins using it.) Totally eliminating the $bp global will be much more difficult to do, though we can easily go it through-out BuddyPress core as we go.

Attached patch finishes the singleton job, but keeps the $bp global around as a byref to the singleton.

[johnjamesjacoby]

Use _doing_it_wrong() in place of wp_die() to protect users from misbehaving plugins

[DJPaul]

Nice and simple. Looks good. I think you're right that we work on moving $bp references over as-and-when rather than all at once, so we can get this patch in and handle any regressions one at a time..

[boonebgorges]

Replying to DJPaul:

Nice and simple. Looks good. I think you're right that we work on moving $bp references over as-and-when rather than all at once, so we can get this patch in and handle any regressions one at a time..

+1

[foxly]

From the patch...

To prevent unauthorized access, these variables are stored in a private array 
that is magically updated using PHP 5.2+ methods. This is to prevent third party
plugins from tampering with essential information indirectly, which would cause
issues later.

Seriously??

If my team wants to "tamper" with Buddypress, we can, and if necessary we will, hook into the WP plugin load process, read your class files in with fread(), modify them with preg_replace() and then load them back into memory with eval().

By making it difficult to inspect and modify BP's core variables, the only thing you're doing is wasting *everyone's* time (it makes it VERY hard to do unit testing) and annoying other plugin developers.

Yes, make it a singleton, but do NOT start setting methods and variables to private ...Buddypress is nowhere near mature enough for that yet.

-F

[johnjamesjacoby]

Replying to foxly:

Seriously??

Yes.

If my team wants to "tamper" with Buddypress, we can, and if necessary we will, hook into the WP plugin load process, read your class files in with fread(), modify them with preg_replace() and then load them back into memory with eval().

By making it difficult to inspect and modify BP's core variables, the only thing you're doing is wasting *everyone's* time (it makes it VERY hard to do unit testing) and annoying other plugin developers.

Yes, make it a singleton, but do NOT start setting methods and variables to private ...Buddypress is nowhere near mature enough for that

This patch changes none of the things you're upset about. There are are magic getters and setters in place that nullify your argument here.

Try:

function wow_init() {
	global $bp;

	$bp->version = 'wow';

	// uncomment to 
	//$bp = buddypress();
	//$bp->version = 'wow';	
}
add_action( 'bp_init', 'wow_init' );

function wow_wp() {

	echo buddypress()->version;

	global $bp;
	echo $bp->version;
	
	die;
}
add_action( 'wp', 'wow_wp' );

You're still able to "tamper" with whatever you'd like, the same as you always have. Nothing is more difficult, no ones time is wasted, no one is annoyed, no methods or variables are incorrectly private, existing visibility is maintained, and the $bp global is untouched.

[foxly]

I wasn't talking about the specific PHP code in the patch, I was talking about the design philosophy implied by the source code comment.

Making vars and methods private will cause you EPIC problems. Since PHP has no concept of "friends" like the C language family, there's no way for unit test code to "tamper" with a class' internal variables as part of the test process. It can also cause frustrating problems with debugging, because variables you think should be there are "invisible".

Build-out your unit tests with Razor. When you're done (and you actually have tests with decent code coverage) its unlikely you'll still be so keen on locking-down BP's classes.

-F

[johnjamesjacoby]

Replying to foxly:

I wasn't talking about the specific PHP code in the patch, I was talking about the design philosophy implied by the source code comment.

There are variables (version/db_version) that shouldn't be tampered with by third party plugins.

Making vars and methods private will cause you EPIC problems. Since PHP has no concept of "friends" like the C language family, there's no way for unit test code to "tamper" with a class' internal variables as part of the test process. It can also cause frustrating problems with debugging, because variables you think should be there are "invisible".

No method scopes have changed here, nor is there any reason for them to.

Build-out your unit tests with Razor. When you're done (and you actually have tests with decent code coverage) its unlikely you'll still be so keen on locking-down BP's classes.

I'm keen on us locking things down that are appropriate to lock down. You're arguing both sides: BuddyPress isn't mature enough to make visibility changes to -- write unit tests for what is apparently an immature platform. Which is it?

Both rules do apply, but to different portions of BuddyPress's code. This change, currently has very little to do with either of them, or anything else you've brought up so far.

Why a unit test would want to "tamper" with variables outside of the boundaries BuddyPress internally allows defeats the purpose of that test, since it will clearly always fail; rightfully so.

The lengthy clean-up efforts of 1.5 and 1.6 enable us to work towards a goal of protecting third party plugins from each other, preventing users of BuddyPress powered sites from seeing wp_die() messages they shouldn't see, and protect BuddyPress developers from themselves and each other, similar to WordPress's _doing_it_wrong() function.

Where the idea came from that it's anyone's intent to lock everything down, I'm uncertain. It's an incorrect conclusion to jump to.

I want you to know I appreciate your attention to detail in matters like this. In the extreme cases you describe, there is no benefit or reward from making this change. Please understand that those cases do not actually exist, and as such are not worth addressing at this time.

[foxly]

Why a unit test would want to "tamper" with variables outside of the boundaries BuddyPress
internally allows defeats the purpose of that test, since it will clearly always fail;
rightfully so.

The reason its imperative that unit tests be able to access and modify private methods and variables is so that you can get an acceptable level of test coverage (80%) without having to use an astronomically complicated test rig.

Listed below is the class 'misery()' from one of my second year CE courses. It's laid out how you're proposing to structure your new BP classes: private methods and variables for things that other developers aren't supposed to 'mess with'.

So to write a unit test for this class, all you have to do is run it through each possible input state, and check the output is valid. Therein lies the name: three of the class's methods are co-dependent (alice, bob, and mallory) private methods, and we can't modify the class' state variables ($foo, $bar, and $baz).

​http://en.wikipedia.org/wiki/Karnaugh_map

Thus, we would need to check 385 different states in the unit test, and 8 of those are impossible to validate due to aliasing in the output.

class misery {

  private var $foo;
  private var $bar;
  private var $baz;

  public function ed($arg){

      $this->foo= $arg;
  }

  public function joe(){

      $result = '';
 
      if($this->foo){
         $result .= $this->foo;
      }

      if($this->bar){
         $result .= $this->$bar;
      }

      if($this->baz){
         $result .= $this->$baz;
      }

      return $result;

  }

  private function alice() {

     $this->foo++;

     if($this->bar >= 5){
        $this->bar = 0;
     }
     else {
       self::bob();
     }
  }

  private function bob(){

     $this->bar++; 
     
     if($this->bar >= 7){
        $this->bar = 0;
     } 
     else {
        self::mallory();
     }   
  }

  private function mallory(){

     $this->baz++; 
     
     if($this->baz>= 11){
        $this->baz= 0;
     }    
  }

}

Misery is a simple class used to illustrate a point. In the real world, the situation is usually *much* worse.

None of misery's private methods accept arguments, each of which adds another degree of freedom to the equation. Throw in some methods that call assorted methods from outside classes and don't use dependency injection, and you can end up with a class that's literally *impossible* to test.

In addition to high complexity and long run times, test results from a class like misery provide little insight into what's causing a problem. Imagine misery's three private methods were each 500 lines long. The results would show you had a problem "somewhere" in those 1,500 lines of code. Or something those lines of code interacted with. Good luck finding it.

If I hadn't intervened in this this ticket, I'm pretty sure there would be all sorts of misery-like situations in BP within a few months.

Now, if you simply didn't set those vars and methods as private, the entire class could be tested in just nine operations:

1) Set $foo to 0 and check the state variables
2) Set $foo to 4 and check the state variables
3) Set $bar to 0 and check the state variables
4) Set $bar to 6 and check the state variables
5) Set $baz to 0 and check the state variables
6) Set $baz to 10 and check the state variables
7) Null the sate variables, run $ed, check the state variables
8) Set all state variables to 0 and run joe()
9) Set all the state variables to 2 and run joe()

And these tests would show *precisely* which method isn't working properly.

And that's why you don't set your vars and methods to private.

But really, why take our word for it. Did you know WP doesn't use *any* private methods or vars in their classes *either*? ...seriously, just ctrl-shift-f 'private var' and 'private function' and be enlightened.

**
 * Helper class to remove the need to use eval to replace $matches[] in query strings.
 *
 * @since 2.9.0
 */
class WP_MatchesMapRegex {

	/**
	 * store for matches
	 *
	 * @access private
	 * @var array
	 */
	var $_matches;

        // ...

	/**
	 * do the actual mapping
	 *
	 * @access private
	 * @return string
	 */
	function _map() {
		$callback = array(&$this, 'callback');
		return preg_replace_callback($this->_pattern, $callback, $this->_subject);
	}

}

Instead, they mark private methods and variables with a '_' prefix $_private_var and _private_method(){}

I'll give you one guess as to why they're doing that:

​http://unit-tests.trac.wordpress.org/browser/trunk/

The reason things have to be done this way is because PHP lacks two important OOP paradigms: Friends, ​http://en.wikipedia.org/wiki/Friend_function and Multi-inheritance, ​http://en.wikipedia.org/wiki/Multi-inheritance.

If PHP had friends, we could set vars and methods as protected with impunity, then add the unit test rig as a 'friend'. Or if PHP had multi-inheritance, we could partition a super-class' methods into sub-classes containing atomic groups of methods, unit-test the sub-classes, then multi-inherit them into a working super-class.

But PHP has neither of these, so if we start limiting the visibility of things, the only thing we're really doing is making our own life difficult.

The purpose of unit tests is to test the smallest individual units of code in an app *one at a time*. In all but the simplest of classes, this means testing the individual class methods.

If you make it impossible to test individual methods by making a class' vars and methods private, you are no longer unit testing, you're black-box testing ​http://en.wikipedia.org/wiki/Black-box_testing and you do *not* want to be taking on that level of complexity for an open-source project.

-F

[foxly]

The lengthy clean-up efforts of 1.5 and 1.6 enable us to work towards a goal of protecting
third party plugins from each other, preventing users of BuddyPress powered sites from seeing
wp_die() messages they shouldn't see, and protect BuddyPress developers from themselves and
each other, similar to WordPress's _doing_it_wrong() function.

Developers don't need to be protected from either themselves or each other.

When you design a system that idiots can use, only idiots will use it; and in the coding world, those idiots will also burn tremendous amounts of your time and trash you all over the internet while doing so.

This is Wordpress, not Windows.

You're working in a common memory space with a staggering array of dangerous functions that let other developers do practically anything they want to your code. Leave it open. Design for failure. By making even a token attempt to 'nerf' your code, you'll end up with the *worst* of *both* situations: something that's difficult to mess with when you need to and impossible to stop others from messing with when you don't.

The fact that people are 'messing' with Buddypress in the first place means its design is flawed.

Either:

1) Your design doesn't work properly and they're messing with it to make it work properly
2) You have inadequate documentation
3) You've failed to provide an interface surface that other developers want
4) Or your code monolithic, excessively complicated, and difficult to trace

Here's an example of why people are 'messing' with Buddypress.

See: example-01.jpg

Practically every client I work with that has group forums enabled on their site asks me to remove the "Home" tab from their forums. That's because, due to bad design decisions, the home tab offers confusingly similar, redundant, functionality to the group's forum and doesn't synchronize with it.

So how would another developer remove that tab?

See: example-02.jpg

Well, first they'd probably check your codex. Unfortunately, the search function on it is broken, and has been for more than a year.

See: example-03.jpg

Next, they'd probably check Google ...and a *lot* of people are clearly doing that, because it's #4 on the auto-suggest dropdown and #1 out of 24,000 results. If they follow the top link, it points to a 5-page long thread on the BP forums that's been going for 2.5 years, includes horribly out-of-date information, and has 2 out of 3 members of the core BP dev team flat-out saying "tamper with the $bp global to fix the problem".

But, even using the most current solution posted in the thread, the tab persists.

function bbg_remove_send_invites_from_group() {
	bp_core_remove_nav_item( bp_get_current_group_slug(), 'send-invites' );
}
add_action( 'bp_setup_nav', 'bbg_remove_send_invites_from_group' );

At this point, you've lost 95% of your customers, although people that code for a living would start to go hunting. Only experienced developers would know to use ctrl-shift-f to search the entire plugin source tree for bp_core_remove_nav_item(), which has been cleverly hidden in a file called bp-core-buddybar.php.

Totally thought to look there.

See: example-04.jpg

Examining the source file, its obvious at least one of these methods has incorrect documentation, since they can't both do the same thing. Because none of the function headers in the file have a @version or @since tag, we have to start guessing.

The correct function is bp_core_remove_subnav_item(), and it works as expected for most of the tabs on the groups screen. However, when we call bp_core_remove_nav_item( bp_get_current_group_slug(), 'home' ); it 404's the page.

There are maybe twenty people in the *entire world* that fully understand why this would happen.

See example-05.jpg, example-06.jpg

The first root cause is located beside the red square in the screen capture. Because your team didn't follow good code formatting standards its hidden from view on even a widescreen monitor. The second root cause happens because you made a design assumption in bp-groups-loader.php that invalidates the interface you offer through bp_core_remove_nav_item().

At this point, there's no practical option left except directly modifying the $bp global variable; and if you block plugins from modifying the global $bp variable, the only remaining option is to fork Buddypress.

In this example, you've failed on all four counts:

Your base design doesn't work properly. You've provided not only inadequate, but flat-out wrong documentation. You haven't provided a centralized interface surface to get a common set of tasks done, and you've hidden the functions that actually do the job in all sorts of obscure places.

In addition to this, while I've skipped over it in this example to save time, it would take another developer *days* of work to trace through the code and understand how your routing system works well enough to remove the tab's screen.

That's simply unacceptable. Especially considering that we wrote you a faster, easier to understand, and better documented class that's a drop-in replacement for the offending code, and sent it in almost eight months ago.

Other developers aren't 'messing' with Buddypress because they don't know what they're doing; they're messing with Buddypress because, in many cases, you didn't know what you were doing, and they're going to continue 'messing' with Buddypress until you fix the problems that made them mess with it in the first place.

The best thing you can do to fix all these problems is get your unit tests built: the simple act of testing your code, and the challenges you'll face in doing so, will straighten out your architecture and coding practices incredibly quickly.

Razor will find practically every design flaw in Buddypress and kick your ass until you fix it.

Write some tests. You'll see ... :)

-F

[johnjamesjacoby]

Patches welcome.

[cnorris23]

But really, why take our word for it. Did you know WP doesn't use *any* private methods or vars in their classes *either*? ...seriously, just ctrl-shift-f 'private var' and 'private function' and be enlightened.

It should be noted that this is not true. Expand your search for true enlightenment.

[foxly]

@cnorris23

Well played ...:)

Seriously though, other than in some of the 3rd party libs they're using, where are you seeing a private method or var in the core?

-F

[johnjamesjacoby]

Replying to foxly:

@cnorris23

Well played ...:)

Seriously though, other than in some of the 3rd party libs they're using, where are you seeing a private method or var in the core?

-F

class-http.php
class-smtp.php
class-wp-theme.php
taxonomy.php

It's not because it makes unit testing easier; it's because WordPress was PHP4 compatible for a majority of it's life.

In WordPress 3.5, the WP_Post class is final; WP_Internal_Pointers is final; WP_Screen is final; plenty of places where the intentions are not to allow extension or poisoning of object data.

There are mixes and matches of private variables and data, appropriately paired with magic getters and setters, that (just like with this ticket) make the points your trying to prove inaccurate for the purpose of this ticket.

[foxly]

Replying to johnjamesjacoby:

Fine. You win. Have your private vars and methods.

The more important issue is, have you made any progress on your unit tests yet?

-F

[johnjamesjacoby]

Replying to foxly:

Replying to johnjamesjacoby:

Fine. You win. Have your private vars and methods.

This was never a win/lose situation.

The more important issue is, have you made any progress on your unit tests yet?

Not yet; and you're doing a great job here already.

[johnjamesjacoby]

(In [6285]) Theme Compatibility:

• First pass at including theme compatibility into BuddyPress.
• Introduce dependency, theme-compatibility, template-loader files into Core component.
• Add theme compatibility classes for components with direct template access.
• Move actions and filters around for improved plugin dependency.
• Remove old $bbp references.
• Turn $bp global into byref singleton, and include methods for setting up theme compatibility. (Fixes #4470.)
• Add is_buddypress() function to bp-core-template.php.
• Rename incorrectly named bp_after_theme_setup sub-action.
• See: #3741.