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

Limit validation to AMP theme support #1132

Merged
merged 1 commit into from May 8, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

commented May 7, 2018

  • Fixes problems related to the_content filters being applied in the admin. See #1130.
  • Eliminates validation errors from being displayed when the_content filters apply when creating new posts.
  • Reduces influx of support topics related to warning messages; the debug link is not helpful unless theme support is present.

To test:

git clone --recursive https://github.com/Automattic/amp-wp.git amp
cd amp
git checkout update/warning-notice
npm install
composer install
npm run build

This will create an amp.zip file that you can install on your site. Note you'll have to first deactivate and uninstall the v0.7.0 AMP plugin before you can upload and activate this ZIP on the plugins admin screen. Ideally you could test this on a staging site.

Support tickets:

@westonruter westonruter added this to the v0.7.1 milestone May 7, 2018

@westonruter westonruter requested a review from kienstra May 7, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2018

Thanks, Will Review Soon

Hi @westonruter,
Good idea here. If it's alright, I'll start reviewing in about an hour.

@kienstra
Copy link
Collaborator

left a comment

Approved

Hi @westonruter,
Thanks, this pull request looks good, and works as expected. There are no validation errors when using Legacy Templating. Only in Native AMP or Paired Mode.

It's nice that this also simplifies the repo, using less if blocks.

There's a question here, but it's not a blocker.

$validation_errors = array();
// Skip if the post type is not viewable on the frontend, since we need a permalink to validate.
if ( ! is_post_type_viewable( $post->post_type ) ) {
return;

This comment has been minimized.

Copy link
@kienstra

kienstra May 8, 2018

Collaborator

Good idea to exit if the post type isn't viewable.

// Make sure original post is restored after applying shortcodes which could change it.
$GLOBALS['post'] = $post; // WPCS: override ok.
setup_postdata( $post );
if ( $validation_status_post ) {

This comment has been minimized.

Copy link
@kienstra

kienstra May 8, 2018

Collaborator

Could this be an else block?

if ( ! $validation_status_post ) {
	return;
} else { 
	$validation_errors = json_decode( $validation_status_post->post_content, true );
}

This comment has been minimized.

Copy link
@westonruter

westonruter May 8, 2018

Author Member

Actually, there's no need for the if at all here. Good point.

Limit validation to AMP theme support
* Fixes problems related to the_content filters being applied in the admin.
* Eliminates validation errors from being displayed when the_content filters apply when creating new posts.
* Reduces influx of support topics related to warning messages; the debug link is not helpful unless theme support is present.

@westonruter westonruter force-pushed the update/warning-notice branch from 871bf4f to 801b9cf May 8, 2018

@westonruter westonruter merged commit b5c53be into 0.7 May 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 update/warning-notice branch May 8, 2018

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.