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

feat: Integrate XHGui into DDEV, fixes #6894 #7069

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

rfay
Copy link
Member

@rfay rfay commented Mar 12, 2025

The Issue

Honors to @tyler36, who pioneered and then matured the xhgui add-on until it could be brought into DDEV here.

How This PR Solves The Issue

Add integration for xhgui

TODO

  • Handle case of existing ddev-xhgui add-on
  • Tests
  • Fix ddev xhgui -j, perhaps add some support
  • Make the xhgui port configurable
  • Make work without router like mailpit
  • FIgure out if ddev xhgui should launch browser. Adjust as needed
  • Report to TYPO3 folks (for typo3.org) about the outcome at link, using these examples.

Manual Testing Instructions

Docs suggestions:

  • ddev config global --xhprof-mode=xhgui
  • ddev xhgui on
  • ddev xhgui status
  • ddev xhgui launch
  • ddev xhprof on
  • Validate that xhprof still works with xhprof-mode=prepend
  • Install ddev/ddev-xhgui and then ddev start to see removal
  • Test with a Postgres project

Automated Testing Overview

  • TestDdevXhprofXhguiEnabled
  • TestCmdXHGui
  • TestXHGuiSetup

Release/Deployment Notes

  • Note that the ddev-xhgui add-on will not be compatible.
  • Announce to supporting TYPO3 community!
  • Blog and screencast

@rfay rfay changed the title Integrate xhgui into DDEV, fixes #6894 Integrate XHGui into DDEV, fixes #6894 Mar 12, 2025
@rfay rfay changed the title Integrate XHGui into DDEV, fixes #6894 feat: Integrate XHGui into DDEV, fixes #6894 Mar 12, 2025
@github-actions github-actions bot added dependencies Pull requests that update a dependency file enhancement labels Mar 12, 2025
@rfay rfay force-pushed the 20250307_rfay_xhgui branch from a242807 to 22f4430 Compare March 12, 2025 21:09
@rfay rfay force-pushed the 20250307_rfay_xhgui branch 2 times, most recently from 64d750d to 4fd2b3f Compare March 14, 2025 22:07
@rfay rfay marked this pull request as ready for review March 15, 2025 18:20
@rfay rfay requested review from a team as code owners March 15, 2025 18:20
@rfay rfay requested review from tyler36 and stasadev March 15, 2025 18:20
@rfay
Copy link
Member Author

rfay commented Mar 15, 2025

Moved to "Ready for Review" even though certainly not perfect. Folks who have used xhgui will be great reviewers. Hope to get @tyler36 's expertise in here.

@rfay rfay mentioned this pull request Mar 15, 2025
15 tasks
@rfay rfay force-pushed the 20250307_rfay_xhgui branch from b6a57c1 to a00248c Compare March 16, 2025 15:41
@tyler36
Copy link
Collaborator

tyler36 commented Mar 17, 2025

Did some basic testing with DDEV v1.24.3-54-g485c3ad89

  • ddev xhgui asks to add global configuration
  • ddev config global --xhprof-mode=xhgui updates config
  • ddev xhgui on starts debugging, confirmed with ddev xhgui launch
  • ddev xhgui status correctly reports on
  • ddev xhgui off and am no longer able to access website via ddev xhgui lanuch
  • ✅ delete project database, changed to Postgres 16 and correctly ran ddev xhgui launch
  • ddev xhgui -h and ddev xhgui --help both correctly display help messgae
  • ddev xhgui -j launches xhgui website.

@rfay
Copy link
Member Author

rfay commented Mar 17, 2025

Thanks @tyler36 - I didn't implement ddev xhgui -j, what would you expect it to do?

ddev xhgui and ddev xhgui launch do kind of the same as ddev xdebug and ddev xdebug on

@tyler36
Copy link
Collaborator

tyler36 commented Mar 17, 2025

Thanks @tyler36 - I didn't implement ddev xhgui -j, what would you expect it to do?

Not sure what I expected. Just felt a little strange that it opened my browser.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!

This change also needs an update to the global schema.json

(I will test edge cases later today.)

@rfay
Copy link
Member Author

rfay commented Mar 17, 2025

@tyler36 I think I'll change ddev xhgui to enable and show status and not launch browser. I guess the reason it did that was the command in the add-on did that (but that's all it did).

@stasadev stasadev self-requested a review March 17, 2025 14:03
@stasadev
Copy link
Member

I think I'll change ddev xhgui to enable and show status and not launch browser.

I actually like that it opens the browser. No need to remember anything. (See my suggestion for -j).

@rfay rfay force-pushed the 20250307_rfay_xhgui branch 3 times, most recently from f13688d to 2e557e9 Compare March 18, 2025 15:12
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old approach doesn't work for me.
(I don't see any records when I refresh the page.)

ddev config global --xhprof-mode-reset && ddev restart && ddev xhprof && ddev launch /xhprof

(Not important) I noticed some inconsistency in --xhprof-mode-reset (it may also exist in another -reset flag):

ddev config global --xhprof-mode=xhgui
ddev config --xhprof-mode=prepend

# doesn't reset .ddev/config.yaml
ddev config --xhprof-mode-reset

# but this works
ddev config --xhprof-mode=""

@@ -34,6 +34,8 @@ var SSHAuthTag = "v1.24.3"
// BusyboxImage is used a couple of places for a quick-pull
var BusyboxImage = "busybox:stable"

var XhguiImage = "ddev/ddev-xhgui:20250307_rfay_xhgui"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the same logic as for other images, i.e. two variables Image and Tag.

It's needed for deleteDdevImages - which also needs an update to clean old xhgui images.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really have wanted to leave the two-variable technique behind. Hate it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier for me to see which tags I need to update (looking at the whole variable) when working on a new release.

@rfay rfay force-pushed the 20250307_rfay_xhgui branch from 9854613 to 49bda54 Compare March 18, 2025 21:11
@stasadev stasadev force-pushed the 20250307_rfay_xhgui branch from 49bda54 to 594f140 Compare March 19, 2025 13:00
@stasadev
Copy link
Member

I pushed several new commits:

  • rebased
  • added && ddev restart to ddev xhgui output (because restart is required) when prepend is active.
  • added variable for tag in versionconstants.go for xhgui
  • added cleanup in deleteDdevImages for xhgui
  • reformatted ddev xhgui -h examples

@stasadev stasadev self-requested a review March 19, 2025 13:09
@rfay
Copy link
Member Author

rfay commented Mar 19, 2025

I added a sleep to the TestCmdXHGui

Improved the ddev xhgui -h

Copy link
Collaborator

@tyler36 tyler36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ddev version v1.24.3-95-g915cd248b.

  • ddev xhgui asks to add global configuration

    • Manually remove xhprof-mode from global config
    $ ddev xhgui
    XHProf Mode is set to 'prepend', can't use 'ddev xhgui'.
    Use the command below to enable it:
    ddev config global --xhprof-mode=xhgui && ddev restart
  • ddev config global --xhprof-mode=xhgui updates config

    $ ddev config global --xhprof-mode=xhgui && ddev restart
    ...
    Your project can be reached at https://lara11-base-demo.ddev.site
  • ddev xhgui on starts debugging, confirmed with ddev xhgui launch

    $ ddev xhgui                                            
     Container ddev-lara11-base-demo-xhgui  Created 
     Container ddev-lara11-base-demo-xhgui  Started 
    Started optional compose profiles 'xhgui' 
    Enabled XHProf for XHGui. 
  • ddev xhgui status correctly reports on

    $ ddev xhgui status
    XHProf is enabled and capturing performance information. 
    The XHGui service is running and you can access it at https://lara11-base-demo.ddev.site:8142 
  • ddev xhgui off and am no longer able to access website via ddev xhgui lanuch

    $ ddev xhgui off 
    $ ddev xhgui status
    XHProf is disabled. 
    XHGui is disabled. 
  • ✅ delete project database, changed to Postgres 16 and correctly ran ddev xhgui launch

    $ ddev delete -Oy
    $ ddev config --project-type=php --database=postgres:16
    $ ddev xhgui
  • ddev xhgui -h and ddev xhgui --help both correctly display help messgae

    $ ddev xhgui
    ddev xhgui -h    
      Starts or checks status of XHGui/XHProf performance monitoring and UI.
      You can use 'ddev xhgui on' to enable, 'ddev xhgui' or 'ddev xhgui launch' to view the UI.
      'ddev xhgui status' shows the status.
    ...
  • ddev xhgui -j does nothing (previously opened browser)

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Install ddev/ddev-xhgui and then ddev start to see removal
  • Validate that xhprof still works with xhprof-mode=prepend
  • tested it with mysql:8.4
  • tested it without db container, seeing an expected error on ddev xhgui

My nitpicks:

  • We should have a named volume for xhgui Dockerfile has an unnamed volume for /run/nginx perftools/xhgui#505

  • The xhgui image is pulled up during the first ddev start, which may slow down some CI tests, but on the other hand, people may see it and wonder "What is this new image? What does it do?" and start using it, which can give us more feedback on ddev xhgui

  • The port configuration can only be done with manual editing for .ddev/config.yaml, I don't expect people will use this often, so not asking for new flags in ddev config

  • Testing without router worked only when I added host_xhgui_port: 8142, without it opens the main website (the host port is not assigned automatically, I don't see it in the ddev describe),

  • Not related to this PR, but without router ddev mailpit opens http://127.0.0.1:8025, and not the actual port from ddev describe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants