Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alert users to force adding xhprof_prepend.php #18

Closed
wants to merge 1 commit into from

Conversation

deviantintegral
Copy link
Contributor

@deviantintegral deviantintegral commented Nov 10, 2023

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

@rfay
Copy link
Member

rfay commented Nov 10, 2023

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
Copy link
Collaborator

tyler36 commented Nov 28, 2023

Any more thoughts here?

@rfay
Copy link
Member

rfay commented Nov 29, 2023

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
Copy link
Contributor Author

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

  • 1st xdebug_disable() call stores the reference to the returned data, stores it in a class property.
  • By default, subsequent calls return that same data.
  • If you call enable() again, we set the property back to [] or null or whatever.

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

@tyler36
Copy link
Collaborator

tyler36 commented Jan 12, 2024

@deviantintegral is that a DDEV core or addon suggestion?

@deviantintegral
Copy link
Contributor Author

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

@rfay
Copy link
Member

rfay commented Jan 15, 2024

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
Copy link
Collaborator

tyler36 commented Jan 16, 2024

Issue created: ddev/ddev#5698

@tyler36
Copy link
Collaborator

tyler36 commented Apr 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants