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

Introduce and expose query arguments for debugging AMP content generation #1294

Closed
westonruter opened this issue Jul 31, 2018 · 13 comments
Closed
Assignees
Labels
Documentation Documentation related tasks Groomed Needs More Info Follow-up required in order to be actionable. P2 Low priority WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

westonruter commented Jul 31, 2018

There is currently a query that can be added to an AMP URL to facilitate debugging:

  • amp_preserve_source_comments: This prevents source comments from being removed when performing an amp_validate request.

This isn't exposed anywhere in the UI to discover, and the developer just has to know it exists. Maybe this is fine, but there should be a Wiki page with debugging information to inform of its use. Alternatively, when WP_DEBUG is enabled there could be a link added to the AMP admin bar menu to link to it. (Update: UI to be addressed in a subsequent issue.)

But beyond amp_preserve_source_comments there is probably other flags that would be useful for debugging:

  • Disable output buffering and short-circuit the sanitizers. This would facilitate inspection of the response from WordPress prior to the post-processors kicking in. This could be particularly helpful debugging embed handlers as well as figuring out where a fatal error may be happening.
  • Disable response cache. This would be useful to help with performance debugging since there is a response cache to prevent post-processors from re-processing the same HTML repeatedly.
  • Exclude sanitizers from being run. This could help with identifying which sanitizer is causing problems.
  • Prevent redirection from AMP URL to non-AMP URL when there are unaccepted validation errors. The amp_validate param does this already, but it also has the behavior of adding validation error source comments, so having another param may be helpful.
  • Automatically reject all validation errors on a given page.
  • Others?

These debug parameters should only be made available when an appropriately-authorized user is authenticated.

@westonruter
Copy link
Member Author

The amp_validate query param can also be used to preview what a site in classic mode will look like when theme support is enabled. The user must be an admin for this to be honored.

@westonruter westonruter added this to the v1.1 milestone Dec 5, 2018
@westonruter westonruter removed this from the v1.1 milestone Mar 6, 2019
@westonruter westonruter added this to the v2.0 milestone Jul 15, 2019
@amedina amedina changed the title Introduce and expose query arguments for debugging AMP responses Introduce and expose query arguments for debugging AMP content generation Aug 1, 2019
@kienstra
Copy link
Contributor

Question About Working On This

Hi @westonruter,
Hope your week's off to a great start.

It looks like this is assigned to you. Could I work on it? Otherwise, no problem 😄

Thanks, Weston!

@westonruter
Copy link
Member Author

@kienstra Yes, please go for it.

@westonruter westonruter assigned kienstra and unassigned westonruter Sep 23, 2019
@westonruter westonruter modified the milestones: v2.0, v1.3.1 Sep 23, 2019
@kienstra
Copy link
Contributor

Thanks, Weston!

@kienstra
Copy link
Contributor

kienstra commented Oct 4, 2019

Question about a query arg

Hi @westonruter,
Happy friday!

Exclude sanitizers from being run. This could help with identifying which sanitizer is causing problems.

Should this exclude only a single sanitizer, like ?exclude_sanitizer=nav-menu-dropdown

Or would this somehow be different from the other query arg:

Disable output buffering and short-circuit the sanitizers. This would facilitate inspection of the response from WordPress prior to the post-processors kicking in. This could be particularly helpful debugging embed handlers as well as figuring out where a fatal error may be happening.

Thanks, Weston!

@westonruter
Copy link
Member Author

Excluding all sanitizers would essentially be the same as short-circuiting the entire post-processor. I'm not sure of the value of providing a UI to disable individual sanitizers. I suggest revisiting that later.

@kienstra
Copy link
Contributor

kienstra commented Oct 4, 2019

OK, thanks

@westonruter
Copy link
Member Author

westonruter commented Oct 4, 2019

  • One additional debug flag that would be helpful is to auto-accept all validation errors for excessive_css. Often times a site may look broken but the biggest issue is too much CSS. Being able to quickly just eliminate the 50KB limit can help narrow-down where to focus efforts. This is closely related to Discontinue excluding excessive CSS by default on Standard mode sites #2326. Auto-rejecting all validation errors would be a superset of this, but it goes too far.

@kienstra
Copy link
Contributor

kienstra commented Oct 4, 2019

Yeah, that's a good idea. Excessive CSS errors can be tough on big sites.

@westonruter
Copy link
Member Author

westonruter commented Oct 11, 2019

Something else that just came up which would be helpful for debugging: when adding the Countdown block from the Ultimate Blocks plugin, you will get two validation errors due to scripts being enqueued on the page. If you are in Transitional mode, it is easy to see what the expected behavior of the block is sine you can just look at the non-AMP version. If you are in Standard mode, you don't want to have to switch to Transitional. You also don't want to turn off AMP for the post/page in order to check it. The alternative then is to try marking the two validation errors as Rejected so that they scripts don't get sanitized and then click the Preview button. This allows the scripts to be included on the page, but the countdown timer then has broken styling because the styles were removed given that they weren't in the page at serve time. So to help identify that as being an issue, a debug flag would be helpful.

  • Add debug flag to force AMP to be disabled for the current request. (This is different than the flag to short-circuit the post-processor, since in that case the embed handlers will be outputting AMP components.)
  • Add debug flag to disable tree shaking (and allow all excessive_css, per above).

@kienstra
Copy link
Contributor

@westonruter, sounds good. I'll add those to #3448.

@swissspidy swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019
@kienstra
Copy link
Contributor

kienstra commented Oct 30, 2019

As this issue's PR is awaiting acceptance criteria, I'm moving this back to 'To Do.'

@westonruter westonruter modified the milestones: v.1.4.1, v1.5 Nov 7, 2019
@kmyram kmyram removed the Sprint: 22 label Jan 30, 2020
@westonruter westonruter removed this from the v1.5 milestone Mar 5, 2020
@westonruter
Copy link
Member Author

One-off build to help with debugging support topic: amp.zip (v1.5.0-RC1-20200326T192429Z-d69f536dd)

Includes patch:

--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -1889,6 +1889,9 @@ class AMP_Theme_Support {
 	 * @return string Processed Response.
 	 */
 	public static function finish_output_buffering( $response ) {
+		if ( isset( $_GET['amp_bypass_post_processing'] ) ) {
+			return '<!-- amp:output_buffered -->' . $response . '<!-- /amp:output_buffered -->';
+		}
 		self::$is_output_buffering = false;
 		return self::prepare_response( $response );
 	}

@amedina amedina removed the Size: M label Mar 31, 2020
@westonruter westonruter linked a pull request Apr 16, 2020 that will close this issue
5 tasks
@westonruter westonruter removed a link to a pull request Apr 16, 2020
5 tasks
@westonruter westonruter linked a pull request Apr 16, 2020 that will close this issue
5 tasks
@westonruter westonruter removed a link to a pull request Apr 16, 2020
5 tasks
@amedina amedina added the P2 Low priority label May 14, 2020
westonruter added a commit that referenced this issue Jun 27, 2020
…can validate

Also add a "View with AMP disabled" link to the admin bar for such users to easily disable AMP for a URL when a site is in Standard mode without requiring them to switch the entire site to Transitional mode or to disable AMP for a given URL.

See #1294
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@kmyram kmyram added the Groomed label Nov 24, 2020
@kmyram kmyram added the Documentation Documentation related tasks label Nov 24, 2020
@kmyram kmyram added this to the v2.1 milestone Nov 24, 2020
@kmyram kmyram added the Needs More Info Follow-up required in order to be actionable. label Nov 24, 2020
@westonruter westonruter removed this from the v2.1 milestone Dec 10, 2020
@westonruter westonruter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation related tasks Groomed Needs More Info Follow-up required in order to be actionable. P2 Low priority WS:Core Work stream for Plugin core
Projects
Archived in project
Development

No branches or pull requests

7 participants