-
Notifications
You must be signed in to change notification settings - Fork 383
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
Add debugging query arguments #3448
Conversation
- disable_post_processing - disable_response_cache - prevent_redirect - reject_all_errors @todo: add a query arg to exclude sanitizers.
I think these would work well as toggles in an submenu item of the AMP admin bar menu item, if the user has the capability to use them. Clicking on a nav menu item can toggle the option on/off as it reloads the page. |
includes/class-amp-theme-support.php
Outdated
@@ -1872,6 +1893,10 @@ public static function start_output_buffering() { | |||
newrelic_disable_autorum(); | |||
} | |||
|
|||
if ( isset( $_GET[ self::DISABLE_POST_PROCESSING_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of introducing these as top-level query parameters, how about putting them all under some amp_flags
query var? This would make it easier to scrub them from the URL and it would reduce opportunities for collision. So So here it could be checking isset( $_GET['amp_flags'][ self::DISABLE_POST_PROCESSING_QUERY_VAR ] )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19363fc adds an amp_flags query var, to access these query vars like you mentioned.
For example:
https://loc.test/your-post/?amp&_flags[reject_all_errors]
includes/class-amp-theme-support.php
Outdated
[ | ||
self::DISABLE_RESPONSE_CACHE_QUERY_VAR, | ||
AMP_Validation_Error_Taxonomy::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, this could just remove amp_flags
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll apply this also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, it's applied here.
@@ -465,6 +472,11 @@ public static function get_validation_error_sanitization( $error ) { | |||
$forced = 'with_preview'; | |||
} | |||
|
|||
if ( isset( $_GET[ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] ) && AMP_Validation_Manager::has_cap() ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As opposed to adding a new condition here, I suggest using the amp_validation_error_sanitized
filter below, since these debug scenarios are outside the normal band of validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, how does this look?
Hi @westonruter,
Ah, interesting. Something like this? |
Yes, something like that. I realize that some of the toggles are mutually-exclusive, so that complicates it a bit. |
…nditional As Weston mentioned, this can simply use the filter that exists,
Ah, the commit above needs reworking. |
…evel var The filter in accept_validation_errors() might not actually run, if get_acceptable_errors() doesn't return anything.
Should the query vars be prefixed to not cause conflicts with other plugins? |
Hi @swissspidy,
Good question. 19363fc adds an amp_flags query var, to access these query vars like:
...which is accessed like: $_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] ) |
This adds a callback for 'amp_validation_error_sanitized', and also checks for the presence of the 'reject_all_errors' query var in that callback. The query var is accessed like: ?amp_flags[accept_excessive_css]
This prevents raising an 'excessive_css' error, and simply prints all of the styling in the <style amp-custom>
This is mainly intended for Standard mode, as it would be strange to have an ?amp query var, then one after it to disable AMP. Still, there's an issue where on redirecting in Transitional mode, the query var is still present in the URL, like: https://example.com/new-post-2/?amp_flags%5Bdisable_amp%5D
… arg It's possible, though probably not common, that a request would have ?amp and the query arg to disable AMP. When in Transitional mode, this will cause a redirect to the non-AMP endpoint. This ensures that the amp_flags query var is stripped from the redirect URL.
This uses React on the front-end, as it will use toggles, and may need to de-select some toggles when certain toggles are selected. It might be a little too much to enqueue React (wp-element) just for this, so I'll have to revisit whether it's necessary.
Remove a test that looks to have caused and undefined index error.
It looks like another function was running on the same filter, and had an error. So remove_remove_all_filters() for that filter.
It looks like these didn't fix the failing Travis builds. So instead, register the 'wp-element' script.
There is still much remaining, including adding the toggles and styling.
Both in the name of the function and the query var. This is to stay consistent with other query vars that have 'disable' in them, like 'disable_amp'
Use the <ToggleControl> from Gutenberg, though this still needs a lot of styling.
There's a 'View AMP version' link, so it probably makes sense to have these query vars available.
@kienstra I think the submenu could be made a bit simpler by removing the checkboxes. In other words, each of the nav menu items would be a link which would be preceded by some on/off indicator. Clicking the link would cause the page to reload with the query var update(s), and opening the submenu would then show the toggle indicator had been flipped. |
To test the behavior with: &_flags[disable_post_processing]=random-string
@@ -101,6 +101,7 @@ class AMP_Autoloader { | |||
'AMP_Validation_Callback_Wrapper' => 'includes/validation/class-amp-validation-callback-wrapper', | |||
'AMP_Validated_URL_Post_Type' => 'includes/validation/class-amp-validated-url-post-type', | |||
'AMP_Validation_Error_Taxonomy' => 'includes/validation/class-amp-validation-error-taxonomy', | |||
'AMP_Debug' => 'includes/validation/class-amp-debug', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's debatable whether class-amp-debug.php
should be in the validation/
directory. Most of the debugging flags have to do with validation, but some don't, like disable_amp
.
For example, &_flags[disable_post_processing]=5555
This should help to see how to use this.
What about the flag without value?
|
Ah, good question. That'll be |
Request For Another Round Of Review Hi @westonruter and @schlessera, When you have a chance, could you please do another round of review? The table of flags above is still up-to-date. And here are the values that the query var can have. |
@kienstra Would it make sense to change this so that no flag value means |
Sure, that's the behavior for the |
Now, something like: &_flags[disable_amp] will be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent PR so far @kienstra. The only concern I have right now is that with the use of checkboxes it gives the impression that multiple flags can be assigned at once, which is not so. I would suggest using something similar to the radio input.
Quick mockup of what I'm thinking of:
*/ | ||
public static function get_debugging_option_title( $title_text, $query_var ) { | ||
$checkbox = AMP_Debug::has_flag( $query_var ) ? '☑' : '☐'; | ||
return sprintf( '<span class="amp-debug-option" style="font-family:Roboto,Oxygen-Sans,sans-serif;font-size:17px">%s</span> %s', $checkbox, esc_html( $title_text ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this can't be a checkbox input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pierlon, That's an interesting suggestion. It's possible, though not likely, that 2 debug flags would be present at once. Still, one would have to manually add those query vars, it's not possible to get 2 debugging flags by simply clicking in the admin bar. For example: |
…-args-debugging * 'develop' of github.com:ampproject/amp-wp: (163 commits) Remove entries array test after completed. Update dependency core-js to v3.3.4 (#3624) Quick UX fixes (#3611) Fix most pressing RTL issues (#3558) Add test case for broken parent relationship Adapt broken test Throw a _doing_it_wrong() when an expected parent is not found Change template hierarchy query arguments to public Reuse data-set-focus to reliably match twentytwenty keyboard focus management Add tabindex attribute to modals Guess the role of a modal based on its classes Disable Code Editor (#3608) Update dependency postcss to v7.0.20 (#3613) Update dependency autoprefixer to v9.7.0 (#3614) Update dependency browserslist to v4.7.2 (#3612) Add rel=preconnect link for AMP CDN Fix block nav padding and margin (#3610) Adjust Gutenberg / WordPress requirement (#3609) Don't allow multiple CTA / attachment block to be pasted. (#3601) Fix page crashing when duplicating immovable blocks (#3593) ...
Ah, this actually is the intention here, to allow multiple debug flags to be checked at once. A tricky bit, however, is that various debug flags are mutually-exclusively (#3448 (comment)), so there would need to be some logic that removes/disables some of the options based on the state of the others. |
@kienstra Let's put this on pause until we can flesh out more of the acceptance criteria and determine how the different options relate to each other, and now those options apply in Standard vs Transitional modes. |
Sounds good, thanks |
|
Summary
Adds query arguments for debugging under the
amp_flags
query arg. For example:https://example.com/baz-post/?amp&_flags[disable_post_processing]=1
disable_post_processing
disable_response_cache
prevent_redirect
reject_all_errors
accept_excessive_css
excessive_css
errors. Tree shaking still takes place, but no styling is removed because it's over the limit.disable_amp
disable_tree_shaking
@todo:
disable_post_processing
)Addresses issue #1294
Checklist