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

WordAds Block: AMP Support #12733

Merged
merged 14 commits into from
Aug 26, 2019
Merged

WordAds Block: AMP Support #12733

merged 14 commits into from
Aug 26, 2019

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Jun 18, 2019

Changes proposed in this Pull Request:

This PR brings AMP compatibility to the Jetpack Ads block.

A couple of gotchas:

  1. The Additional ad placements->Top of each page option will lead to AMP incompatibility in TwentySeventeen, TwentyFifteen, and TwentyFourteen. This is due to https://github.com/Automattic/jetpack/blob/master/modules/wordads/wordads.php#L400-L402. I plan to address this in a separate PR since this doesn't directly relate to the block.
  2. AMP ads will be blocked if Settings->Reading->Search Engine Visibility->Discourage search engines from indexing this site is checked.
  3. Make sure you have no ad blockers enabled (I fell for this one more than once :-) )

Testing instructions:

Proposed changelog entry for your changes:

  • AMP compatibility for the Jetpack Ads block.

@jeffersonrabb jeffersonrabb requested a review from a team June 18, 2019 17:58
@jeffersonrabb jeffersonrabb added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jun 18, 2019
@jeffersonrabb jeffersonrabb self-assigned this Jun 18, 2019
@jetpackbot
Copy link

jetpackbot commented Jun 18, 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 2349a05

@dbspringer
Copy link
Member

One question I have is: there's a few different AMP implementations available for self-hosted sites, does this one target one in particular, or is it more of a generic solution? We may need to consider supporting a handful of the most popular AMP options.

Though I suppose if a different version of AMP is used our is_amp checks will just return false.

@jeffersonrabb
Copy link
Contributor Author

One question I have is: there's a few different AMP implementations available for self-hosted sites, does this one target one in particular, or is it more of a generic solution? We may need to consider supporting a handful of the most popular AMP options.

By "different AMP implementations" do you mean plugins other than the official one: https://wordpress.org/plugins/amp/ ?

Copy link
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.

Generally looks good, just a question about which AMP implementations we need to support.

modules/wordads/wordads.php Outdated Show resolved Hide resolved
@@ -496,6 +514,17 @@ function get_ad_snippet( $section_id, $height, $width, $location = '', $css = ''
if ( ! empty( self::$ad_location_ids[ $location ] ) ) {
$loc_id = self::$ad_location_ids[ $location ];
}
if ( class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request() ) {
Copy link
Member

Choose a reason for hiding this comment

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

We may want to consider extracting this into a separate is_amp function to include support for various versions of AMP solutions self-hosted sites might use.

Unless we're firm on only supporting the Jetpack_AMP_Support version, then nevermind :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted in #12733 (comment), let's see what Jetpack folks think?

@dbspringer
Copy link
Member

Looks like there's at least three different implementations w/ > 10,000 active installs. Maybe if there's an "official" one we can just say that's the one we support.

Screen Shot 2019-06-18 at 11 15 09 AM

@jeffersonrabb
Copy link
Contributor Author

Looks like there's at least three different implementations w/ > 10,000 active installs. Maybe if there's an "official" one we can just say that's the one we support.

Yeah, interesting point. This check https://github.com/Automattic/jetpack/blob/master/3rd-party/class.jetpack-amp-support.php#L56-L72 is already used pretty widely throughout Jetpack, but I'll leave it to @Automattic/jetpack-crew to comment on whether we might need to broaden support?

@westonruter
Copy link
Contributor

Maybe if there's an "official" one we can just say that's the one we support.

There is an official one, and it is the one being directly supported: https://wordpress.org/plugins/amp/

This is what runs on WordPress.com

Other plugins that want to shim support can do so via the filter exposed in Jetpack_AMP_Support.

@westonruter
Copy link
Contributor

Also, the AMP for WP plugin includes an old version of the official AMP plugin, so the same conditionals should work.

@dbspringer
Copy link
Member

dbspringer commented Jun 18, 2019

Regardless of whether or not we add support for other versions, I think it makes sense to extract the check into a separate function -- which I did :)

<amp-ad width="$width" height="$height"
type="pubmine"
data-siteid="$site_id"
data-section="$amp_section_id">
Copy link
Contributor

Choose a reason for hiding this comment

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

These should use esc_attr() no?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, added.

HTML;
}

