Skip to content

Fix: Unnecessary query calls without WooCommerce - PR ready for review#10664

Merged
greenafrican merged 3 commits intoAutomattic:masterfrom
tonytettinger:fix/woocommerce-queries
Nov 27, 2018
Merged

Fix: Unnecessary query calls without WooCommerce - PR ready for review#10664
greenafrican merged 3 commits intoAutomattic:masterfrom
tonytettinger:fix/woocommerce-queries

Conversation

@tonytettinger
Copy link
Copy Markdown
Contributor

@tonytettinger tonytettinger commented Nov 19, 2018

As the issue #9736 states, there are a number of unnecessary query calls from the wp-woocommerce-analytics.php.

Changes proposed in this Pull Request:

I have changed the order of the calling of the functions, to make the check if WooCommerce is installed/active to avoid other functions calls that result in unnecessary query calls. As it was mentioned in the issue description there might be other ways to check if the plugin is active, I have changed (and tested) the function to is_plugin_active() . Not sure if it has any effect on the performance, but it's less code and works well.

Testing instructions:

I recommend testing with the Query Monitor Plugin.
Check the Query Monitor > Queries by Component > Plugin: jetpack menu, with WooCommerce activated and deactivated. The mentioned queries should only show up when it's activated.

WooCommerce Activated:
queryoriginal17

WooCommerce Deactivated:
queryafterfix

Proposed changelog entry for your changes:

None required.

@tonytettinger tonytettinger requested a review from a team November 19, 2018 03:49
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Nov 19, 2018

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 26, 2018.
Scheduled code freeze: November 19, 2018

Generated by 🚫 dangerJS

@tonytettinger tonytettinger changed the title Fix: Unnecessary query calls without WooCommerce Fix: Unnecessary query calls without WooCommerce [Status] Needs Review Nov 19, 2018
@tonytettinger tonytettinger changed the title Fix: Unnecessary query calls without WooCommerce [Status] Needs Review Fix: Unnecessary query calls without WooCommerce - PR Ready for review Nov 19, 2018
@tonytettinger tonytettinger changed the title Fix: Unnecessary query calls without WooCommerce - PR Ready for review Fix: Unnecessary query calls without WooCommerce - PR ready for review Nov 19, 2018
@jeherve jeherve added [Feature] WooCommerce Analytics [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Nov 20, 2018
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I would revert to using the Jetpack function in this case.

*
* This action is documented in https://docs.woocommerce.com/document/create-a-plugin
*/
if ( !is_plugin_active( 'woocommerce/woocommerce.php' ) ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would recommend that you keep using Jetpack::get_active_plugins() here; it accounts for more use-cases than is_plugin_active (e.g. multisite).

You can check the get_active_plugins function in class.jetpack.php to find out more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @jeherve I have checked it out and the change is reverted to Jetpack's get_active_plugins() function.

@jeherve jeherve added [Focus] Performance [Status] Needs Review This PR is ready for review. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. labels Nov 20, 2018
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me. 👍

@greenafrican Would you mind taking a look as well, and merge if this looks good to you?

Thank you!

@greenafrican greenafrican merged commit 5189847 into Automattic:master Nov 27, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 27, 2018
@tonytettinger tonytettinger deleted the fix/woocommerce-queries branch December 3, 2018 01:52
jeherve added a commit that referenced this pull request Dec 19, 2018
jeherve added a commit that referenced this pull request Jan 3, 2019
jeherve added a commit that referenced this pull request Jan 3, 2019
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants