Skip to content

WordAds: Do not try to inject ad when in a feed#7095

Merged
dereksmart merged 2 commits intomasterfrom
fix/wordads-feed
May 17, 2017
Merged

WordAds: Do not try to inject ad when in a feed#7095
dereksmart merged 2 commits intomasterfrom
fix/wordads-feed

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented May 1, 2017

WordAds will inject the inline JS into the_content and the_excerpt. This won't work very well since it's XML and none of the header or scripts hooks are firing :)

The PR will bail early if being ran while is_feed() is true.

Real like failure (from https://micro.blog/bjk , powered via RSS feed from kraft.blog):
screen shot 2017-04-30 at 22 04 14

XML before patch:
screen shot 2017-04-30 at 21 59 10

XML after patch:
screen shot 2017-04-30 at 22 00 00

@kraftbj kraftbj added [Feature] WordAds [Pri] Normal [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels May 1, 2017
@kraftbj kraftbj requested a review from dbspringer May 1, 2017 03:06
jeherve
jeherve requested changes May 1, 2017
View reviewed changes
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.

Minor nit-picking

modules/wordads/wordads.php Outdated
*/
function insert_ad( $content ) {
// Ad JS won't work in XML feeds.
if ( is_feed() ){
Copy link
Copy Markdown
Member

@jeherve jeherve May 1, 2017

Choose a reason for hiding this comment

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

nit-picking, but could you add a space after the closing parenthesis to make phpcs happy?

Thanks!

Copy link
Copy Markdown
Contributor Author

@kraftbj kraftbj May 1, 2017

Choose a reason for hiding this comment

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

Resolved.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 1, 2017
dbspringer
dbspringer previously requested changes May 1, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@dbspringer dbspringer left a comment

Choose a reason for hiding this comment

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

Ah, good catch. Though I'd recommend we add it to the should_bail() (here) function so we skip everything as early as possible. e.g.:

public function should_bail() {
	return ! $this->option( 'wordads_approved' ) || is_feed();
}

@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented May 1, 2017

@dbspringer I don't think we can use is_feed there. Looks like that's running on the init hook, which is too early for the query conditional functions.

@kraftbj kraftbj added [Status] In Progress and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 1, 2017
@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented May 1, 2017

Marking as in progress while we debate the best way to bail.

@dbspringer
Copy link
Copy Markdown
Member

dbspringer commented May 1, 2017

In that case, just make sure you're also doing a check for the other spots we try to load stuff, the add_action|filter calls in insert_adcode() is a good place to see what's getting addded.

@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented May 1, 2017

Cool. I think we're good. The rest of those don't run on feeds

@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented May 1, 2017

@jeherve If everything else looks good, can you mark as reviewed or in need of review?

@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label May 1, 2017
jeherve
jeherve approved these changes May 1, 2017
View reviewed changes
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.

LGTM!

@jeherve jeherve 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 May 1, 2017
@dereksmart dereksmart merged commit dce59b0 into master May 17, 2017
@dereksmart dereksmart deleted the fix/wordads-feed branch May 17, 2017 15:54
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 17, 2017
jeherve added a commit that referenced this pull request May 23, 2017
@jeherve
Changelog: add #7095
51743ec
jeherve added a commit that referenced this pull request May 29, 2017
@jeherve
Changelog: add #7095
ebe5ec6
eliorivero pushed a commit that referenced this pull request May 30, 2017
@jeherve @eliorivero
Changelog: first pass at a changelog for 5.0 (#7116)
eee27f0 
* Changelog: first pass at a changelog for 5.0

* Changelog: delete 4.9 testing list.

* Changelog: update minimum WP version to match ver. in jetpack.php

Fixes #7158

* Changelog: add #6051

* Changelog: add #6753

* Changelog: add #6928

* Changelog: add #6964

* Changelog: add #7014

* Changelog: add #7057

* Changelog: add #7060

* Changelog: add #7068

* Changelog: add #7070

* Changelog: add #7072

* Changelog: add #7071

* Changelog: add release date and post shortlink.

* Changelog: add #7094

* Changelog: add #7100

* Changelog: add #7108

* Changelog: add #7113

* Changelog: add #7123

* Changelog: add #7135

* Changelog: add #7143

* Changelog: add #7151

* Changelog: add #6996

* Changelog: add #7105

* Changelog: add #7132

* Changelog: add #7166

* Changelog: fix typo in 4.9 changelog.

* Changelog: remove older releases' changelogs.

@see p1HpG7-42e-p2

* Changelog: add #7090

* Changelog: add #7095

* Changelog: add #7112

* Changelog: add #7115

* Changelog: add #7122

* Changelog: add #7137

* Changelog: add #7138

* Changelog: add #7140

* Changelog: add #7154

* Changelog: add ##7155

* Changelog: add #7163

* Changelog: add #7167

* Changelog: add #7171

* Changelog: add #7180

* Changelog: add #7181

* Changelog: add #7183

* Changelog: add #7184

* Changelog: add #7189

* Changelog: add #7191

* Changelog: add #7193

* Changelog: add #7198

* Changelog: add #7200

* Changelog: add #7209

* Changelog: add #7212

* Testing list: add instructions for #7115

* Changelog: add #7188

* Changelog: add #7205

* Changelog: add #7225

* Changelog: add #6872

* Changelog: add #7107

* Changelog: add #7118

* Changelog: add #7142

* Changelog: add #7170

* Changelog: add #7210

* Changelog: add #7218

* Changelog: add #7232

* Changelog: add #7211

* Changelog: add #7213

* Changelog: add #7229

* Changelog: add #7230

* Changelog: add #7214

* Draft changelog for 5.0

* Changelog updates: 2nd pass at a clearer changelog.

- Fix typos.
- Use consistent tense and tone across all changelog.
- Remove unclear items.

* Changelog: add #7026

* Changelog: add #7058

* Changelog: add #7125

* Changelog: add #7249

* Changelog: add #7185

* add mentions of image widget migration

* Changelog: add info about new output for CLI command.

* Changelog: add WP version number matching the new Image Widget.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@dbspringer dbspringer dbspringer left review comments

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] WordAds [Pri] Normal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kraftbj @dbspringer @jeherve @dereksmart