16. Send the user back only to a screen the admin can serve - #80
Open
nikolaystrikhar wants to merge 2 commits into
Open
16. Send the user back only to a screen the admin can serve#80nikolaystrikhar wants to merge 2 commits into
nikolaystrikhar wants to merge 2 commits into
Conversation
…nothing The docblock said `string|null`, the parameter was declared as nothing, the guard refused anything that was not a non-empty string, and Resolver -- the one caller -- had already made a string of $_SERVER['REQUEST_URI'] before it called. Four answers to one question. Narrowed the documented type to what the caller really passes rather than widening it to what the guard tolerates. `string` is a promise callers can be held to statically: Resolver's own narrowing is now checkable instead of redundant, and a host reaching for this class is told what to hand over. Widening the docblock to `mixed` would have thrown that away to describe a runtime backstop, and declaring the type would have been worse still -- the value comes from the SAPI and any plugin may have filtered $_SERVER, so under strict_types a broken one would fatal from inside plugins_loaded, on the request whose job is to get the admin back to a working screen. So the type is documented and not declared, and the guard stays exactly as wide as it was. The test that pinned the guard now runs over null, an array, an integer and a boolean, since every one of those has to land on the plugins list rather than raise. `null` moves out of the request-URI provider to join them.
The pattern said a basename was well formed, and the class read that as naming an admin screen. `/wp-content/plugins/give/give.php`, `/wp-login.php` and `/wp-admin/edits.php` all satisfy it, so each was rebuilt as an admin URL for a file that is not in wp-admin -- an unstyled 404 from the web server, on the one request whose job is to hand the admin back the screen they were on. The documented answer for a URI naming no admin screen is the plugins list, and that is what all three take now. Asked of the filesystem, not of a list of core's screens. A list would be wrong the first time a plugin registered a top-level page, and the class has to keep working for a host's own screens: those are `admin.php`, `edit.php` or `tools.php` with a `page` argument on them, so the file is core's however many screens hang off it, and `is_file()` says yes to every one of them without knowing any of their names. What it cannot tell apart is a screen from one of the admin's own includes -- a browser is only ever on the first kind, and both are inside wp-admin either way. Asked of the admin the destination will be built in, matching admin_url_for()'s three branches. The network and user admins hold only the files core gives them, so a network request naming `options-general.php` was never on that screen, and network_admin_url() would name the file that is missing. The name still meets the pattern first, which is what keeps a crafted URI from climbing out of the directory being asked about, and the `\z` anchor is still the only thing refusing a trailing newline: the test for it stubs `is_file()` true, since there is no "edit.php\n" on disk either and the anchor could otherwise be taken out with nothing noticing.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
1 participant
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.
What:
Conflict\Redirector::after_deactivation()documents thestringits one caller passes, and returns only a screen the current admin has a file for — everything else takes the documentedplugins.phpfallback.Usage: an admin resolving a conflict from
/wp-admin/admin.php?page=give-settingsstill lands back on that page; one whose request URI names/wp-login.php, a plugin's owngive.phpor a typo'dedits.phpnow lands on the plugins list instead of on a wp-admin 404.Why this way:
The documented type narrows to what is really passed, and the guard stays as wide as it was.
Conflict\Resolvermakes a string of$_SERVER['REQUEST_URI']before it calls, sostringis a promise static analysis can hold callers to; declaring it on the signature would answer a host's filtered$_SERVERwith aTypeErrorfrom insideplugins_loaded, which is the one thing this library never does.A well-formed basename is not a screen. The pattern admitted any
*.php, so a URI naming one was rebuilt as an admin URL for a file that is not in wp-admin — a bare 404 on the request whose whole job is to put the admin back where they were.Asked of the filesystem, not of a list of core's screens. A list would be wrong the first time a plugin registered a top-level page; a host's screens are
admin.php,edit.phportools.phpwith apageargument, sois_file()says yes to screens nobody has written yet — and it is asked in the admin the destination will be built in, because the network and user admins hold only the files core gives them.The pattern still runs first, and still with
\z. It is what keeps a crafted name from climbing out of the directory being asked about; the anchor test stubsis_file()true so the anchor stays the only thing refusing a trailing newline.