search

preinheimer / xhprof

XHGUI is a GUI for the XHProf PHP extension, using a database backend, and pretty graphs to make it easy to use and interpret.
Other
834 stars 185 forks source link

Several small fixes and enhancements #115

Open lopezio opened 6 years ago

lopezio commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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