ddev / ddev-xhgui

XHGui service for a DDEV project
Apache License 2.0
12 stars 7 forks source link

Alert users to force adding xhprof_prepend.php #18

Closed deviantintegral closed 7 months ago

deviantintegral commented 1 year ago

Without this, others cloning the repository won't have a working setup.

rfay commented 1 year ago

I don't believe that's correct (about forcing). DDEV automatically detects files on its list that do not have #ddev-generated and so will show them wanting to be committed. But it would have to be done right after the add-on is installed.

Bottom line is that we need a better way to manage this whole thing, perhaps not using the existing xhprof setup and doing our own.

tyler36 commented 1 year ago

Any more thoughts here?

rfay commented 1 year ago

I think we'll want to re-think the defaults of how we're doing this in core ddev to make it a little more flexible here.

There are lots of people who use xhprof as it is now, so we don't want to break them, but we should be able to figure this out with just a little work, and changing things in core is fine, or maybe a Dockerfile can handle it.

deviantintegral commented 10 months ago

I wonder if it would be OK for us to require the php-profiler package.

The challenge with the current API (imposed by xdebug) is that when you call xdebug_disable(), you get the profiler data and no one else can. But, if we used the above package, we could add a class that keeps a reference to the return'ed data, and returns that instead of calling xdebug_disable().

Something like

https://github.com/perftools/php-profiler/tree/main/src/Profilers

tyler36 commented 10 months ago

@deviantintegral is that a DDEV core or addon suggestion?

deviantintegral commented 10 months ago

I think it would have to be a ddev core suggestion, since it owns the current xhprof_prepend.php file.

rfay commented 10 months ago

Sorting this out is definitely a near term priority as the xhgui approach will be best for most people. If we don't have an issue going in core ddev, please open one, thx

tyler36 commented 10 months ago

Issue created: https://github.com/ddev/ddev/issues/5698

tyler36 commented 7 months ago

Thank you for this PR.

After thinking about this, I came up with #28 which feels like a better solution. Happy to discuss before moving forward.