search

humanmade / hm-platform

HM Hosting platform required plugins
https://engineering.hmn.md/platform/plugins/
23 stars 4 forks source link

Add final database query to X-Ray #184

Closed Nikschavan closed 4 years ago

Nikschavan commented 4 years ago

See #183

joehoyle commented 4 years ago

Hmm I'm not sure if this is the right approach. Is the filter naturally called higher up in the call stack that wpdb::query? Wondering if we can capture the query at a different point. As it is, something that might hookquery would erroneously see queries that are not actually sent to mysql for querying.

Nikschavan commented 4 years ago

Is the filter naturally called higher up in the call stack that wpdb::query

I would say the filter is run just before the query is executed, Also AFAIK this is the last filter available before the query is executed.

So I am thinking any other queries will not get added to this filter which is not supposed to be executed?

https://github.com/WordPress/WordPress/blob/32edd58e4c8e42b68813e3b92886aaa3e50cc1a9/wp-includes/wp-db.php#L1888-L1912

Another solution to removing the placeholders would be to pass the query from remove_placeholder_escape()

But that will miss if any query is being modified using this query filter.

Let me know what you think would be the right approach.

joehoyle commented 4 years ago

It looks like we could instead replace _do_query() in this subclass, as that gets passed the query after placeholders etc have been replaced. I think that would be a better approach.

Nikschavan commented 4 years ago

Right, that will work.

Also looking more into this, Looks like in the current query() method, instead of the variable $query we can use $this->last_query which is already filtered and will have the final version of the query being executed.

I think I will prefer $this->last_query as this does not need overriding a new method. Do let me know what you prefer and I will change the PR accordingly.

Nikschavan commented 4 years ago

@joehoyle Let me know if you think using $this->last_query would be good and I will update the PR accordingly.

joehoyle commented 4 years ago

@Nikschavan I'm suggesting we change our overriden mathed to _do_query rather than overwriting a new one. I think $this->last_query is less guerenteed, as it might be possible for that value to be something other than the query passed to query() (e.g. future changed, or select found rows() etc.

Nikschavan commented 4 years ago

@joehoyle I have changed the PR, Instead of overriding query() now I am overriding _do_query() as it seems query() was added to this class only to send the segment to X-Ray.

joehoyle commented 4 years ago

I think we should also port this to https://github.com/humanmade/altis-cloud/blob/master/inc/class-db.php

Nikschavan commented 4 years ago

I have a PR on the altis-cloud repo which I will get updated with the new changes here - https://github.com/humanmade/altis-cloud/pull/235

Can you tag a release for hm-platform as well so that I can start using this fix in the project?

nathanielks commented 4 years ago

@Nikschavan 1.4.9 has been tagged 👍