$ad_number = count( $this->ads ) . '-' . uniqid();
Copy link
Contributor

Choose a reason for hiding this comment

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

Using uniqid breaks the AMP plugin's post-processor cache. Better to use https://developer.wordpress.org/reference/functions/wp_unique_id/ or a polyfill.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though I suppose this won't run in AMP anyway since it's after the return?

Copy link
Member

@dbspringer dbspringer Jun 18, 2019

Choose a reason for hiding this comment

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

Though I suppose this won't run in AMP anyway since it's after the return?

That's correct.

dbspringer
dbspringer previously approved these changes Jun 18, 2019
Copy link
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.

Okay, I think this update is now a solid enough first release while the other updates noted get worked on.

@jeffersonrabb
Copy link
Contributor Author

  1. The Additional ad placements->Top of each page option will lead to AMP incompatibility in TwentySeventeen, TwentyFifteen, and TwentyFourteen. This is due to https://github.com/Automattic/jetpack/blob/master/modules/wordads/wordads.php#L400-L402. I

This is resolved in 877f0cd. For AMP requests, the unit will be inserted above the content, below the title. After discussing with @dbspringer we decided there wasn't any way to match the non-AMP positioning without using Javascript. This isn't precisely the same as the non-AMP placement, but is similar in spirit.

@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Jun 20, 2019
@jeherve jeherve added this to the 7.5 milestone Jun 20, 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.

I'm getting the following notice when trying to view a post in an AMP view:

PHP Notice:  is_amp_endpoint was called <strong>incorrectly</strong>. is_amp_endpoint() was called before the parse_query hook was called. Please see <a href="https://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 0.4.2.) in /var/www/html/wp-includes/functions.php on line 4773
[20-Jun-2019 13:49:11 UTC] PHP Stack trace:
[20-Jun-2019 13:49:11 UTC] PHP   1. {main}() /var/www/html/index.php:0
[20-Jun-2019 13:49:11 UTC] PHP   2. require() /var/www/html/index.php:17
[20-Jun-2019 13:49:11 UTC] PHP   3. require_once() /var/www/html/wp-blog-header.php:13
[20-Jun-2019 13:49:11 UTC] PHP   4. require_once() /var/www/html/wp-load.php:37
[20-Jun-2019 13:49:11 UTC] PHP   5. require_once() /var/www/html/wp-config.php:85
[20-Jun-2019 13:49:11 UTC] PHP   6. do_action() /var/www/html/wp-settings.php:525
[20-Jun-2019 13:49:11 UTC] PHP   7. WP_Hook->do_action() /var/www/html/wp-includes/plugin.php:465
[20-Jun-2019 13:49:11 UTC] PHP   8. WP_Hook->apply_filters() /var/www/html/wp-includes/class-wp-hook.php:310
[20-Jun-2019 13:49:11 UTC] PHP   9. WordAds->init() /var/www/html/wp-includes/class-wp-hook.php:286
[20-Jun-2019 13:49:11 UTC] PHP  10. WordAds->insert_adcode() /var/www/html/wp-content/plugins/jetpack/modules/wordads/wordads.php:156
[20-Jun-2019 13:49:11 UTC] PHP  11. WordAds::is_amp() /var/www/html/wp-content/plugins/jetpack/modules/wordads/wordads.php:235
[20-Jun-2019 13:49:11 UTC] PHP  12. Jetpack_AMP_Support::is_amp_request() /var/www/html/wp-content/plugins/jetpack/modules/wordads/wordads.php:78
[20-Jun-2019 13:49:11 UTC] PHP  13. is_amp_endpoint() /var/www/html/wp-content/plugins/jetpack/3rd-party/class.jetpack-amp-support.php:62
[20-Jun-2019 13:49:11 UTC] PHP  14. _doing_it_wrong() /var/www/html/wp-content/plugins/amp/includes/amp-helper-functions.php:275
[20-Jun-2019 13:49:11 UTC] PHP  15. trigger_error() /var/www/html/wp-includes/functions.php:4773

Previous art on that topic: #11195

@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! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 20, 2019
@jeffersonrabb
Copy link
Contributor Author

@dbspringer any issue with skipping insert_head_iponweb() and insert_head_meta() in AMP environments? 0e1a7ed

@dbspringer
Copy link
Member

dbspringer commented Jun 20, 2019

Yeah, totally fine 👍(I'm pretty sure AMP would just strip them out anyway)

@jeherve
Copy link
Member

jeherve commented Jun 20, 2019

Rebasing to fix tests.

@jeffersonrabb
Copy link
Contributor Author

Rebased.

@jeffersonrabb jeffersonrabb added [Status] Needs Review To request a review from fellow Jetpack developers. 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 Jul 26, 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.

I am now getting the following errors when editing a post that contains Ad blocks:

Notice:  Trying to get property 'blog_id' of non-object in wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php on line 93
Notice:  Trying to get property 'cloudflare' of non-object in wp-content/plugins/jetpack-dev/modules/wordads/wordads.php on line 566
Notice:  Trying to get property 'blog_id' of non-object in wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php on line 93
Notice:  Trying to get property 'cloudflare' of non-object in wp-content/plugins/jetpack-dev/modules/wordads/wordads.php on line 566
Notice:  Trying to get property 'blog_id' of non-object in wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php on line 93
Notice:  Trying to get property 'cloudflare' of non-object in wp-content/plugins/jetpack-dev/modules/wordads/wordads.php on line 566
Warning:  Cannot modify header information - headers already sent by (output started at wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php:93) in wp-includes/option.php on line 947
Warning:  Cannot modify header information - headers already sent by (output started at wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php:93) in wp-includes/option.php on line 948

$wordads->params appears to be null.

This can even cause a Fatal error if you hide one of the ads on mobile using the block option:

Fatal error: Uncaught Error: Call to a member function is_mobile() on null
in wp-content/plugins/jetpack-dev/extensions/blocks/wordads/wordads.php on line 83

This does not appear on Jetpack Stable.

@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! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 26, 2019
@jeffersonrabb
Copy link
Contributor Author

Hmmm, this is tricky. Looks like the new Block fatals came about because of the change in the init action from init to wp in 65ae218. @westonruter, given this change was made to address the AMP _doing_it_wrong error (#12733 (comment)) do you have any suggestions for how to reconcile these two situations?

@westonruter
Copy link
Contributor

Why do the fatals occur due to moving the logic from init to wp, an action that happens later in WordPress's execution?

@jeffersonrabb
Copy link
Contributor Author

After discussing with @westonruter 2349a05 should resolve the issue by using different initialization hooks for API and non-API requests.

@jeffersonrabb jeffersonrabb added [Status] Needs Review To request a review from fellow Jetpack developers. 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 5, 2019
@jeherve
Copy link
Member

jeherve commented Aug 6, 2019

For the most part it seems to work well now. I do, however, run into Fatals from AMP now:

[06-Aug-2019 17:04:24 UTC] PHP Fatal error:  Unknown: Cannot use output buffering in output buffering display handlers in Unknown on line 0
[06-Aug-2019 17:04:24 UTC] PHP Stack trace:
[06-Aug-2019 17:04:24 UTC] PHP   1. shutdown_action_hook() /var/www/html/wp-includes/load.php:0
[06-Aug-2019 17:04:24 UTC] PHP   2. do_action() /var/www/html/wp-includes/load.php:954
[06-Aug-2019 17:04:24 UTC] PHP   3. WP_Hook->do_action() /var/www/html/wp-includes/plugin.php:465
[06-Aug-2019 17:04:24 UTC] PHP   4. WP_Hook->apply_filters() /var/www/html/wp-includes/class-wp-hook.php:310
[06-Aug-2019 17:04:24 UTC] PHP   5. wp_ob_end_flush_all() /var/www/html/wp-includes/class-wp-hook.php:286
[06-Aug-2019 17:04:24 UTC] PHP   6. ob_end_flush() /var/www/html/wp-includes/functions.php:4339
[06-Aug-2019 17:04:24 UTC] PHP   7. AMP_Theme_Support::finish_output_buffering() /var/www/html/wp-includes/functions.php:4339
[06-Aug-2019 17:04:25 UTC] PHP   8. AMP_Theme_Support::prepare_response() /var/www/html/wp-content/plugins/amp/includes/class-amp-theme-support.php:1766
[06-Aug-2019 17:04:25 UTC] PHP   9. AMP_Content_Sanitizer::sanitize_document() /var/www/html/wp-content/plugins/amp/includes/class-amp-theme-support.php:2093
[06-Aug-2019 17:04:25 UTC] PHP  10. AMP_Style_Sanitizer->sanitize() /var/www/html/wp-content/plugins/amp/includes/templates/class-amp-content-sanitizer.php:117
[06-Aug-2019 17:04:25 UTC] PHP  11. AMP_Style_Sanitizer->process_link_element() /var/www/html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php:731
[06-Aug-2019 17:04:25 UTC] PHP  12. AMP_Style_Sanitizer->process_stylesheet() /var/www/html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php:1186
[06-Aug-2019 17:04:25 UTC] PHP  13. AMP_Style_Sanitizer->prepare_stylesheet() /var/www/html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php:1315
[06-Aug-2019 17:04:25 UTC] PHP  14. AMP_Style_Sanitizer->parse_stylesheet() /var/www/html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php:1565
[06-Aug-2019 17:04:25 UTC] PHP  15. Sabberworm\CSS\Parser->parse() /var/www/html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php:1480
[06-Aug-2019 17:04:25 UTC] PHP  16. Sabberworm\CSS\CSSList\Document::parse() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/Parser.php:38
[06-Aug-2019 17:04:25 UTC] PHP  17. Sabberworm\CSS\CSSList\CSSList::parseList() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/CSSList/Document.php:21
[06-Aug-2019 17:04:25 UTC] PHP  18. Sabberworm\CSS\CSSList\CSSList::parseListItem() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/CSSList/CSSList.php:49
[06-Aug-2019 17:04:25 UTC] PHP  19. Sabberworm\CSS\RuleSet\DeclarationBlock::parse() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/CSSList/CSSList.php:98
[06-Aug-2019 17:04:25 UTC] PHP  20. Sabberworm\CSS\Parsing\ParserState->consumeUntil() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/RuleSet/DeclarationBlock.php:31
[06-Aug-2019 17:04:25 UTC] PHP  21. Sabberworm\CSS\Parsing\ParserState->consumeComment() /var/www/html/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/Parsing/ParserState.php:225

@westonruter Does this ring a bell?

@westonruter
Copy link
Contributor

@jeherve Humm. Are you getting any other error in the log?

What is the MEMORY_LIMIT in your install? I've been able to reproduce that problem if I lower my MEMORY_LIMIT: ampproject/amp-wp#1915 (comment)

When testing with @jeffersonrabb we didn't see this issue.

@jeffersonrabb
Copy link
Contributor Author

What is the MEMORY_LIMIT in your install?

If this is a memory issue, wouldn't we expect to see the same on Jetpack stable?

@jeherve
Copy link
Member

jeherve commented Aug 7, 2019

What is the MEMORY_LIMIT in your install?

64M, but I see that I have the same problem with master, so whatever is happening on my site is not caused by this branch.

I'll let someone else jump in and test this instead of me.

@jeherve jeherve added this to the 7.7 milestone Aug 7, 2019
@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 fellow Jetpack developers. Label will be renamed soon. labels Aug 26, 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 tests well for me today. Merging.

@jeherve jeherve merged commit e12fef4 into master Aug 26, 2019
@jeherve jeherve deleted the add/amp-ads-support branch August 26, 2019 13:42
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 26, 2019
jeherve added a commit that referenced this pull request Aug 26, 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
AMP [Feature] WordAds [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.

7 participants