Map global variable receivers to their classes - #282
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. The exported-name change is deliberate and the body flags it; recommend landing after #284 so the corpus diff enumerates it.
Verified
- CI green on 7.4 and 8.4; mergeable, clean, based on
master. - The dead-mapping diagnosis is correct. In
Method_Call_Reflector::getName()the receiver is$printer->prettyPrintExpr( $caller )for anyExpr, so a global receiver prints as$wpdb, and_resolveName()passes it through untouched (it only rewrites$this/self/parent). The lookup isarray_key_exists( $caller, $class_mapping )against keys that had the sigil stripped — it could never match. Thesed "s/\\\$//g"stage in the generating command is the smoking gun, and removing that stage from the comment makes the documented pipeline actually reproduce the new keys. I traced the sed chain: it does leave the$intact, so the comment is now accurate rather than aspirational. $wp_functionsreally is unaffected, as claimed: those keys are built asnameToString( $caller->name ) . '()'from theFuncCallbranch, no sigil involved.- Static calls do not regress.
Static_Method_Call_Reflector::getName()overridesgetName()entirely and never consults the mapping, so the only other live path into it isExpr_New, where the receiver is a bareNode\Name. Under the old keys that path could mis-map a lowercase class name (new post(...)→WP_Post); after this change it cannot. Strictly fewer false positives there. The two real core cases (new wpdb(...),new wp_xmlrpc_server(...)) were identity mappings, so nothing moves. - The slug-rescue claim holds.
Relationships::import_item()builds$to_method['class'] . '-' . $to_method['name']and runs it throughsanitize_title(), which drops the$— so$wpdb-updateandwpdb-updatealready collapsed to the same slug. The cases that actually change are the ones where the variable name differs from the class:$wp_the_query-get→wp_query-getis a link that has been broken since the mapping was written. - Tests are RED on master.
assertFileUsesMethodroutes toentity_uses(), which matches onlineand then does an exactassertEquals— so the changed'class' => 'wpdb'expectation fails on master, and the new$wp_the_query->get( 'paged' )case at line 23 (confirmed against the.inc) assertsWP_Query, which master cannot produce. Not a subset assertion, so no tautology.
Worth noting
- The mapping is now live, so its staleness matters. Several targets no longer describe modern core:
$wp_filesystem => WP_Filesystem(core's object is aWP_Filesystem_Basesubclass;WP_Filesystemis a function),$wp_json => Services_JSON(gone from core),$phpmailer => PHPMailer(namespaced asPHPMailer\PHPMailer\PHPMailersince 5.5). These produce class names that will not resolve to a method post — no worse than today's unresolvable$wp_filesystem, so not a blocker, but the table is worth an audit pass now that it does something. Good follow-up issue. - The heuristic widens to locals. Any variable named
$post,$wp_query,$userdata, etc. now maps to a class whether or not it is the global. That is the documented intent of the table ("leverages globals for most used classes") but it has never actually been exercised, so it is a new source of plausible-but-wronguses.methods[].classvalues in non-core code. Acceptable for a parser aimed at core; worth knowing before someone reports it as a bug. - Land after #284 and re-run the corpus job: it will enumerate exactly which receivers changed, which turns "a corpus regeneration diff will show class-name changes" from a promise into a reviewable list.
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.
Method_Call_Reflector::_getClassMapping()maps well-known WordPress globals to their classes with keys like'wpdb', but the pretty-printed receiver it is matched against is'$wpdb'— the comment documenting the list's generation even shows thesed "s/\\$//g"stage that stripped the sigils. The$wp_globalshalf of the mapping has therefore never matched; the existing test pinned the fallout (class => '$wpdb'), evidently unintentionally. The$wp_functionshalf (get_current_screen()→WP_Screen) was unaffected.The keys now carry the
$sigil, so the mapping matches. This is a deliberate behavior change to exporteduses.methods[].classfor calls on these globals:$wp_the_query,$post,$authordata,$userdata,$wp_customize,$wp_hasher,$wp_json,$phpmailer,$custom_background,$custom_image_header— the exported class (and thus the method slug built from it) now points at the real method post. Those cross-links have been broken all along.$wpdb,$wp_query, …) only the exported string changes ($wpdb→wpdb); the link slug was already rescued bysanitize_title()dropping the$.The alternative — deleting the dead half — was considered and rejected: the mapping repairs real cross-links. A corpus regeneration diff will show class-name changes for these receivers; they are intended.
The updated
$wpdbexpectation and the new$wp_the_query→WP_Queryassertion fail on master and pass with the fix; the full suite passes.Found by the multi-agent review during #262; extracted as a standalone change.
🤖 Generated with Claude Code