Fix relationship name-to-post resolution - #280
Open
sirreal wants to merge 2 commits into
Open
Conversation
sirreal
commented
Aug 20, 2026
sirreal
left a comment
Member
Author
There was a problem hiding this comment.
Note
This is an agentic review, generated by Claude Code at the repository owner's request.
Ready to land. Three genuine bugs, three minimal fixes, tests that actually fail on master.
Verified
- CI green on 7.4 and 8.4; mergeable, clean, based on
master. - Bug 1 — swapped
strpos()arguments.strpos( '\\', $name )searches a one-character haystack for$name; it returnsfalsefor every realistic input, so$fully_qualifiedwas permanentlyfalseand\Foo\bar()called inside namespaceBazstill gotBaz\Foo\baras its first candidate.strpos( $name, '\\' ) === 0is the right test, and it matches the resolution rules the method's own docblock describes. - Bug 2 —
continuewherebreakwas meant. The inner loop walks the candidate chain most-specific-first;continueadvances to the next candidate and keeps matching, so a name that exists both namespace-scoped and globally produced two entries in$slugs_with_idsand therefore two p2p connections. The comment on the line already said "stop searching the chain".breakexits the innerforeachonly, leaving the outerforeach ( $slugs as ... )intact — correct scope. - Bug 3 — post ID 1. Confirmed by reading the second loop: on
master, when$this->slugs_to_ids[ $to_type ]is empty thecontinueleaves$this->relationships[...][ $to_type ]holding the raw array-of-candidate-arrays. The connection loop then doesintval( $to_id, 10 )on an array, which is1for any non-empty array, passes the0 != $to_idguard, and connects the item to whatever post holds ID 1. Assigningarray()before thecontinueis the right minimal fix — the connection loop iterates nothing, and nothing else reads that key. Note the assignment is safe against the surrounding iteration:$to_typescomes from(array) @$this->relationships[ $from_type ], a by-value copy, and the non-empty branch already writes the same key. - Bugs 1 and 2 interact, in the right direction. Together they mean a
\-qualified call inside a namespace now resolves to exactly one target — the global one — instead of possibly connecting to a namespace-local post as well. - RED/GREEN structure is as described: 3 of 5 fail on
master.test_names_to_slugs_fully_qualified,test_get_ids_for_slugs_first_matching_scope_wins, andtest_ending_import_with_empty_slug_map_makes_no_connections(its first assertion — the raw candidate arrays survive on master) are RED;test_names_to_slugs_unqualifiedandtest_get_ids_for_slugs_unknown_slug_is_ignoredpin already-correct behavior. No tautologies. The public visibility of$slugs_to_ids/$relationshipsmakes the direct-injection setup legitimate rather than a hack. - PHP 7.4-clean.
Worth noting
- Confirmed the
'data'vs'date'p2p connection meta typo is noted in the description and left alone — the functions branches pass'date' => current_time( 'mysql' )while all three methods branches pass'data' =>. Agreed it does not belong in this PR; it does deserve its own issue, since fixing it changes stored connection meta and anything readingdateoff amethods_to_*connection has been reading nothing. - This is importer/relationship code, so the corpus-diff check in #284 will never cover it — the corpus diff compares exporter output only. These unit tests are the entire safety net for this file, which is exactly why adding the first
tests/phpunit/tests/relationships.phpmatters. - Minor doc drift:
names_to_slugs()'s@returnstill says "starting with the context of the namespace, and falling back to the global namespace", which is now only true for unqualified names. The prose above it already explains the\rule; a one-line touch-up would close the gap.
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.
Three bugs in
lib/class-relationships.phpbroke the name→slug→post-ID resolution that builds p2p connections:names_to_slugs()never detects fully qualified names.strpos( '\\', $name )has haystack and needle swapped, so$fully_qualifiedis always false and a\-prefixed call is still resolved against the current namespace first. PHP semantics say a fully qualified name resolves only globally; now it does. This deliberately changes candidate order for\-qualified calls made inside a namespace.get_ids_for_slugs()connects every matching scope. The loop says "stop searching the chain" but usescontinue, so when both the namespace-scoped and the global candidate resolve to posts, both are connected — duplicate/spurious p2p connections. Now the first (most specific) match wins.An empty slug map connects items to post ID 1. When no posts of a target type were imported, the raw array-of-candidate-arrays was left in
$relationships, and the connection loop'sintval( array, 10 )coerces each to1— connecting items to whatever post has ID 1. The candidates are now cleared so nothing reaches the connection loop. This is theTODO why might this be empty? test class-IXR.phpspot.The new tests fail on master (3 of 5; the other two pin existing correct behavior) and pass with the fix; the full suite passes.
Observed while here, left alone: the methods branches of the connection loop pass
'data' => current_time( 'mysql' )where the functions branches pass'date' =>— looks like a typo, but changing stored p2p meta is out of scope for this fix.Found by the multi-agent review during #262; extracted as a standalone change.
🤖 Generated with Claude Code