Several small fixes and enhancements

[lopezio]

Hi there,

I just wanted to share a few fixes / enhancements I've been doing while working with xhprof. I know it's a pull request with several changes, but they all boil down to:

• Fixes to be able to load xhprof in my Symfony 3.4 / PHP 7.1 environment (inclusion via composer.json / vendor/ structure). Note that since there are so many forks of this project, I gave it a rather generic name in composer.json (xhprof/xhgui), so that whoever includes it can choose from which repo/vendor he/she gets the code.
• Fixes to the hrefs so that they all honour the $_xhprof['url']
• Enhancements in the handling of config file loading (So that it can also be specified by ENV variable, constant, or apache environment variable)

If you deem them useful, I'd be happy to refer to the upstream repo instead of my fork in the future. If there's anything you'd like me to change and rebase, let me know.

Best wishes,

Lorenzo

[aik099]

The approach of connecting xhprof via Composer and then setting a constant (or env variable) indicating actual XHProf config doesn't look too intuitive.

Also if installed via Composer you won't be able to view XHProf reports, because vendor folder (in case of Symfony) is located outside of DocumentRoot.

I'd like bugfixes part of this PR though, but please remove all indentation changes that makes this PR look too big (lots of changes).

[lopezio]

Hi, I'll try to remove the indentation changes.

But about the vendor/ thing:

The changes don't require anyone to put xhprof in vendor or outside of the document root. In fact, the original authors actually designed this to be fully compatible outside of the document root, with exception of the xhprof_html directory (and it does work pretty well: includes can cross that boundary).

The changes are designed to be optional and completely backwards compatible and not break the current behavior.

Same goes for the constant / env variable: The old behavior works as is, no one is forced to do it, but for those who want to keep the configuration out of the vendor/ directory (like in many modern php projects), this gives them this possibility.

I'll see what I can to do split the PR.

[aik099]

The changes are designed to be optional and completely backwards compatible and not break the current behavior.

I understand that. I just don't see any reason for any project to connect profiling library as it's development dependency so that later same project (xhprof) needs to be cloned elsewhere to access same database, where profiling info was stored into.

I'll see what I can to do split the PR.

Good.

[lopezio]

I understand that. I just don't see any reason for any project to connect profiling library as it's development dependency so that later same project (xhprof) needs to be cloned elsewhere to access same database, where profiling info was stored into.

Sorry if I insist ;-). I think it does: Of course only in the require-dev section of composer.json, and not in the production requirements (require) section. And your point is also exactly the reason behind moving the config.php in a place where configs are located within the project (such as the parameters.yml in Symfony: It is individual per environment, and usually not committed into any repo): to make sure it's something used only in development / profiling. I'm stitching a new commit together with the indents corrected..

[lopezio]

So, I tried to remove all indent problems and it seems that I found. The documents already had mixed indents (partially tabs, partially spaces), that's where the confusion came from. I edited index.php and header.php to reflect the original indenting.

[lopezio]

Hi aik099, I just pushed the latest proposal, which should reflect most if not all of your requests. PHPStorm changed more CS than I expected, I did my best to reverse this.

Please see my comments, and let's agree on a suitable composer.json name for this. preinheimer/xhprof would be perfectly fine for me, but this needs to be in sync with what will be submitted to packagist. For the same reason, I removed the composer hint in the README, which is best added after a package is submitted to packagist.

Best,

Lorenzo