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

Add WP Profiler mu-plugin #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add WP Profiler mu-plugin #42

wants to merge 1 commit into from

Conversation

joemcgill
Copy link
Collaborator

This adds an example mu-plugin that can be used for configuring a WordPress environment set up using the wp-env package with XHProf support (related PR).

Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@joemcgill Thank you, I left some feedback. Overall it looks good, though I wonder whether we could work towards having a profiling environment in this repository (not necessarily all in this one PR) rather than only the wp-profiler plugin.

"WP_DEBUG_DISPLAY": false,
"SCRIPT_DEBUG": false
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having this file here seems a bit out of scope to me as it's for the environment, and the plugin folder itself does not make that environment work.

I think it would be great to implement a simple wp-env configuration in this repository for XHProf so that it's standalone. Of course for XHProf that requires us to first merge your PR WordPress/gutenberg#48147.

My suggestion would be to, instead of a general plugins folder and this plugin, add a folder like wp-profiling-env which includes a basic package.json with the necessary dependencies and scripts to run wp-env, and then an mu-plugins folder with the wp-profiler plugin you have here, as that is most useful as part of that environment.

WDYT? We could of course split that work apart, since right now @wordpress/env does not yet support the necessary XHProf work, but I think eventually this would be a better outcome, have a standalone solution in here rather than only the MU plugin and require the rest to be set up manually.

@@ -0,0 +1,34 @@
<?php
require __DIR__ . '/vendor/autoload.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit-pick? Should we have a simple plugin header here?

} catch (Exception $e){
// throw away or log error about profiling instantiation failure
error_log($e->getMessage());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit-pick: This file doesn't follow WP coding standards (mostly indentation and spacing-wise).

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.

2 participants