Skip to content

Map global variable receivers to their classes - #282

Open
sirreal wants to merge 2 commits into
masterfrom
fix-global-receiver-class-mapping
Open

Map global variable receivers to their classes#282
sirreal wants to merge 2 commits into
masterfrom
fix-global-receiver-class-mapping

Conversation

@sirreal

@sirreal sirreal commented Aug 18, 2026

Copy link
Copy Markdown
Member

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 the sed "s/\\$//g" stage that stripped the sigils. The $wp_globals half of the mapping has therefore never matched; the existing test pinned the fallout (class => '$wpdb'), evidently unintentionally. The $wp_functions half (get_current_screen()WP_Screen) was unaffected.

The keys now carry the $ sigil, so the mapping matches. This is a deliberate behavior change to exported uses.methods[].class for calls on these globals:

  • Where the variable name differs from the class — $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.
  • Where they coincide ($wpdb, $wp_query, …) only the exported string changes ($wpdbwpdb); the link slug was already rescued by sanitize_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 $wpdb expectation and the new $wp_the_queryWP_Query assertion 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

@sirreal sirreal left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 any Expr, so a global receiver prints as $wpdb, and _resolveName() passes it through untouched (it only rewrites $this/self/parent). The lookup is array_key_exists( $caller, $class_mapping ) against keys that had the sigil stripped — it could never match. The sed "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_functions really is unaffected, as claimed: those keys are built as nameToString( $caller->name ) . '()' from the FuncCall branch, no sigil involved.
  • Static calls do not regress. Static_Method_Call_Reflector::getName() overrides getName() entirely and never consults the mapping, so the only other live path into it is Expr_New, where the receiver is a bare Node\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 through sanitize_title(), which drops the $ — so $wpdb-update and wpdb-update already collapsed to the same slug. The cases that actually change are the ones where the variable name differs from the class: $wp_the_query-getwp_query-get is a link that has been broken since the mapping was written.
  • Tests are RED on master. assertFileUsesMethod routes to entity_uses(), which matches on line and then does an exact assertEquals — 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) asserts WP_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 a WP_Filesystem_Base subclass; WP_Filesystem is a function), $wp_json => Services_JSON (gone from core), $phpmailer => PHPMailer (namespaced as PHPMailer\PHPMailer\PHPMailer since 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-wrong uses.methods[].class values 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.

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.

1 participant