cadorn / firephp-libs

FirePHP Server Libraries
http://www.firephp.org/
23 stars 15 forks source link

The new firephp 1.0 / insite library has hard-coded paths #7

Closed jerfowler closed 13 years ago

jerfowler commented 14 years ago

The various require() statements throughout the code make it difficult to integrate into 3rd party frameworks. One should use an autoloader to load the library. Also, 5.3 namespaces would be a nice touch...

Plus, refactor to remove all the global variables and procedural functions...

cadorn commented 14 years ago

How do the require() calls make it difficult to integrate?

5.3 namespaces cannot be added as PHP 5.1+ compatibility is a requirement.

Which global variables and procedural functions? If you use FirePHP/Init.php there should be no global variables or functions.

jerfowler commented 14 years ago

I have a Kohana 3 module here: https://github.com/jeremyf76/KO3_FirePHP

Kohana 3 uses a cascading file system, which you can read about here: http://kohanaframework.org/guide/about.filesystem

Which can cause problem when using a require() directly in the code.

I wasn't too serious on the namespaces, which is completely understandable.

The FirePHP/Init.php uses a named constant as a global variable... FIREPHP_ACTIVATED It also has a single use function that is defined and called within the same file. Plus you have a class definition within a control structure.

I just think it could be done better... Take Jade.php for example. It was easy for me to fork that and create a Kohana module. He borrow's Symphony 2's UniversalClassLoader to Initialize the libraries. A very well laid out design and a great example of using 5.3 namespaces. You can see my fork here: https://github.com/jeremyf76/jade.ko3

cadorn commented 14 years ago

I would be happy to use an autoloader instead of the require() calls. Is that something you can refactor and contribute for the three projects?

The FIREPHP_ACTIVATED constant needs to be a global constant to allow for if(FIREPHP_ACTIVATED) in code. I do not think this is unreasonable.

The single use function in the Init.php file is needed to not pollute the global namespace with variables needed during init. Deleting the variables with unset($activate) does not work. I have not tried unset($GLOBALS['activate']) which we could use instead if that works (and get rid of the function).

The class definition in the control structure is needed to define the FirePHP class and route logging calls to nothing if FirePHP is not activated. I don't see a problem with this.

cadorn commented 13 years ago

The FirePHP 1.0 library now uses an autoloader. See: https://github.com/firephp/firephp

Check it out and let me know if things work now. If not, please post an issue on the new repository. (Not this one).