Expose reason for why a validation error is raised

[westonruter]

When there is a validation error causing an element such as the following to be removed:

<link rel="manifest" href="http://example.com/wp-json/app/v1/web-manifest">

There is no corresponding reason why it has been removed. The developer currently has to figure it out on their own. This us unfortunate because the plugin has the reason as part of the validation logic. So in the above example, the validation error is raised due to an illegal protocol being used. The validation error would be detected here:

https://github.com/Automattic/amp-wp/blob/b3e1b4d07dda70c5070d30f7426f849eb3a39823/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php#L983-L985

And this is due to the spec mandating the https protocol for manifest links:

https://github.com/Automattic/amp-wp/blob/b3e1b4d07dda70c5070d30f7426f849eb3a39823/includes/sanitizers/class-amp-allowed-tags-generated.php#L7111-L7119

We need to surface this reason when a validation error happens.

We should be reusing the same error codes (and messages) that the validator itself uses.

Compare/contrast with what the plugin is using today:

amp-wp/includes/validation/class-amp-validation-error-taxonomy.php

Lines 2872 to 2949 in 980375c

	/**
	* Get Error Title from Code
	*
	* @param array $validation_error Validation error.
	* @return string Title with some formatting markup.
	*/
	public static function get_error_title_from_code( $validation_error ) {
	switch ( $validation_error['code'] ) {
	case self::INVALID_ELEMENT_CODE:
	if ( self::is_validation_error_for_js_script_element( $validation_error ) ) {
	$title = esc_html__( 'Invalid script', 'amp' );
	if ( isset( $validation_error['node_attributes']['src'] ) ) {
	$title .= sprintf(
	': <code>%s</code>',
	esc_html( basename( wp_parse_url( $validation_error['node_attributes']['src'], PHP_URL_PATH ) ) )
	);
	}
	} else {
	$title = esc_html__( 'Invalid element', 'amp' );
	$title .= sprintf( ': <code>&lt;%s&gt;</code>', esc_html( $validation_error['node_name'] ) );
	}
	return $title;
	case self::INVALID_ATTRIBUTE_CODE:
	return sprintf(
	'%s: <code>%s</code>',
	esc_html__( 'Invalid attribute', 'amp' ),
	esc_html( $validation_error['node_name'] )
	);
	case 'invalid_processing_instruction':
	return sprintf(
	'%s: <code>&lt;%s%s&hellip;%s&gt;</code>',
	esc_html__( 'Invalid processing instruction', 'amp' ),
	'?',
	esc_html( $validation_error['node_name'] ),
	'?'
	);
	case 'file_path_not_allowed':
	return esc_html__( 'Stylesheet file path not allowed', 'amp' );
	case 'excessive_css':
	return esc_html__( 'Excessive CSS', 'amp' );
	case 'illegal_css_at_rule':
	return sprintf(
	'%s: <code>@%s</code>',
	esc_html__( 'Illegal CSS at-rule', 'amp' ),
	esc_html( $validation_error['at_rule'] )
	);
	case 'disallowed_file_extension':
	return esc_html__( 'Disallowed CSS file extension', 'amp' );
	case 'duplicate_element':
	return sprintf(
	'%s: <code>&lt;%s&gt;</code>',
	esc_html__( 'Duplicate element', 'amp' ),
	esc_html( $validation_error['node_name'] )
	);
	case 'unrecognized_css':
	return esc_html__( 'Unrecognized CSS', 'amp' );
	case 'css_parse_error':
	return esc_html__( 'CSS parse error', 'amp' );
	case 'stylesheet_file_missing':
	return esc_html__( 'Missing stylesheet file', 'amp' );
	case 'illegal_css_property':
	$title = esc_html__( 'Illegal CSS property', 'amp' );
	if ( isset( $validation_error['property_name'] ) ) {
	$title .= sprintf( ': <code>%s</code>', esc_html( $validation_error['property_name'] ) );
	}
	return $title;
	case 'illegal_css_important':
	$title = esc_html__( 'Illegal CSS !important property', 'amp' );
	if ( isset( $validation_error['property_name'] ) ) {
	$title .= sprintf( ': <code>%s</code>', esc_html( $validation_error['property_name'] ) );
	}
	return $title;
	default:
	/* translators: %s error code */
	return sprintf( __( 'Unknown error (%s)', 'amp' ), $validation_error['code'] );
	}
	}

Validation Error Codes

Status	Code
?	ATTR_DISALLOWED_BY_IMPLIED_LAYOUT
?	ATTR_DISALLOWED_BY_SPECIFIED_LAYOUT
?	ATTR_MISSING_REQUIRED_EXTENSION
?	ATTR_REQUIRED_BUT_MISSING
?	ATTR_VALUE_REQUIRED_BY_LAYOUT
?	BASE_TAG_MUST_PRECEED_ALL_URLS
?	CDATA_VIOLATES_BLACKLIST
?	CHILD_TAG_DOES_NOT_SATISFY_REFERENCE_POINT
?	CHILD_TAG_DOES_NOT_SATISFY_REFERENCE_POINT_SINGULAR
?	CSS_EXCESSIVELY_NESTED
?	CSS_SYNTAX_BAD_URL
?	CSS_SYNTAX_DISALLOWED_DOMAIN
?	CSS_SYNTAX_DISALLOWED_KEYFRAME_INSIDE_KEYFRAME
?	CSS_SYNTAX_DISALLOWED_MEDIA_FEATURE
?	CSS_SYNTAX_DISALLOWED_MEDIA_TYPE
?	CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE
?	CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE_WITH_HINT
?	CSS_SYNTAX_DISALLOWED_QUALIFIED_RULE_MUST_BE_INSIDE_KEYFRAME
?	CSS_SYNTAX_DISALLOWED_RELATIVE_URL
?	CSS_SYNTAX_EOF_IN_PRELUDE_OF_QUALIFIED_RULE
?	CSS_SYNTAX_ERROR_IN_PSEUDO_SELECTOR
?	CSS_SYNTAX_INCOMPLETE_DECLARATION
?	CSS_SYNTAX_INVALID_ATTR_SELECTOR
?	CSS_SYNTAX_INVALID_AT_RULE
?	CSS_SYNTAX_INVALID_DECLARATION
?	CSS_SYNTAX_INVALID_PROPERTY
?	CSS_SYNTAX_INVALID_PROPERTY_NOLIST
?	CSS_SYNTAX_INVALID_URL
?	CSS_SYNTAX_INVALID_URL_PROTOCOL
?	CSS_SYNTAX_MALFORMED_MEDIA_QUERY
?	CSS_SYNTAX_MISSING_SELECTOR
?	CSS_SYNTAX_MISSING_URL
?	CSS_SYNTAX_NOT_A_SELECTOR_START
?	CSS_SYNTAX_PROPERTY_DISALLOWED_TOGETHER_WITH
?	CSS_SYNTAX_PROPERTY_DISALLOWED_WITHIN_AT_RULE
?	CSS_SYNTAX_PROPERTY_REQUIRES_QUALIFICATION
?	CSS_SYNTAX_QUALIFIED_RULE_HAS_NO_DECLARATIONS
?	CSS_SYNTAX_STRAY_TRAILING_BACKSLASH
?	CSS_SYNTAX_UNPARSED_INPUT_REMAINS_IN_SELECTOR
?	CSS_SYNTAX_UNTERMINATED_COMMENT
?	CSS_SYNTAX_UNTERMINATED_STRING
?	DEPRECATED_ATTR
?	DEPRECATED_TAG
?	DEV_MODE_ONLY
?	DISALLOWED_ATTR
?	DISALLOWED_CHILD_TAG_NAME
?	DISALLOWED_DOMAIN
?	DISALLOWED_FIRST_CHILD_TAG_NAME
?	DISALLOWED_MANUFACTURED_BODY
?	DISALLOWED_PROPERTY_IN_ATTR_VALUE
?	DISALLOWED_RELATIVE_URL
?	DISALLOWED_SCRIPT_TAG
?	DISALLOWED_STYLE_ATTR
?	DISALLOWED_TAG
?	DISALLOWED_TAG_ANCESTOR
?	DOCUMENT_SIZE_LIMIT_EXCEEDED
?	DOCUMENT_TOO_COMPLEX
?	DUPLICATE_ATTRIBUTE
?	DUPLICATE_DIMENSION
?	DUPLICATE_REFERENCE_POINT
?	DUPLICATE_UNIQUE_TAG
?	DUPLICATE_UNIQUE_TAG_WARNING
?	EXTENSION_UNUSED
?	GENERAL_DISALLOWED_TAG
?	IMPLIED_LAYOUT_INVALID
?	INCONSISTENT_UNITS_FOR_WIDTH_AND_HEIGHT
?	INCORRECT_MIN_NUM_CHILD_TAGS
?	INCORRECT_NUM_CHILD_TAGS
?	INLINE_STYLE_TOO_LONG
?	INVALID_ATTR_VALUE
?	INVALID_JSON_CDATA
?	INVALID_PROPERTY_VALUE_IN_ATTR_VALUE
?	INVALID_URL
?	INVALID_URL_PROTOCOL
?	INVALID_UTF8
?	MANDATORY_ANYOF_ATTR_MISSING
?	MANDATORY_ATTR_MISSING
?	MANDATORY_CDATA_MISSING_OR_INCORRECT
?	MANDATORY_LAST_CHILD_TAG
?	MANDATORY_ONEOF_ATTR_MISSING
?	MANDATORY_PROPERTY_MISSING_FROM_ATTR_VALUE
?	MANDATORY_REFERENCE_POINT_MISSING
?	MANDATORY_TAG_ANCESTOR
?	MANDATORY_TAG_ANCESTOR_WITH_HINT
?	MANDATORY_TAG_MISSING
?	MISSING_LAYOUT_ATTRIBUTES
?	MISSING_REQUIRED_EXTENSION
?	MISSING_URL
?	MUTUALLY_EXCLUSIVE_ATTRS
?	NON_WHITESPACE_CDATA_ENCOUNTERED
?	SPECIFIED_LAYOUT_INVALID
?	STYLESHEET_AND_INLINE_STYLE_TOO_LONG
?	STYLESHEET_TOO_LONG
?	TAG_EXCLUDED_BY_TAG
?	TAG_NOT_ALLOWED_TO_HAVE_SIBLINGS
?	TAG_REFERENCE_POINT_CONFLICT
?	TAG_REQUIRED_BY_MISSING
?	TEMPLATE_IN_ATTR_NAME
?	TEMPLATE_PARTIAL_IN_ATTR_VALUE
?	UNESCAPED_TEMPLATE_IN_ATTR_VALUE
?	UNKNOWN_CODE
?	VALUE_SET_MISMATCH
?	WARNING_EXTENSION_DEPRECATED_VERSION
?	WARNING_EXTENSION_UNUSED
?	WARNING_TAG_REQUIRED_BY_MISSING
?	WRONG_PARENT_TAG

[postphotos]

Related #1360.

[postphotos]

@westonruter - Thanks for the suggestion. This is closely related (duplicate?) To AC2 of #1364.

A few questions:

1. What format do you think this "reasoning" should be? Do you envision this being surfaced in the UI, in the WP-CLI tooling, both?

2. Rereading Generate an error taxonomy for AMP error types #1360, I see that @kienstra prepared a list of what the current caught errors are (based on CSS/HTML/JS rules of AMP) - are there additional errors we should be concerned about? FWIW, https is not on his list.

3. And would there be a listing somewhere that would explain both all the possible errors?

[kienstra]

Question About Not Including In Beta Release

Hi @postphotos,
Thanks for discussing this. It's a really good idea to display a message specific to the error, like:

And you were right that this was part of even the earlier designs.

But would you mind if we delayed this until for after the next Beta release, so that we've had time to iron out the exact error messages?

Thanks, Leo

[westonruter]

This is closely related (duplicate?)

It's not quite a duplicate. This issue is more for the underlying plumbing to get the information to surface. It's not about showing it per se. The reason could be stored in the validation error properties under a reason field, alongside code and error_type (aside: maybe this should just be type). Then with this reason in hand, the UI could display it.

[westonruter]

A reason could be illegal_protocol, for example.

[kienstra]

Moving Off v1.0 Board

If it's alright, I'm moving this off of the v1.0 board, as it looks like this won't be part of v1.0-RC1.

[amedina]

It would be great to have strings a-la Lighthouse audit results, providing information/advice in the context of each error type.

[westonruter]

I've started prototyping this.

[westonruter]

@schlessera points point, as part of this and #3730, we should capture the validation error messages and wrap them in translation functions as part of the spec PHP generator process, so that the strings can all be translated in WordPress.

[westonruter]

With #4401 these error messages (titles) are also displayed in the block editor warnings.