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

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
Copy link
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.

Copy link
Member

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 westonruter force-pushed the add/842-plugin-validation branch 12 times, most recently from 439a233 to d968626 Feb 21, 2018

@westonruter westonruter force-pushed the add/842-plugin-validation branch from d968626 to 0484bd9 Feb 21, 2018

kienstra 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.

kienstra and others added some commits Mar 5, 2018

@westonruter

This comment has been minimized.

Copy link
Member

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.

Copy link
Member

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 [WIP] Issue #842: Plugin AMP validation Add validation error reporting for plugins and themes Mar 8, 2018

@westonruter
Copy link
Member

westonruter left a comment

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 ampproject#971.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.