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

Stats: Update method for inserting JavaScript - Removed fallback / workaround for themes without wp_footer #13182

Conversation

rickcurran
Copy link
Contributor

After discussion on Github issue "Stats: Update method for inserting JavaScript #3763" it was considered that it was a safe assumption to make that a theme would always have a wp_footer action.

This update removes and modifies code which was used to provide a fallback / workaround in the case of themes that did not contain a wp_footer action.

The workaround consisted of setting a global variable $rendered_stats_footer which would get set to true if the wp_footer call was successful, if not successful then the WP Stats JS code would get set via a wp_head action instead.

  • This update modifies code used to load the Jetpack Stats JavaScript and removes an outdated fallback in the case that a theme did not call wp_footer.

After discussion on Github issue "Stats: Update method for inserting JavaScript Automattic#3763" it was considered that it was a safe assumption to make that a theme would always have a `wp_footer` action.

This update removes code which was used to provide a fallback / workaround in the case of themes which did not contain a `wp_footer` action.

The workaround consisted of setting a global variable `$rendered_stats_footer` which would get set to true if the `wp_footer` call was successful, if not successful then the WP Stats JS code would get set via a `wp_head` action instead.
@rickcurran rickcurran marked this pull request as ready for review August 5, 2019 15:39
@rickcurran rickcurran requested a review from a team as a code owner August 5, 2019 15:39
@jetpackbot
Copy link

jetpackbot commented Aug 5, 2019

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: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against ca0f3f7

jeherve
jeherve previously approved these changes Aug 5, 2019
modules/stats.php Outdated Show resolved Hide resolved
@jeherve jeherve dismissed their stale review August 5, 2019 18:07

Clicked the wrong button :)

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. labels Aug 5, 2019
@jeherve jeherve added this to the 7.7 milestone Aug 5, 2019
Fixed incorrect use of spaces instead of tabs
@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Aug 6, 2019
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.

This looks good to me.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 6, 2019
Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

LGTM. Note that this does not address the originally reported SRI issue.

@mdawaffe mdawaffe merged commit 28889b7 into Automattic:master Aug 16, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 16, 2019
jeherve added a commit that referenced this pull request Aug 23, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Pri] Low [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants