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

3rd Party Plugin Compatibility: Query Monitor #686

Closed
EnterpriseBranding opened this issue Feb 25, 2016 · 11 comments
Closed

3rd Party Plugin Compatibility: Query Monitor #686

EnterpriseBranding opened this issue Feb 25, 2016 · 11 comments
Labels
Milestone

Comments

@EnterpriseBranding
Copy link

@EnterpriseBranding EnterpriseBranding commented Feb 25, 2016

I just came across this... and thought I would mention it.

I get two errors from Query Monitor.

See screenshot

These are not reported as "php errors" from Query Monitor, so they don't show up as an issue, unless I read read the query monitor results page.

But thought I would share as I don't know the validity of the error. It says the method doesn't exist.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Feb 25, 2016

@NoahjChampion Are you running ZenCache or Comet Cache? Which version?

@EnterpriseBranding
Copy link
Author

@EnterpriseBranding EnterpriseBranding commented Feb 25, 2016

@raamdev Zencache.

I know about the change to Comet Cache, and have the version that states that from the Zencache page, but not sure how to update to Comet Cache just yet. I didn't dig into it yet. Do I have to manually download Comet Cache.

It all happened quickly. I didn't think Comet Cache would be out already :)

@jaswrks
Copy link

@jaswrks jaswrks commented Feb 25, 2016

@NoahjChampion Take a look at the "Migrating from ZenCache to Comet Cache" section here. https://cometcache.com/blog/announcing-comet-cache-formerly-zencache/

@EnterpriseBranding
Copy link
Author

@EnterpriseBranding EnterpriseBranding commented Feb 25, 2016

@jaswsinc thanks. Done.

I took the time to look over all the query monitor results and there are 19 errors, all of the same kind of reference.

Here is a screenshot of all those errors together @ http://take.ms/sNGqL

@EnterpriseBranding EnterpriseBranding changed the title Error: Method WebSharks\ZenCache\Pro\Plugin::allAdminNotices() does not exist Error: Method WebSharks\CometCache\Pro\Plugin:: ..... does not exist Feb 25, 2016
@raamdev raamdev added this to the Next Release milestone Feb 25, 2016
@raamdev
Copy link
Contributor

@raamdev raamdev commented Feb 25, 2016

I can confirm I'm seeing the same thing with the Query Monitor plugin, however I'm wondering if this is related specifically to how the Query Monitor plugin acquires these results (i.e., is it running each of these hooks itself and then checking the result? and maybe those Comet Cache calls are not meant to be run in that context?).

This will require a bit more research.

Any ideas @jaswsinc?

@raamdev
Copy link
Contributor

@raamdev raamdev commented Feb 26, 2016

2016-02-26_10-04-22
2016-02-26_10-04-07

@raamdev raamdev changed the title Error: Method WebSharks\CometCache\Pro\Plugin:: ..... does not exist 3rd Party Plugin Compatibility: Query Monitor Feb 26, 2016
@raamdev
Copy link
Contributor

@raamdev raamdev commented Feb 26, 2016

After some digging into how Query Monitor works and finding where it populates the callback handler, we were able to narrow this down to an issue with how the Comet Cache codebase is currently doing some tricky things with PHP, which was necessary (until recently) to maintain compatibility with PHP 5.3.

Now that we have announced PHP 5.4+ is required to run Comet Cache, we will be updating the codebase to convert this "tricky" code to use PHP Traits, which should make the codebase fully compatible with with PHP Reflection class that Query Monitor is using.


A few more technical details:

@jaswsinc writes...

Yeah, I see what the issue is. They are doing things right, so it's not a bug in their software. This is a PHP limitation.

On this line: https://github.com/johnbillion/query-monitor/blob/2.8.1/classes/Util.php#L166 they are interpreting an array (e.g., [$this, 'clearCache']) as a class Method instead of as a Closure. That's right of them to do that.

However, this is actually incorrect within Comet Cache, because in reality we are performing some magic in order to work around not being able to use PHP 5.4 code. In our codebase, Closures magically become Methods via __call() magic, and so you can't create a ReflectionMethod out of them, because they are defined as Closures and not as class methods. So even though their codebase is checking for both Methods and for Closures, we are able to fool them just enough for it to cause this problem. No easy way around this issue, because they can't predict what magic we might perform in the __call() handler.

What will resolve this is moving to Traits.

Actually, we don't even use the __get() magic in Comet Cache, we are using $self with dynamically generated methods, which is even more magical. Magical, in the sense that it can lead to some difficulty when trying to guess where a method comes from with any precision, which is what they are trying to do.


So, in summary, once the work in Issue #635 is complete (this is high on our development list), this compatibility issue with Query Monitor should go away.

@raamdev raamdev modified the milestones: Future Release, Next Release Feb 26, 2016
@raamdev raamdev mentioned this issue Feb 26, 2016
5 of 5 tasks complete
@EnterpriseBranding
Copy link
Author

@EnterpriseBranding EnterpriseBranding commented Feb 26, 2016

👍

@raamdev
Copy link
Contributor

@raamdev raamdev commented Mar 9, 2016

Confirming that the switch to PHP Traits has fixed this issue with Query Monitor (this is a screenshot while running the latest dev build):

2016-03-09_14-04-58

@raamdev raamdev removed the blocked label Mar 9, 2016
@raamdev
Copy link
Contributor

@raamdev raamdev commented Mar 9, 2016

Next Release Changelog:

  • Compatibility: Query Monitor. The Query Monitor plugin was reporting false-positive errors indicating that many Comet Cache methods did not exist. This was due to how the Comet Cache codebase was utilizing PHP Closures, which Query Monitor had a hard time handling. The codebase has been refactored to use PHP Traits instead of Closures and now the Query Monitor plugin has no problem recognizing Comet Cache methods. Props to @NoahjChampion for reporting. See Issue #686.
@raamdev raamdev closed this Mar 9, 2016
@raamdev
Copy link
Contributor

@raamdev raamdev commented Apr 16, 2016

Comet Cache v160416 has been released and includes changes from this GitHub Issue. See the v160416 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#686).

@wpsharks wpsharks locked and limited conversation to collaborators Apr 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants