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

Ensure array query parameters do not contain empty items #2462

Merged
merged 4 commits into from
Dec 14, 2021

Conversation

roborourke
Copy link
Contributor

Description of the Change

In some cases query args like post__in that are expected to be array might contain invalid data such as an empty string. An array like [ '' ] will not appear as empty when checked with empty( [ '' ] ) so this update ensures the arrays only contain real values. Database IDs all start from 1 so removing 0 is ok here.

This issue was observed with Yoast SEO adding subqueries for indexables to populate post__in parameters, however the index was incomplete. Can also happens in non-production environments where indexables aren't saved to the database.

Alternate Designs

Benefits

Prevents ES query errors such as this one:

Warning[16-Nov-2021 09:00:26 UTC] PHP Warning:  Error in ElasticSearch request: {"error":{"root_cause":[{"type":"query_shard_exception","reason":"failed to create query: empty String","index_uuid":"nRExUs6-TSCUfwX3cBDodg","index":"examplecom-post-2"}],"type":"search_phase_execution_exception","reason":"all shards failed","phase":"query","grouped":true,"failed_shards":[{"shard":4,"index":"examplecom-post-2","node":"-ftiX3JgRtC_yi8bXpnR9A","reason":{"type":"query_shard_exception","reason":"failed to create query: empty String","index_uuid":"nRExUs6-TSCUfwX3cBDodg","index":"examplecom-post-2","caused_by":{"type":"number_format_exception","reason":"empty String"}}}]},"status":400} (400) in /usr/src/app/vendor/altis/cloud/inc/namespace.php on line 365

Note this part:

"caused_by":{"type":"number_format_exception","reason":"empty String"}

Possible Drawbacks

Verification Process

I added this code to as a mu-plugin reproduce the WP Query args as a minimal test case:

add_action( 'pre_get_posts', function( \WP_Query $query ) : void {
	$query->set( 'post__not_in', [ '' ] );
}  );

Observe running a search query yields no results (or falls back to a db query).

The change is intentionally light in terms of sanitisation and I haven observed no regressions with valid values for the listed query arguments, even if provided as strings e.g. (array) "some string" -> [ "some string" ].

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

In some cases query args like `post__in` that are expected to be array might contain invalid data such as an empty string. An array like `[ '' ]` will not appear as empty when checked with `empty( [ '' ] )` so this update ensures the arrays only contain real values. Database IDs all start from 1 so removing `0` is ok here.

This issue was observed with Yoast SEO adding subqueries for indexables to populate `post__in` parameters, however the index was incomplete. Can also happens in non-production environments where indexables aren't saved to the database.
@brandwaffle
Copy link
Contributor

@roborourke thanks for submitting the PR! We'd like to include this in 3.6.5 which will be shipping next week, but that would require fixing the failing tests. Is this something you would have time for in the short term? If not, we can always slot this for a later release. Thanks in advance!

@roborourke
Copy link
Contributor Author

@brandwaffle I’ll have a crack at the tests tomorrow, some of the failures looked unrelated so I might need a bit of help

includes/classes/Indexable/Post/Post.php Outdated Show resolved Hide resolved
includes/classes/Indexable/Post/Post.php Outdated Show resolved Hide resolved
@mckdemps mckdemps added this to the 3.6.6 milestone Dec 14, 2021
@felipeelia felipeelia merged commit 07f6dd2 into 10up:develop Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants