fix(post): guard the post_author read in format_hits_as_posts - #4354
Open
freibergergarcia wants to merge 1 commit into
Open
fix(post): guard the post_author read in format_hits_as_posts#4354freibergergarcia wants to merge 1 commit into
freibergergarcia wants to merge 1 commit into
Conversation
format_hits_as_posts() copies a fixed list of properties from each
Elasticsearch hit onto the post object it returns. Every property is
read behind an isset() check except post_author, which is special-cased
and read unconditionally through a nested ['id'] subscript.
Any query that narrows _source via ep_formatted_args while leaving
fields at its default is routed to this formatter, so a document
without post_author produces two PHP warnings for every hit. The
plugin's own narrowing (maybe_set_fields, for fields => ids and
id=>parent) always pairs with a different formatter, which is why this
is only reachable from third-party code.
A document whose post_author is a string rather than the indexed object
is worse than noisy: the subscript raises an uncaught TypeError
("Cannot access offset of type string on string").
Guarding the read makes post_author behave like its 20 siblings, which
leave a missing property unset so WP_Post supplies its own default. A
valid id is still copied, including 0 and the empty string the plugin
indexes for a deleted user.
Adds a data-provider test covering every post_author shape a document
can carry, plus a test that a narrowed _source builds posts cleanly.
Five of the nine cases fail without the fix, four of them as errors
raised by the suite's warning-to-exception conversion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CaiQXpxDeoy1XXh4y9PPzx
freibergergarcia
force-pushed
the
fix/post-author-unguarded-read
branch
from
August 13, 2026 08:47
600ee58 to
5f01787
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the Change
QueryIntegration::format_hits_as_posts()copies a fixed list of properties from each Elasticsearch hit onto the post object it returns. Every property is read behind anisset()check exceptpost_author, which is special-cased and read unconditionally through a nested['id']subscript:ElasticPress/includes/classes/Indexable/Post/QueryIntegration.php
Lines 476 to 486 in 9001816
Because the subscript is unguarded, what happens depends entirely on the shape of the value in the document:
post_authorin the document[... 'id' => 5 ]5[ 'id' => 0 ]0[ 'id' => '' ](indexed for a deleted user,Post.php:444-449)''[ 'id' => null ]null, no warningidmissingnull+Undefined array key "id"null+ 2 warningsnull+Trying to access array offset on value of type intTypeError: Cannot access offset of type string on stringThe common route to the middle rows is a query that narrows
_sourcethroughep_formatted_argswhile leavingfieldsat its default, which routes hits to this formatter. That produces two warnings for every hit, on every such query.ElasticPress never trips this itself, which is why it has survived so long. The plugin's own
_sourcenarrowing (Post::maybe_set_fields(), forfields => 'ids'and'id=>parent') always pairs withformat_hits_as_ids()/format_hits_as_id_parents(), formatters that expect a narrow document. Only third-party code reaches the combination of a narrowed_sourceand the default formatter.The change. Give
post_authorthe same guard its 20 siblings already have:A valid id is still copied, including
0and the empty string indexed for a deleted user, sinceisset()is true for both. Anything unusable leaves the property unset, exactly as the sibling properties already do, soWP_Postsupplies its own default.Benefits. Removes the log noise, and removes a fatal error path. Sites narrowing
_sourcefor performance no longer have to choose between the optimisation and a clean log.History suggests this was an oversight rather than a decision.
d96e67588(2015-06-03) introduced the copy loop and thepost_authorspecial case together, with nothing guarded. A month later9bd776390(2015-07-02) changed} else {to} elseif ( isset( $post_array[ $key ] ) ) {— commit subject "isset and unit test using ep_search_post_return_args filter to test. Fixes #306", a deliberate fix for a key in the return-args list being missing from the document. That is the same failure modepost_authorstill has; the fix simply did not extend into the branch above it. Everything since is cosmetic:ba55e0fe87moved the loop,aaf3a97d9ewas PHPCS whitespace,45a6d34df2applied Yoda conditions.Alternative considered, and a question for maintainers. The other candidate fix is
$post->$key = $post_array[ $key ]['id'] ?? null;. Both silence the warnings and theTypeError; they differ only in what a caller sees when there is no usable id:?? nullisset()guard (this PR)null'0'(WP_Post default)isset( $post->post_author )falsetruepost_authorviaep_search_post_return_argsnullvs'0')I went with the guard because it makes all the "no author data" paths agree and yields a value of the type
WP_Postdeclares, but?? nullis the smaller behavioural delta and is a defensible preference. It is a one-line swap plus test expectations — happy to switch if you would rather preservenull.Not addressed here: a
post_authorarriving as an object still fatals (Cannot use object of type stdClass as array), before and after this change. It seemed out of scope for a guard on the documented shape, but say the word if you would like it covered.Closes #
How to test the Change
Automated:
composer run setup-local-tests # if not already set up (needs MySQL + Elasticsearch) EP_HOST=http://127.0.0.1:8890/ composer run test-single-site -- --filter testFormatHitsAsPostsNine cases pass. Reverting just the
includes/classes/Indexable/Post/QueryIntegration.phphunk and re-running fails five of them — four as errors raised by the suite'sconvertWarningsToExceptions, one on the value.Manually, on any indexed site with
WP_DEBUGon:_sourceon a marked query:new WP_Query( array( 'ep_integrate' => true, 'my_id_only_query' => true, 'posts_per_page' => 100 ) );Undefined array key "post_author"andTrying to access array offset on ..., two per hit. After, the log is clean and the posts come back unchanged.To see the fatal, return a string from
ep_retrieve_the_postforpost_authorand run anyep_integratequery.Changelog Entry
Credits
Props @freibergergarcia
Checklist: