Several small fixes and enhancements #115

lopezio · 2018-04-19T13:25:29Z

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

… variable)

aik099 · 2018-04-19T16:19:34Z

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 · 2018-04-20T09:08:18Z

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 · 2018-04-20T09:33:26Z

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 · 2018-04-20T11:23:32Z

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 · 2018-04-20T12:36:22Z

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.

aik099 · 2018-04-20T12:50:42Z

xhprof_lib/utils/xhprof_runs.php

+			global $_xhprof;
+			if ($_xhprof['display'] === true)
+			{
+				echo "Failed to insert: <pre>$query</pre> <br>\n";


This is the only changed line if I view diff using https://github.com/preinheimer/xhprof/pull/115/files?w=1 . I guess it's your IDE that making all these indentation changes.

In git you only stage specific lines if you can't from unwanted changes in IDE itself.

Yes, sorry, again it was due to PHPStorm settings (as you guessed), combined with the fact that many files here have a mixed indentation. I'll fix that right away...

aik099 · 2018-04-20T12:53:43Z

xhprof_lib/templates/profTable.phtml

@@ -29,7 +29,7 @@

    <tr>
 	<td style="background-color: <?php echo $color; ?>;"><span style="display: none"><?php echo $color; ?></span>&nbsp;</td>
-        <td><a href="index.php?run=<?php echo $run1; ?>&amp;symbol=<?php echo urlencode($element['fn']); ?>"><?php echo htmlentities($element['fn'], ENT_QUOTES, 'UTF-8'); ?></a></td>
+        <td><a href="<?php echo $_xhprof['url']; ?>/index.php?run=<?php echo $run1; ?>&amp;symbol=<?php echo urlencode($element['fn']); ?>"><?php echo htmlentities($element['fn'], ENT_QUOTES, 'UTF-8'); ?></a></td>


In which scenarios without this change something breaks? I have xhprof installed in a sub-folder of DocumentRoot and all works perfectly fine already.

Exactly, this is one of the situations where not having the whole xhprof code in the document root breaks it (but, for example, only Alias as suggested in the original version, or a symlink of xhprof_html). Putting in this not only fixes xhprof for that case, but potentially also enables the use of a dedicated virtual host for it (although I haven't tested the latter yet), which might be very useful when comparing, for example, the results of different development hosts.

aik099 · 2018-04-20T12:57:58Z

xhprof_lib/display/xhprof.php

@@ -42,8 +42,9 @@
 * Our coding convention disallows relative paths in hrefs.
 * Get the base URL path from the SCRIPT_NAME.
 */
-$base_path = rtrim(dirname($_SERVER['SCRIPT_NAME']), "/");
-
+// $base_path = rtrim(dirname($_SERVER['SCRIPT_NAME']), "/");


No need to keep old code in commented format. Just replace it. There will be an explanation of made changes in the commit message itself.

You're right, I seem to have forgotten this along the way. Good catch. Removing it..

aik099 · 2018-04-20T12:58:19Z

xhprof_lib/config.sample.php

@@ -8,10 +8,11 @@
 $_xhprof['dbpass'] = 'password';
 $_xhprof['dbname'] = 'xhprof';
 $_xhprof['dbadapter'] = 'Pdo';
-$_xhprof['servername'] = 'myserver';
+$_xhprof['servername'] = 's01';


Why change this?

This is because the table structure has a CHAR(3) in the current version, and when I first copied these settings, they ended up breaking all INSERTs because "myserver" is 8 chars long. I thought it would be more intuitive to have an example with 3 chars, so no need to change table structure, but it doesn't fail on the first try.
There's another PR in the pipeline that I saw, that would rather change the table structure, but I thought that this change is less "destructive" for current users...

aik099 · 2018-04-20T12:59:21Z

xhprof_lib/config.sample.php

 $_xhprof['namespace'] = 'myapp';
 $_xhprof['url'] = 'http://url/to/xhprof/xhprof_html';
 $_xhprof['getparam'] = "_profile";
+$_xhprof['displayparam'] = "_display";


Is this a new parameter or an old one, that wasn't mentioned.

Would it work correctly for people with config without this parameter?

If this is a new parameter, then why it's needed?

A new one, but actually referring to an already present functionality ($_xhprof['display'])

Yes, as with "getparam", a default is set when it is not found (see header.php, line 79).

I thought it would be a nice thing to be able to switch the "display" functionality (which mainly displays a link to the xhprof page for the current run) on/off at runtime, via a parameter - exactly like for the _profile toggle. I like to have the link, but some pages just won't work with it, paricularly JSON output or templates generated for frontend frameworks.
I added the parameter to be "consistent" with the current way of handling the other runtime functionality, which is the "getparam"/profile.

aik099 · 2018-04-20T13:14:14Z

README.markdown

@@ -53,22 +53,36 @@ Installation

 * Install your favourite mix of PHP and web server
 * Install MySQL server
-* Clone the project to some folder
+* Clone the project to some folder of your choice.
+  * Alternatively, for composer fans: add a repository m in the repositories section of your composer.json:


what's with m letter (looks like a typo)

the composer.json should be backticks and composer word be Composer

aik099 · 2018-04-20T13:15:21Z

README.markdown

-* Clone the project to some folder
+* Clone the project to some folder of your choice.
+  * Alternatively, for composer fans: add a repository m in the repositories section of your composer.json:
+  <pre>


Use Markdown syntax instead of HTML (3 backticks for both opening/closing PRE).

aik099 · 2018-04-20T13:16:21Z

README.markdown

+  <pre>
+  {
+    "type": "git",
+     "url": "https://github.com/path-to-this-repo.git"


All this large fragment won't be needed if we publish it on Packagist. Please rewrite.

Cool. Are you going to do the publishing, or shall I take this on? Adding to my change list..

aik099 · 2018-04-20T13:17:27Z

README.markdown

+     "url": "https://github.com/path-to-this-repo.git"
+  }
+  </pre>
+  And then add the requirement `"xhprof/xhgui": "dev-master"` to your composer.json, then `composer update`.


You can replace this and previous 2 lines with just

Run the "composer require packagename --dev".

aik099 · 2018-04-20T13:18:02Z

README.markdown

- * Update the URL of the service (should point to `xhprof_html` over HTTP)
- * Update the `dot_binary` configuration - otherwise no call graphs!
- * Update the `controlIPs` variable to enable access.
+  * Update the SQL server configuration


Not sure why these lines are indented differently now.

Because it looks more intuitive to me to put all the steps pertaining to editing the configuration under it...

…o (SetEnv)

…root and the base path is wrong

…ng, add a _display parameter to toggle displaying of profiler link

…a CHAR(3), so using the old example broke the queries

lopezio · 2018-04-22T14:06:00Z

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

added variable config file location in header.php (via constant / env…

0ffd5ec

… variable)

lopezio force-pushed the master branch 2 times, most recently from fd7bbcf to 4f5eaf1 Compare April 20, 2018 11:56

aik099 requested changes Apr 20, 2018

View reviewed changes

lopezio force-pushed the master branch from 4f5eaf1 to 01c0b08 Compare April 22, 2018 13:54

lopezio added 7 commits April 22, 2018 15:59

added pre tag to error display for easier debugging

1bb665a

added a preliminary composer.json

de5860f

update config searching logic to find apache environment variables to…

63e2e96

…o (SetEnv)

fix urls for situations in which xhprof is symlinked in the document …

b16bbb4

…root and the base path is wrong

header.php refactoring: remove both & and ? from URI before redirecti…

04ddf70

…ng, add a _display parameter to toggle displaying of profiler link

change example 'servername' in sample config. the database column is …

6cf0d4f

…a CHAR(3), so using the old example broke the queries

update README to reflect some changes

13654e3

lopezio force-pushed the master branch from 01c0b08 to 13654e3 Compare April 22, 2018 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several small fixes and enhancements #115

Several small fixes and enhancements #115

lopezio commented Apr 19, 2018

aik099 commented Apr 19, 2018 •

edited

Loading

lopezio commented Apr 20, 2018 •

edited

Loading

aik099 commented Apr 20, 2018

lopezio commented Apr 20, 2018

lopezio commented Apr 20, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018 •

edited

Loading

aik099 Apr 20, 2018

lopezio Apr 22, 2018 •

edited

Loading

aik099 Apr 20, 2018

aik099 Apr 20, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018

aik099 Apr 20, 2018

aik099 Apr 20, 2018

lopezio Apr 22, 2018

lopezio commented Apr 22, 2018

Several small fixes and enhancements #115

Are you sure you want to change the base?

Several small fixes and enhancements #115

Conversation

lopezio commented Apr 19, 2018

aik099 commented Apr 19, 2018 • edited Loading

lopezio commented Apr 20, 2018 • edited Loading

aik099 commented Apr 20, 2018

lopezio commented Apr 20, 2018

lopezio commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopezio Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopezio Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopezio commented Apr 22, 2018

aik099 commented Apr 19, 2018 •

edited

Loading

lopezio commented Apr 20, 2018 •

edited

Loading

lopezio Apr 22, 2018 •

edited

Loading

lopezio Apr 22, 2018 •

edited

Loading