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

Store "Assumed site creation date" in transient #13463

Merged
merged 4 commits into from
Sep 19, 2019

Conversation

dereksmart
Copy link
Member

Changes proposed in this Pull Request:

This PR does two things:

  1. Removes the duplicate Jetpack::get_assumed_site_creation_date() in the main Jetpack class, and replaces it with the package method.
  2. Caches the date in a transient.

This method is called on every Jetpack Admin page load, because it's used in the build_connect_url() method that is passed into the initial state. We don't even need it in most cases (like if the user is already connected) - unless I guess if the user wants to disconnect/reconnect without a page load, so it could be further improved to not pass at all in that case, but not worth the extra complication.

I'm not sure if there was a reason for not caching, but it seems like something that only needs to be checked and stored once.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Slightly improves

Testing instructions:

You can check with a plugin such as Query monitor:

  • Before applying patch, load the Jetpack admin page and note the number of queries.
  • After applying the patch, load the admin page. The first page load may not see signs of improvement, but subsequent page loads will be making fewer queries.

Proposed changelog entry for your changes:

  • Slight performance increase in the admin area? Maybe not worth mentioning?

@dereksmart dereksmart added [Status] Needs Review This PR is ready for review. Admin Page React-powered dashboard under the Jetpack menu [Type] Janitorial labels Sep 14, 2019
@dereksmart dereksmart added this to the 7.8 milestone Sep 14, 2019
@dereksmart dereksmart requested review from tyxla and a team September 14, 2019 14:08
@jetpackbot
Copy link
Collaborator

jetpackbot commented Sep 14, 2019

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against ff2d21e

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is looking good, thank you for the improvement!

I'm only hesitant to directly remove a method vs deprecating it for at least a couple of versions.

Should be good after updating that. Also, left some questions for your consideration.

@@ -4651,7 +4652,7 @@ public static function build_authorize_url( $redirect = false, $iframe = false )
'site_lang' => get_locale(),
'_ui' => $tracks_identity['_ui'],
'_ut' => $tracks_identity['_ut'],
'site_created' => Jetpack::get_assumed_site_creation_date(),
'site_created' => $connection->get_assumed_site_creation_date(),
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the existing connection manager so we don't instantiate a new connection manager if there's one already?

Suggested change
'site_created' => $connection->get_assumed_site_creation_date(),
'site_created' => self::connection()->get_assumed_site_creation_date(),

tyxla
tyxla previously approved these changes Sep 14, 2019
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM, just left a small suggestion for fixing the deprecated notice call.

Feel free to 🚢 once it's solved 👍

tyxla
tyxla previously approved these changes Sep 14, 2019
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again 🚢

@tyxla tyxla added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 14, 2019
@jeherve jeherve force-pushed the update/cache-assumed-site-creation branch from c15e290 to 0cace06 Compare September 19, 2019 10:18
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Sep 19, 2019
@jeherve jeherve force-pushed the update/cache-assumed-site-creation branch from 0cace06 to 9653553 Compare September 19, 2019 10:19
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
@jeherve jeherve force-pushed the update/cache-assumed-site-creation branch from 9653553 to ff2d21e Compare September 19, 2019 10:20
Copy link
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.

Rebased. Looks good now. Merging!

@jeherve jeherve merged commit a8e73e1 into master Sep 19, 2019
@jeherve jeherve deleted the update/cache-assumed-site-creation branch September 19, 2019 10:36
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 19, 2019
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Focus] Performance [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants