phpBB3 Reractoring

[topdown]

Last week I was working on something in phpBB3 and started looking through the core and structure.
Once I was done with the project I was working on I started a fresh copy and went to it with changes that have been in the back of my mind for a while.

I was working on a master branch copy as I didn't look through the develop branch code all that well.
Tonight I decided to push the changes to a branch in my phpBB3 fork.
https://github.com/topdown/phpbb3/tree/ ... ctor/phpBB

Something to note: I added a diff, but make sure you have a decent machine when viewing it. Its BIG. A little over 11mg and 350,500 lines long.
Opens in Vim on my workstation fine.

Some key points of things I did.
Removed common globals and replaced them with static instances.
Why?
Because it gets rid of the thousands of global variable calls for every function.
So no more global $phpbb_root_path, $phpEx, $config, $cache, $db, $user, $auth, $template.
https://github.com/topdown/phpbb3/blob/ ... /phpbb.php
Ignore the files head docbloc it was the for testing my settings for phpcs

Restructured some of the core.
A lot of the includes files were classes named functions_*
Well they were not functions, but classes. All renamed and moved to system/core
includes got moved to system/core
the includes acp & mcp & ucp are in system/modules/

Because it makes sense, to me anyway.
All of the other system directories got moved to the system/ as well.

I completed a bunch of doc blocks while working on this.
Mostly because all of the missing parts of the PHPDocs lit up my IDE like a Christmas tree when viewing some of these files.

I realize it would have made more sense for me to do this with the develop branch but like I said I haven't looked at that branches code all that well.

Not sure what the point is behind these expressions
$phpbb_root_path = (defined('PHPBB_ROOT_PATH')) ? PHPBB_ROOT_PATH : './';

It makes no sense. By default PHPBB_ROOT_PATH does not exist so this always returns the else './'
And there are a bunch of these expressions.

Also statically called methods that are not static. eg captcha and profile_field methods.
Was this for backwards PHP compatibility?

What I did notice is that the new files in the develop branch are properly commented. Thanks

Anyway, have a look if you wish.

[naderman]

Our development strategy is to add new functionality and make improvements in 3.1 without breaking backward compatability entirely across the code base. A change like yours would undoubtedly break compatability for MODs. We're writing phpBB4 from scratch to solve such problems properly. For the time being I'd rather not do such large scale refactoring, because people still use MODs for 3.x. MODs depend on stable code structure for compatability.

It'd be great if you want to supply a patch that adds doc comments to existing functions for 3.1, since those won't break any MODs and they should help a lot

Some key points of things I did.
Removed common globals and replaced them with static instances.
Why?
Because it gets rid of the thousands of global variable calls for every function.
So no more global $phpbb_root_path, $phpEx, $config, $cache, $db, $user, $auth, $template.
https://github.com/topdown/phpbb3/blob/ ... /phpbb.php

While the idea of removing those global lines is a good one, your solution introduces several problems. Those variables should really be parameters for the functions they are used in or constructors of the containing class.

With globals one can at least replace these variables when calling a function. With your solution there is only one global instance of each that one is stuck with. Essentially you introduced a bunch of singletons to replace a set of variables. Just google for "singleton bad" or "singleton evil" to find plenty of explanations of why this is a bad idea. You're assuming that all code will always want to refer to exactly the same instances, which reduces code reusability and introduces tons of unecessary dependencies. Additionally it makes unit testing hard to impossible.

Not sure what the point is behind these expressions
$phpbb_root_path = (defined('PHPBB_ROOT_PATH')) ? PHPBB_ROOT_PATH : './';

It makes no sense. By default PHPBB_ROOT_PATH does not exist so this always returns the else './'
And there are a bunch of these expressions.

External scripts - e.g. a bridge integrating phpBB into a CMS - can use these constants to overwrite the value of these variables.

Also statically called methods that are not static. eg captcha and profile_field methods.
Was this for backwards PHP compatibility?

3.0.x is still compatible with PHP4. PHP4 does not know OOP keywords liks static. However as mentioned before, we do not plan on making such changes in 3.1 unless we need to modify the respective code anyway or there is significant benefit from making the stylistic change.

[igorw]

Hi.

First of all, it does not make much sense to make big changes against the 3.0 code base to be merged into 3.1. It will result in lots of conflicts.

Now, most of the changes you did just add phpbb:: before all of the globals. This was done in the first attempt at writing 3.1, but the idea was later dropped. While replacing globals with namespaced globals gives you the advantage of preventing naming conflicts, it is a huge break for compatibility. But the actual problem is that using globals is a fundamental design issue, and replacing globals with namespaced globals will not fix this.

This is why for any new code we try to get rid of all globals and inject dependencies instead. Which makes the code more modular, maintainable, testable, yadda yadda.

I don't see the point in renaming `includes` to `system`, as they are both equally bad names, of which neither comply with the PSR-0 naming standard.

As Nils mentioned, some of these minor improvements may be useful, such as fixing docblocks. And I am not opposed to including them (if they are provided as an isolated patch).