Error: s.sources is not iterable

[westonruter]

Bug Description

As reported in a support forum topic:

The culprit may be one of these lines:

amp-wp/assets/src/block-validation/components/error/get-error-source-title.js

Line 20 in c2b9b38

for ( const source of sources ) {

amp-wp/assets/src/block-validation/hooks/use-validation-error-state-updates.js

Line 214 in c2b9b38

for ( const source of validationError.error.sources ) {

amp-wp/assets/src/components/site-scan-context-provider/get-sources-from-scannable-urls.js

Line 32 in c2b9b38

for ( const source of validationError.sources ) {

I suggest we investigate what could be causing sources to end up not being an array (like either null or {}) and then guard against that case.

Support topics:

• https://wordpress.org/support/topic/r-sources-not-iterable/
• https://wordpress.org/support/topic/s-sources-is-not-iterable/

Expected Behaviour

The sources prop should never be a non-array.

Screenshots

No response

PHP Version

No response

Plugin Version

2.2

AMP plugin template mode

Standard, Transitional, Reader

WordPress Version

No response

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

[westonruter]

cc @delawski

[milindmore22]

Another report for same issue

Possible cause :

 "url": "https://viodi.com/wordpress",
  "home": "https://viodi.com",

[westonruter]

For that support topic, it's interesting to note that the error is slightly different: r.sources is not iterable. So here the object is r but in the original topic the object is s. That would seem to indicate it's happening in multiple places, perhaps two of the code instances I cited.

[westonruter]

I was able to reproduce the issue. In looking at the preloaded data for scannable-urls, the search results item included this:

"validation_errors":[{"node_name":"script","parent_name":"body","code":"DISALLOWED_TAG","type":"js_error","node_attributes":[],"text":"document.write(__DOUBLE_QUOTED_STRING__);","node_type":1,"sources":null}]

I'm not sure why it was saved with null, and after rescanning it is no longer null.

[westonruter]

I think this code may be the problem:

amp-wp/includes/validation/class-amp-validation-manager.php

Lines 785 to 799 in bf8aa7f

	public static function add_validation_error( array $error, array $data = [] ) {
	$node = null;
	$sources = null;
	if ( isset( $data['node'] ) && $data['node'] instanceof DOMNode ) {
	$node = $data['node'];
	}
	if ( self::is_validate_request() ) {
	if ( ! empty( $error['sources'] ) ) {
	$sources = $error['sources'];
	} elseif ( $node ) {
	$sources = self::locate_sources( $node );
	}
	}

Notice how $sources is set to have an initial value of null, and then it is set to be an array only if $error['sources'] is not empty or $node is not empty. But in the else condition, nothing happens and so it can remain null. That then causes the error to happen.

Rather than make sure that every possible sources is set to be an array (including in data stored), I think it may be simpler just to treat sources as nullable in JS, and to check to see if it is an array before looping over it.

[westonruter]

Or maybe actually even simpler to just ensure that sources is an array in the REST API response:

diff --git a/src/Validation/ScannableURLsRestController.php b/src/Validation/ScannableURLsRestController.php
index f5117a768..762689789 100644
--- a/src/Validation/ScannableURLsRestController.php
+++ b/src/Validation/ScannableURLsRestController.php
@@ -189,7 +189,15 @@ public function prepare_item_for_response( $item, $request ) {
 
 			$data = json_decode( $validated_url_post->post_content, true );
 			if ( is_array( $data ) ) {
-				$item['validation_errors'] = wp_list_pluck( $data, 'data' );
+				$item['validation_errors'] = array_map(
+					static function ( $validation_error ) {
+						if ( ! isset( $validation_error['sources'] ) || ! is_array( $validation_error['sources'] ) ) {
+							$validation_error['sources'] = [];
+						}
+						return $validation_error;
+					},
+					wp_list_pluck( $data, 'data' )
+				);
 			}
 
 			$item['validated_url_post'] = [

(This may also need to handle ensuring nested sources also being arrays.)

[westonruter]

This would need to be combined with fixing the non-cached validation response as well:

diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
index 4ab6746c0..b14accf4e 100644
--- a/includes/validation/class-amp-validation-manager.php
+++ b/includes/validation/class-amp-validation-manager.php
@@ -784,7 +784,7 @@ public static function has_cap( $user = null ) {
 	 */
 	public static function add_validation_error( array $error, array $data = [] ) {
 		$node    = null;
-		$sources = null;
+		$sources = [];
 
 		if ( isset( $data['node'] ) && $data['node'] instanceof DOMNode ) {
 			$node = $data['node'];

[delawski]

@westonruter Thanks for the investigation.

I was able to reproduce the issue.

Can you share more on this? I tried reproducing the error on a multisite instance but with no luck.

[westonruter]

Can you share more on this? I tried reproducing the error on a multisite instance but with no luck.

That's a good question. It was happening when I opened the Settings screen, but I don't know how I caused it! I then reproduced the issue by manually causing the condition to happen. I'm trying to find a way to reproduce the issue otherwise.

[westonruter]

Another report for same issue

Possible cause :

 "url": "https://viodi.com/wordpress",
  "home": "https://viodi.com",

I created an environment like this but I got no issues with detecting validation error sources. I tried with Contact Form 7.

I also tried creating a multisite subdirectory install, and for a subdirectory subsite I also didn't get any errors from detecting sources.

So the kind of WordPress install may not be the issue. It may be a specific kind of error scenario that is resulting in null as sources.

[westonruter]

I'm at a loss for how to replicate the issue (other than by forcing the error state by modifying the code). Maybe @milindmore22 or @fellyph will have better luck based on the active plugins from the users?

If we can't find the cause, I think we'll need to end up merging #6846 and then investigate further the failure to locate sources later.

[westonruter]

User QA verified fix:

[dhaval-parekh]

✅ QA Passed.

The Site scanning on AMP setting page work fine even when the error source is null.

[fellyph]

Tested on 2.2.x and ✅ QA Passed.

[zdenys]

Just wanted to let share that some WordPress.com customers have also been affected by this bug. Is there a timeline for when version 2.2.1 will be released?

[westonruter]

@zdenys The plan is to release 2.2.1 today.

[westonruter]

@zdenys It was released several hours ago: https://github.com/ampproject/amp-wp/releases/tag/2.2.1