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

Improve handling of xhprof_prepend.php #28

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Apr 26, 2024

The default xhpro_prepend.php file that ships with recent flavors of DDEV is NOT compatible with this addon.

Currently, we take ownership of it so this addon can function. However, we don't return ownership. This could cause problems if the addon is removed or disabled.

This PR offers a different approach.

  • use post-start hook to claim ownership.
  • use pre-stop hook to return ownership.

This handles ownership while the addon is active.
But returns ownership when stop; thus DDEV will return to "default" state next time it checks the file.

Fixes #11

@tyler36 tyler36 marked this pull request as draft April 26, 2024 04:03
@tyler36 tyler36 force-pushed the 11-Unwilling-to-remove-xhprof_prependphp-because-it-does-not-have-ddev-generated-in-it branch from 9539519 to 40b6db1 Compare April 26, 2024 04:07
@tyler36 tyler36 marked this pull request as ready for review April 26, 2024 04:07
@tyler36 tyler36 force-pushed the 11-Unwilling-to-remove-xhprof_prependphp-because-it-does-not-have-ddev-generated-in-it branch from 40b6db1 to 118cae3 Compare April 26, 2024 09:10
Copy link
Contributor

@deviantintegral deviantintegral left a comment

Choose a reason for hiding this comment

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

Great idea! 👏

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Minor changes, suggesting cp if that is easy for you.

Overall, I'm still not even happy that we mount the prepend file and think we should be doing something better in DDEV rather than working around it. But here we are, so this approach is definitely an improvement.

// We'll temporarily override it here, but return control back later.
// If you don't want this behavior, comment out the hooks in ".ddev/config.xhgui.yaml".
return;
PHP
Copy link
Member

Choose a reason for hiding this comment

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

This line would be easier to understand if it were just the traditional EOF or maybe PHP_EOF. Caught me by surprise. Not that important of course.

hooks:
post-start:
- exec: |
cat <<-PHP > .ddev/xhprof/xhprof_prepend.php
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to use a cp here instead of cat? Then it's also one line.

PHP
pre-stop:
- exec: |
cat <<-PHP > .ddev/xhprof/xhprof_prepend.php
Copy link
Member

Choose a reason for hiding this comment

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

Same, wouldn't cp be easier?

- commands/host/xhgui
- xhgui/Dockerfile
- xhgui/xhgui.config.php
- xhgui/collector/xhgui.collector.config.php
- xhgui/collector/xhgui.collector.php
- xhgui/nginx.conf
- xhprof/xhprof_prepend.php
Copy link
Member

Choose a reason for hiding this comment

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

If you use cp you'd still have this, it would just be indirectly installed.

@tyler36
Copy link
Collaborator Author

tyler36 commented May 10, 2024

Wouldn't it be easier to use a cp here instead of cat? Then it's also one line.

The idea here was to avoid copying a file. I want to dynamically write out it.

Because the file contents contains #ddev-generated , I couldn't figure out how to escape the hash using echo.
Instead, I went with HEREDOC syntax. The output file contains documentation that makes it more understandable about what the file does and how it behaves.

If we went with cp, we would copy over a file (so we now how 2 files, our temp fake and the replace xhprof_prepend.php).
What about pre-stop hook? Writing #ddev-generate cause DDEV to replace the file with it's default on start up (see #29 (comment)) so we could keep it pre-stop here.
I not sure about using cp here, would that be another file to copy over?

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I think you should go ahead with this. It can be adjusted in the future.

Another technique is to use something like #xhgui-generated in the text of the prepend file, and feel free to delete it (using removal_actions) if it contains that. if grep '#xhgui-generated' <phpfile>; then rm <phpfile>; fi

@tyler36 tyler36 force-pushed the 11-Unwilling-to-remove-xhprof_prependphp-because-it-does-not-have-ddev-generated-in-it branch from 0a22a26 to d3c0644 Compare May 28, 2024 01:26
@tyler36
Copy link
Collaborator Author

tyler36 commented May 28, 2024

Rebased.

@tyler36 tyler36 merged commit 4284e94 into main May 28, 2024
2 checks passed
@tyler36 tyler36 deleted the 11-Unwilling-to-remove-xhprof_prependphp-because-it-does-not-have-ddev-generated-in-it branch May 28, 2024 01:46
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.

Unwilling to remove 'xhprof_prepend.php' because it does not have #ddev-generated in it:
3 participants