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

Remove the option for disabling NREUM on AMP pages #11

Merged
merged 2 commits into from
Mar 22, 2017

Conversation

goldenapples
Copy link
Contributor

There's no context where you would want to include New Relic scripts in
AMP pages, so it doesn't make sense to expose this as an option. This
removes the option, and just hooks disable_nr_autorum() onto the
pre_amp_render_post action, so that any pages which are served through
the AMP plugin will not include the browser monitoring instrument
script.

There's no context where you would want to include New Relic scripts in
AMP pages, so it doesn't make sense to expose this as an option. This
removes the option, and just hooks `disable_nr_autorum()` onto the
`pre_amp_render_post` action, so that any pages which are served through
the AMP plugin will not include the browser monitoring instrument
script.
@goldenapples
Copy link
Contributor Author

Fixes #7.

Doesn't address the case noted in #10 (comment), but that can't be addressed in this plugin since the plugin isn't loaded at the time that batcache intervenes.

@goldenapples goldenapples requested a review from nicholasio March 22, 2017 20:49
@goldenapples
Copy link
Contributor Author

@nicholasio Can you review/merge this for me? It just removes a meaningless option from the plugin's settings fields.

Copy link
Member

@nicholasio nicholasio left a comment

Choose a reason for hiding this comment

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

Looks Good

@nicholasio nicholasio merged commit 2fcce23 into 10up:master Mar 22, 2017
@goldenapples goldenapples deleted the 7-remove-disable-for-amp-option branch March 22, 2017 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants