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

Meta tag invalid property error #4722

Closed
joshcronin opened this issue May 14, 2020 · 9 comments · Fixed by #4866
Closed

Meta tag invalid property error #4722

joshcronin opened this issue May 14, 2020 · 9 comments · Fixed by #4866
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@joshcronin
Copy link

joshcronin commented May 14, 2020

Bug Description

The AMP validation is picking up an invalid property in the viewport meta tag even though the property it is saying is invalid does not exist.

Expected Behaviour

Viewport meta tag should not be flagged as invalid.

Steps to reproduce

  1. Include meta tag for viewport such as <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=5"/>
  2. Go to the AMP plugin "Validated URLs" page
  3. Open a specific page result page
  4. See error Invalid property: 0

Screenshots

image

Additional context

  • WordPress version: 5.4.1
  • Plugin version: 1.5.3
  • AMP plugin template mode: Transitional
  • PHP version: 7.2.24
  • OS: Ubuntu
  • Browser: All
  • Device: All

Originally reported in support forum topic.


Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter
Copy link
Member

Interesting. I’m not able to reproduce that issue.

I’ve tried editing Twenty Twenty’s header.php to supply this meta viewport:

<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=5"/>

When I validate the page, I get no validation error.

If I change it to:

<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1,maximum-scale=5,foo=bar">

Then I get one validation error: “Invalid property: foo”. It then says that the original invalid property value was:

width=device-width,initial-scale=1,minimum-scale=1,maximum-scale=5,foo=bar

Which is correct.

So I’m confused why this is happening for you. What happens if you remove the (unnecessary) trailing XML self-closing slash from the meta tag? So:

<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=5">

@schlessera
Copy link
Collaborator

According to the screenshot, you have a property 0=Array in your meta tag, which is the invalid property.

I guess that some code within the theme fails generating a valid property because it doesn't receive [ $key => $value ], but rather [[ $key => $value]], which is parsed to 0 => [ $key => $value ].

Finding that broken piece of code and fixing it should get rid of the above validation error.

@westonruter
Copy link
Member

@schlessera apparently there is no PHP generation of the meta tag though? @joshcronin can you share the full PHP code that is generating the tag?

@schlessera
Copy link
Collaborator

@westonruter Note that the screenshot shows something else than the issue description.

@schlessera
Copy link
Collaborator

Ah, just read through the support ticket. The GH issue here is misleading, actually.

I'm thinking it could also be a part of our processing code that fails in producing a valid attribute somewhere. I'll double-check.

@joshcronin
Copy link
Author

So I’m confused why this is happening for you. What happens if you remove the (unnecessary) trailing XML self-closing slash from the meta tag? So:

<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=5">

Removing the trailing self-closing slash makes no difference.

can you share the full PHP code that is generating the tag?

The tag is hard coded into the head, the code is:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=5"/>
    <?php wp_head(); ?>
</head>

@westonruter westonruter added the Bug Something isn't working label May 18, 2020
@westonruter
Copy link
Member

I just saw something pop up in a support forum topic which appears to be a similar issue here:

PHP Notice: Array to string conversion in …amp/includes/sanitizers/class-amp-style-sanitizer.php on line 3405

This is line in question:

return $property_name . '=' . $viewport_rules[ $property_name ];

@westonruter
Copy link
Member

@joshcronin Do you have @viewport (with or without vendor prefixes) in your stylesheets?

I found a way to reproduce the problem, with this plugin code:

add_action( 'wp_head', function () {
	?>
	<style>
		@media screen {
			@viewport {
				width: device-width;
			}
		}
	</style>
	<?php
} );

The fix is to correct how $viewport_rules are merged:

--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -2079,7 +2079,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
 				if ( ! $sanitized ) {
 					$at_rule_processed_list = $this->process_css_list( $css_item, $options );
 					if ( ! empty( $at_rule_processed_list['viewport_rules'] ) ) {
-						$viewport_rules[] = $at_rule_processed_list['viewport_rules'];
+						$viewport_rules = array_merge( $viewport_rules, $at_rule_processed_list['viewport_rules'] );
 					}
 
 					$validation_results = array_merge(

@westonruter
Copy link
Member

Since the PR has been fixed and the commit cherry-picked onto the 1.5 branch, you can test the most recent 1.5.4-alpha build here: https://storage.googleapis.com/ampwp_github_artifacts/refs/heads/1.5/prod/amp.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants