-
Notifications
You must be signed in to change notification settings - Fork 52
Query Monitor support #212
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
Conversation
81c83c2 to
4dfca33
Compare
2e03007 to
09cdd0f
Compare
09cdd0f to
5c2265e
Compare
| } | ||
|
|
||
| // Query monitor integration: | ||
| if ( $query_monitor_active && class_exists( 'QM_Backtrace' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could use the QM_DB class directly without recreating its logic here? I see it's class QM_DB extends wpdb. Perhaps we could provide it with a version of wpdb that uses SQLite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Inspired by what WP CLI does with wp-config.php, I guess we could try to read the QB_DB contents, change the extends ... clause, and then eval it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nono, that will break on plugin update. I've meant not including the root wpdb and providing our own class with that name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Oh, but we're extending wpdb, we need to load it, at least under a different name — and I guess that's not really doable without actually rewriting it at some point.
What I meant is adjusting QM_DB on the fly. This could be fine, as when QM is active, we don't expect performance to be critical anyway. Something like:
$contents = file_get_contents( '.../QM_DB.php' );
$contents = str_replace( '<?php', '', $contents );
$contents = str_replace( 'extends wpdb', 'extends WP_SQLite_DB', $contents );
eval( $contents );
$wpdb = new QM_DB( ... );But maybe it's better to suggest some improvements upstream so we can reuse some code without the hacks? For example, I think that making the inheritance configurable can be done pretty simply:
$wpdb_class = defined( 'WPDB_CLASS' ) && class_exists( WPDB_CLASS ) ? WPDB_CLASS : 'wpdb';
class_alias( $wpdb_class, 'QM_WPDB' );
class QM_DB extends QM_WPDB { ... }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream improvement sound like a great idea. Until then, this PR looks solid. Thank you 👍
adamziel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one idea but otherwise I think it's a good solution. Thank you @JanJakes! Is there any way we could add this to our test suite? Ideally not via Playground, just to separate the tool from its testing infrastructure.
Follows up in #212. Accounts for the WordPress Playground runtime, where the sapi name is `cli` and the `$wpdb` variable [may not be available](https://github.com/WordPress/wordpress-playground/blob/c27a05386ce480c19a67be316d9bc093ffeec256/packages/playground/wordpress/src/index.ts#L391-L462). ## Testing instructions Just confirm it doesn't break CI. We don't have an easy way of running this PR in Playground (simply installing the plugin will trigger different code paths).
This PR integrates Query Monitor so that it can be used together with the SQLite Database Integration plugin, and it extends the Query Monitor panel to include SQLite query information.
This implementation uses some tricks and workarounds to achieve this functionality. We could improve the integration by proposing upstream changes to the Query Monitor plugin.
The
wp-content/db.phpissueThe main issue to overcome was that both plugins rely on the usage of the
wp-content/db.phpdatabase drop-in file. To resolve this, I’ve enabled the SQLite plugin to override thedb.phpfile created by Query Monitor while also ensuring it eagerly boots Query Monitor when needed. The behavior depends on which plugin is activated first:db.phpfile will never be replaced by Query Monitor. The SQLite plugin will detect when Query Monitor is active and boot it eagerly while also storing detailed query information for Query Monitor to consume.db.phpfile, and it will retain the eager boot logic for Query Monitor. The user will be notified about thedb.phpfile replacement.Extended query information for Query Monitor
The SQLite plugin now stores enhanced query information for Query Monitor when it’s active, effectively mirroring a tiny portion of the Query Monitor code. To reduce duplication, we can propose changes to the Query Monitor codebase that would allow this logic to be reused rather than reimplemented.
Extending the Query Monitor panel
The Query Monitor panel was extended to provide information about the executed SQLite queries for each MySQL query. This makes it easy to inspect and debug which SQLite queries correspond to each MySQL statement.
Currently, Query Monitor doesn’t offer a way to customize the rendered query information. To overcome this limitation, I implemented this by capturing the generated HTML and modifying it before it is sent to the browser. This is another area where we could improve integration by proposing upstream changes to the Query Monitor plugin.
Playground support
Playground loads the SQLite Database Integration differently, without creating a
db.phpfile. As a result, third-party plugins can still inject theirdb.phpdrop-in. For Query Monitor, I’ve added a simple fix. Setting theQM_DB_SYMLINKconstant tofalseprevents it from using thedb.phpdrop-in, and the SQLite plugin will boot it as needed.