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

Invalid markup by AMP plugin with enabled JavaScript tracking #181

Closed
MatzeKitt opened this issue Aug 27, 2020 · 4 comments · Fixed by #182
Closed

Invalid markup by AMP plugin with enabled JavaScript tracking #181

MatzeKitt opened this issue Aug 27, 2020 · 4 comments · Fixed by #182

Comments

@MatzeKitt
Copy link
Member

Every time I save a post I get two validation errors in the official AMP plugin with enabled JavaScript tracking:

Invalid inline script
Fehlercode
DISALLOWED_TAG
Ungültiges Markup

<script … > Element-Attribute type text/javascript id statify-js-js-extra Element-Name script Übergeordnetes Element body Textinhalt 128 Bytes /* */

Invalid script snippet.min.js
Fehlercode
DISALLOWED_TAG
Ungültiges Markup

<script … > Element-Attribute type text/javascript src https://kittmedia.com/wp-content/plugins/statify/js/snippet.min.js id statify-js-js Element-Name script Übergeordnetes Element body

Due to the fact that Statify is recommending using the JavaScript tracking method to track AMP pages as well, I would assume that it will work properly. However, as soon as this invalid markup gets removed (which is required in order to serve AMP pages), Statify isn’t able anymore to track any AMP page visits.

AMP version: 2.0.0 (already occurred in version 1.5.x)
Statify version: 1.8.0
WordPress version: 5.5.0

@Zodiac1978
Copy link
Member

Related #110 and #116

@stklcode
Copy link
Contributor

stklcode commented Aug 27, 2020

That's right, we always include the JS, if JS-Tracking is enabled:

wp_enqueue_script(
'statify-js',
plugins_url( 'js/snippet.min.js', STATIFY_FILE ),
array(),
STATIFY_VERSION,
true
);
// Add endpoint to script.
wp_localize_script(
'statify-js',
'statify_ajax',
array(
'url' => admin_url( 'admin-ajax.php' ),
'nonce' => wp_create_nonce( 'statify_track' ),
)
);

Statify isn’t able anymore to track any AMP page visits.

Are you sure it isn't?

We do have the amp_post_template_analytics() method to register a separate analysis hook:

public static function amp_post_template_analytics( $analytics ) {

The script tag is removed from AMP markup, but that should be OK here. Otherwise we would track twice.
So if visits are not tracked, it's not because the script is removed, but likely for a different reason.

Nevertheless we should not include it at all, if we are processing an AMP request to remove the unnecessary warning.

@MatzeKitt
Copy link
Member Author

Are you sure it isn’t?

Yes, I am.

Visiting https://kittmedia.com as guest adds an entry in the database table of Statify, visiting https://kittmedia.com/?amp does not.

@stklcode
Copy link
Contributor

OK, I got the point. In fact we got 2 problems here:

  1. the invalid markup due to the unnecessary <script> tags
    => Simple solution, don't add the snippet on AMP requests
  2. We use the amp_post_template_analytics hook, which is only available in Reader Mode
    => Additionally hook into amp_analytics_entries for Standard/transitional Mode (cave: different data model)

@stklcode stklcode added the bug label Aug 28, 2020
@stklcode stklcode added this to the 1.8.1 milestone Aug 29, 2020
Zodiac1978 added a commit that referenced this issue Oct 17, 2020
fix AMP compatibility for Standard and Transitional mode (#181)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants