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 validation error reporting for plugins and themes #971

Merged
merged 94 commits into from Mar 8, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Collaborator

kienstra commented Feb 20, 2018

This pull request for #842 shows which plugin echoed invalid markup.

validate-query-var

If the user has the 'edit_post' capability, and the query var of amp_validate is present, it adds a response header.

This has all of the data the the response on post.php has, in addition to a 'plugins_with_invalid_output' value. This array has all of the plugins that output invalid markup.

But maybe this isn't the best way to display an error.

The main item remaining for me is the failed Travis build. The tests seem to stop, without error, on PHP < 5.5. This might be related to the closure.

Fixes #842.
Fixes #843.

kienstra added some commits Feb 17, 2018

Issue #842: Begin to validate plugins.
If a <script> or stylesheet gets removed,
store it in $removed_assets.
There isn't yet a way to report inline scripts.
Like <script type="text/javascript">myFunc();</script>.
This uses the 'src' attribute to find the plugin or theme.
Issue #842: Begin plugin callback validation.
Iterate through all of the registered callbacks.
If a callback is from a plugin and outputs markup,
wrap the markup in comments.
Later, the sanitizer can identify which plugin
any illegal markup is from.
Thanks to @westonruter for this strategy.
Issue #842: Handle 'Closure' callbacks in get_plugin().
Also, address a Travis error.
Instead of testing for the presence of 'amp-wp,'
simply make it 'amp.'
This plugin seems to be in the directory 'amp-wp'
in the Travis tests.
Issue #842: Correct an issue in failed Travis build.
The plugin is in 'amp-wp' in some Travis tests.
So instead of expecting the plugin to be 'amp,'
Simply expect it to contain 'amp.'
Issue #842: Process comments for invalid plugin output.
Add an extra parameter to track_removed().
If $source is passed, store that.
Then, if a node is removed,
look for the presence of $source.
Issue #842: Output validation errors in header.
If the query var of 'validate' is present,
Output the previous validation response,
like on the post.php page.
But also add a 'plugins_with_invalid_output' value.
This includes all of the plugins that output markup
which had removed elements or attributes.
Issue #842: Remove list() in favor of array access.
This might have caused issues,
as the order of assignment is different in PHP 7.0.
So insted, access the array values.
Issue #842: Use the constant CALLBACK_KEY.
Substitute all uses of 'remove_invalid_callback'
With AMP_Validation_Utils::CALLBACK_KEY.
Issue #842: Modify how callback function is called.
Mainly use the logic in WP_Hook::apply_filters().
This accounts for the number of accepted args.
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Feb 21, 2018

Collaborator

Figuring out where to display validation errors is tough. Normally this is something that we'd want to show in the admin bar, but this isn't available to us… yet. I think it should be a goal for us to get the admin bar to be fully supported, and then we can show the errors in it. But otherwise, what we may want to do is introduce a new admin screen for “AMP status”. This could eventually interface with the Google Search Console, maybe, to get an external view of AMP validity. This can be augmented with an internal view of validity, being populated with the results of what you are building here.

Maybe what we should do is introduce a new amp_validation_error post type to store logs of such errors. Such a post could include the error, the URL where the error occurred, and plugin/theme responsible, and so on. Then upon plugin activation, a request could be made back to the homepage with the query var auth key to request a validation check, and instead of the errors being returned as an HTTP response header they could be added as post type entries. (Warnings could be displayed at this point as well.) This process could then also be done at WP Cron to regularly check the homepage and maybe the most recently-published post.

By having the errors logged then a user could be informed of them asynchronously, and with the original URL being present, the validation errors could be re-checked after the fact. There could be a button like “Re-check”.

Duplicates should be avoided. So I suggest storing the validation error and the responsible plugin/theme in the post content and then taking the data and creating an md5 hash which then can be used as the post_name. If such a post already exists with that post_name, then no new post would be inserted, but rather perhaps a postmeta for that post could be modified to amend the URL for where the validation error is encountered.

Eventually we'll want to include a WP-CLI command to crawl the entire site and find all validation errors with these being all logged.

Collaborator

westonruter commented Feb 21, 2018

Figuring out where to display validation errors is tough. Normally this is something that we'd want to show in the admin bar, but this isn't available to us… yet. I think it should be a goal for us to get the admin bar to be fully supported, and then we can show the errors in it. But otherwise, what we may want to do is introduce a new admin screen for “AMP status”. This could eventually interface with the Google Search Console, maybe, to get an external view of AMP validity. This can be augmented with an internal view of validity, being populated with the results of what you are building here.

Maybe what we should do is introduce a new amp_validation_error post type to store logs of such errors. Such a post could include the error, the URL where the error occurred, and plugin/theme responsible, and so on. Then upon plugin activation, a request could be made back to the homepage with the query var auth key to request a validation check, and instead of the errors being returned as an HTTP response header they could be added as post type entries. (Warnings could be displayed at this point as well.) This process could then also be done at WP Cron to regularly check the homepage and maybe the most recently-published post.

By having the errors logged then a user could be informed of them asynchronously, and with the original URL being present, the validation errors could be re-checked after the fact. There could be a button like “Re-check”.

Duplicates should be avoided. So I suggest storing the validation error and the responsible plugin/theme in the post content and then taking the data and creating an md5 hash which then can be used as the post_name. If such a post already exists with that post_name, then no new post would be inserted, but rather perhaps a postmeta for that post could be modified to amend the URL for where the validation error is encountered.

Eventually we'll want to include a WP-CLI command to crawl the entire site and find all validation errors with these being all logged.

westonruter and others added some commits Feb 21, 2018

Issue #842: Remove 'static' from methods, add assertions.
In the PHPUnit test class,
there's no need to declare the methods statically.
Also, this adds assertions for a plugin function
that doesn't output anything.
Issue #842: Register validation error post type.
Also, remove add_header().
Adding a response header isn't part of the plan.
Issue #842: Store the validation errors in CPT.
After the preprocessors have run,
get the validation response.
If a plugin output invalid markup,
store the response in a custom post type.

@westonruter westonruter added this to the v0.7 milestone Feb 22, 2018

kienstra added some commits Feb 22, 2018

Issue #842: Store the error URL in metadata.
Also, add more conditions for updating the error post.
If there's already an error post,
but there's no error anymore
delete the post.
Issue #842: Rename function to register_post_type().
Previously, this was post_type().
Also, pass false as the value for 'public.'
Issue #842: Rename function to should_validate_front_end().
Previously, this was do_validate_front_end().
As Weston mentioned, this is more clear.
Issue #842: Change how nodes are processed.
Props @westonruter.
Only call set_plugin_output() if it's a commment.
Issue #842: Rename function to capture_current_source().
Props @westonruter.
This also needs to account for themes and mu-plugins.
@kienstra

This comment has been minimized.

Show comment
Hide comment
@kienstra

kienstra Feb 23, 2018

Collaborator

Thank You, Will Apply Other Suggestions Later Tonight

Hi @westonruter,
Thanks for your suggestions here, and sorry for the delay in applying them. I'll apply the rest of them later tonight.

Collaborator

kienstra commented Feb 23, 2018

Thank You, Will Apply Other Suggestions Later Tonight

Hi @westonruter,
Thanks for your suggestions here, and sorry for the delay in applying them. I'll apply the rest of them later tonight.

kienstra added some commits Feb 23, 2018

Issue #841: Rename property $current_source.
Before, this was $current_plugin_output.
This needs to account for themes and plugins.
Issue #842: Change $current_sources to a stack.
On Weston's suggestion,
as this can track nested plugins in themes.
Also, update PHPUnit tests for this.
Issue #842: Move get_plugin() into get_source().
At Weston's suggestion,
as this needs to apply to themes.
Issue #842: Change how the post meta is updated.
As Weston suggested, started with add_post_meta().
If that is false, get the post meta,
And append the new URL to the array.

kienstra and others added some commits Mar 5, 2018

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Mar 6, 2018

Collaborator

With dd9d629 the AMP Validation Errors are shown in the admin menu:

image

With e6d12c8 the AMP Validation Errors are shown in the At a Glance widget:

image

Collaborator

westonruter commented Mar 6, 2018

With dd9d629 the AMP Validation Errors are shown in the admin menu:

image

With e6d12c8 the AMP Validation Errors are shown in the At a Glance widget:

image

westonruter added some commits Mar 6, 2018

Improve validation error reporting for stylesheets
* Generalize remove_invalid_callback to be validation_error_callback; intend to pass validation error array, not just node. Merge track_removed into add_validation_error.
* Check for stylesheet length overage while iterating over stylesheets; report validation error once reached instead of outputting HTML comments.
* Report CSS mutations when removing !important and overflow from stylesheets.
* Report validation errors regarding amp-keyframes.
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Mar 7, 2018

Collaborator

Additional potential todos:

  • Continue to do existing content check on load of post type is not public. Only if post type supports content. But if public then opt for loopback request. Do with drafts too by passing cookies. Merge content check and frontend check? Otherwise do nothing. Beware of post status is not publish.
  • The notice when saving post should indicate which plugins are are at fault.
  • Add a debug link for the admin notice that appears when saving a post.
  • Link to validation status post from admin notice.
  • Re-visit cron integration implementation and REST endpoint.
  • Move additional logic from theme support to validation utils.
  • Account for trashed posts when locating existing post? Restore from trash.

Future ideas:

  • Eliminate cap check in favor of auth key or nonce?
  • What happens if an individual URL had it's issue fixed, but it isn't fixed on the other URLs with the same problem?
  • Export queried object as response header to store in validation status, then add link from queried post to the validation status post?
  • Add ability to ignore certain validation errors? Dismiss a validation status by changing it to something else. Let pending mean it is not dismissed, but rather unprocessed. See #1003.
  • Leverage xml sitemap to crawl site for WP cron and WP-CLI.
Collaborator

westonruter commented Mar 7, 2018

Additional potential todos:

  • Continue to do existing content check on load of post type is not public. Only if post type supports content. But if public then opt for loopback request. Do with drafts too by passing cookies. Merge content check and frontend check? Otherwise do nothing. Beware of post status is not publish.
  • The notice when saving post should indicate which plugins are are at fault.
  • Add a debug link for the admin notice that appears when saving a post.
  • Link to validation status post from admin notice.
  • Re-visit cron integration implementation and REST endpoint.
  • Move additional logic from theme support to validation utils.
  • Account for trashed posts when locating existing post? Restore from trash.

Future ideas:

  • Eliminate cap check in favor of auth key or nonce?
  • What happens if an individual URL had it's issue fixed, but it isn't fixed on the other URLs with the same problem?
  • Export queried object as response header to store in validation status, then add link from queried post to the validation status post?
  • Add ability to ignore certain validation errors? Dismiss a validation status by changing it to something else. Let pending mean it is not dismissed, but rather unprocessed. See #1003.
  • Leverage xml sitemap to crawl site for WP cron and WP-CLI.

@westonruter westonruter self-assigned this Mar 7, 2018

westonruter added some commits Mar 8, 2018

Recheck the validation status of a post on frontend after saving when…
… possible

* Move validation logic from theme support to validation utils
* Eliminate saving of validation status post when doing GET request; only store validation errors by invoker.

@westonruter westonruter changed the title from [WIP] Issue #842: Plugin AMP validation to Add validation error reporting for plugins and themes Mar 8, 2018

@westonruter

What a massive effort. And what a great result! This sets us up in a big way to do way more with validation in upcoming releases.

@westonruter westonruter merged commit df7db64 into develop Mar 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/842-plugin-validation branch Mar 8, 2018

kienstra added a commit that referenced this pull request Mar 18, 2018

Revert commit that removed REST API logic.
Commit ba9365 removed this,
as it wasn't needed for 0.7.
This restores the logic for the REST API,
but not the wp-cron logic.

See PR #971.

juanchaur1 added a commit to juanchaur1/amp-wp that referenced this pull request May 9, 2018

Revert commit that removed REST API logic.
Commit ba9365 removed this,
as it wasn't needed for 0.7.
This restores the logic for the REST API,
but not the wp-cron logic.

See PR Automattic#971.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment