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

AMP Auto Ads Compatibility with AMP for WP plugin #971

Closed
ernee opened this issue Dec 11, 2019 · 13 comments
Closed

AMP Auto Ads Compatibility with AMP for WP plugin #971

ernee opened this issue Dec 11, 2019 · 13 comments
Labels
Good First Issue Good first issue for new engineers P2 Low priority Type: Bug Something isn't working
Milestone

Comments

@ernee
Copy link

ernee commented Dec 11, 2019

When using the AMP for WP plugin on a site implementing AMP Auto Ads, the following validation error is thrown on AMP pages:

The extension ‘amp-auto-ads’ was found on this page, but is unused. Please remove this extension.

AMP auto ads are not implemented/ displayed.

  1. Install and activate AMP for WP plugin
  2. Visit AMP page (typically/amp endpoint)
  3. See the validation error ( in AMP validator or the validation extension)

Screenshots

AMP validation extension error

AMP validator error

Additional Context

  • PHP Version: 7.1.33
  • OS: MacOS
  • Browser: Chrome
  • Plugin Version: 1.1.3
  • Device: MacBook Air

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • In AMP reader mode, the amp-auto-ads component should only be loaded if an amp-auto-ads tag generated by Site Kit is actually present in the post content.

QA Instructions

  1. Activate the AMP plugin and set it to Reader mode.
  2. With AdSense module set up and it set up to render the tag, ensure the amp-auto-ads tag is still displayed in AMP reader mode content, and that there are no AMP validation errors (i.e. the amp-auto-ads component is also loaded).
  3. Add a mini plugin with the following: add_filter( 'the_content', '__return_empty_string', 999 );, which should make every post render with empty content in the frontend. Go back to AMP reader mode content (where the post content should now as well be empty), and ensure that there are no AMP validation errors (i.e. the amp-auto-ads component is not loaded).

Implementation Brief

Changelog entry

@ernee ernee added the Type: Bug Something isn't working label Dec 11, 2019
@felixarntz
Copy link
Member

@westonruter While this is not related to our AMP plugin, would there be any problem if we simply removed adding amp-auto-ads to amp_component_scripts? Our AMP plugin does that automatically if an amp-auto-ads tag exists in the page right?

@westonruter
Copy link
Collaborator

@felixarntz In Standard/Transitional modes, yes. In Reader mode this is only done for components in the post content. However, this is changing in v1.5, in which the entire Reader mode templates are sent through the post-processor (ampproject/amp-wp#2202). So as of v1.5, filtering amp_component_scripts will be truly a thing of the past in the AMP plugin.

@felixarntz felixarntz added the P2 Low priority label Jan 16, 2020
@felixarntz felixarntz self-assigned this Jan 16, 2020
@TB7722
Copy link

TB7722 commented Jan 21, 2020

@felixarntz What is the targeted release date for this. Thanks

@felixarntz
Copy link
Member

@TB7722 We don't have a clear timeline yet, but this might make it into the next release scheduled for in 3 weeks from now.

@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Jan 22, 2020
@aaemnnosttv
Copy link
Collaborator

@felixarntz as long as the implementation is the same across all AMP modes, ✅ IB

@felixarntz felixarntz removed their assignment Jan 22, 2020
@demiromer
Copy link

Hi is there any workaround about this?
Google stop indexing amp pages due to validation error

@jamesozzie
Copy link
Collaborator

@ernee While I can't reproduce this validation error I can see the fallback hook is used in the AMP for WP templates to place the Site Kit Auto ads for AMP code. Maybe we can take a look at this later if you're free.

The image below shows the code placements on AMP for WP (via Site Kit).
image

@demiromer Can you share your site URL and I can see if I notice anything in relation to those errors?

@demiromer
Copy link

Its been over a month @jamesozzie and I have already replaced the plugin

@ernee
Copy link
Author

ernee commented Mar 12, 2020

@jamesozzie as discussed, I no longer get the error as well. I suspect the AMP for WP plugin has added compatibility in version 1.0.7 per their changelog.

@tofumatt tofumatt added the Good First Issue Good first issue for new engineers label Mar 25, 2020
@eclarke1 eclarke1 added this to the Sprint 20 milestone Mar 26, 2020
@ryanwelcher
Copy link
Contributor

I am unable to reproduce this either.

@tofumatt
Copy link
Collaborator

tofumatt commented Apr 1, 2020

If QA can't reproduce this either I think we can safely close it as fixed upstream 👍

@cole10up
Copy link

cole10up commented Apr 2, 2020

I'm unable to reproduce this one on my end. Looks to be fixed.

Passed QA ✅

@cole10up cole10up removed their assignment Apr 2, 2020
@ernee
Copy link
Author

ernee commented Apr 8, 2020

Closing as it was not reproducible any longer.

@ernee ernee closed this as completed Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P2 Low priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests