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

Theme/plugin responsible for prematurely calling is_amp_endpoint() should be identified #4602

Closed
westonruter opened this issue Apr 17, 2020 · 1 comment · Fixed by #5289
Closed
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P1 Medium priority WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Apr 17, 2020

Feature description

A very common issue for themes and plugins integrating with the AMP plugin is calling is_amp_endpoint() before the wp action, for example:

Often users think that the AMP plugin is at fault because they only see the issue being emitted from the AMP plugin. In reality the issue is with another theme/plugin, but the other theme/plugin is not being identified. In #4574 (comment) I proposed:

One more thing that comes to mind to improve this is to look at the call stack to find out which theme/plugin is actually responsible for calling is_amp_endpoint() prematurely.

This would be really helpful for users trying to find out who to contact about the problem. We can make use of the same logic used to identify a theme/plugin for the validation error source stacks here. At the moment, this logic is contained in \AMP_Validation_Manager::get_source( $callback ). If we can grab all the functions from the callstack via debug_backtrace() and then obtain the function from each and obtain the source, we can then list the theme/plugin in the message that is emitted by _doing_it_wrong.

Note that this should probably only be done when WP_DEBUG is enabled.

If the admin bar is being displayed (and WP_DEBUG is not enabled) perhaps the issue should be called out as a warning under the AMP admin menu item. And if there, should the issue also be called out on the Validated URL screen? This gets into some new territory for the Validation screens, namely in the beginning they were limited to showing AMP validation errors. However, then they added Stylesheet information. I've also proposed that fatal errors occurring during validation should be exposed (#4580). What about incorrect usage issues or other problems with AMP content? The AMP validator itself has two classes of issues: errors and warnings. Invalid JSON is actually a warning and not an error (see #4430). Should those there be a section on the validated URL screen for warnings, including things like invalid JSON, calling is_amp_endpoint incorrectly, and fatal errors?


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

Acceptance criteria

  • When a theme or plugin calls is_amp_endpoint() (and friends) too early, any notice that is displayed should identify that theme or plugin as the culprit.

Implementation brief

  • Use debug_backgrace() to obtain the first call stack entry outside the AMP plugin to identify who called the function.

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Enhancement New feature or improvement of an existing one P0 High priority labels Apr 17, 2020
@westonruter westonruter changed the title Theme/plugin responsible for calling is_amp_endpoint() too early should be identified Theme/plugin responsible for prematurely calling is_amp_endpoint() should be identified Apr 17, 2020
@kmyram kmyram added this to the v1.6 milestone May 27, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 15, 2020
@westonruter westonruter added P1 Medium priority and removed P0 High priority labels Jun 15, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@westonruter westonruter added this to the v2.0.1 milestone Aug 27, 2020
@westonruter westonruter modified the milestones: v2.0.1, v2.0.2 Sep 3, 2020
@westonruter westonruter self-assigned this Sep 17, 2020
@westonruter
Copy link
Member Author

QA Passed. ✅

Must-Use Plugin

<?php
/**
 * Plugin Name: Must use calling is_amp_endpoint() early
 */
add_action( 'init', function () {
	is_amp_endpoint();
} );

image

Plugin

<?php
/**
 * Plugin Name: Call is_amp_endpoint() early
 */
add_action( 'init', function () {
	is_amp_endpoint();
} );

image

Theme

Adding this code to Twenty Twenty:

add_action( 'init', function () {
	is_amp_endpoint();
} );

image

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P1 Medium priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants