Decline only the file that declares the function of that name - #6288
Decline only the file that declares the function of that name#6288SanderMuller wants to merge 1 commit into
Conversation
wouldReIncludeALoadedFile() declined on any trapped read of an already-loaded file, but an aliasing autoloader reads the file of the class it aliases *to*: class_alias(Target::class, 'File') autoloads Target, and Target already being loaded is the normal case - class_alias() includes nothing. So the guard declined the very case it was written to keep working, and `use File;` still reported class.notFound with Laravel's AliasLoader. Only re-including the file that declares the function of that name can redeclare it, so compare the trapped read against that file. A built-in has no declaring file and nothing to redeclare. e2e/bug-15102b required its alias target upfront, which is why it passed: class_alias() then reads no file and the guard never fires. It now autoloads the target like AliasLoader does, and fails without this change. Reported and diagnosed by @hoetaek, including the suggested condition and a red-team case for the redeclare hazard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Red-teamed my own change, and it does narrow the guarantee - worth stating explicitly rather than leaving for someone to find. What the narrowed guard still protects (verified, not assumed): stubbing What it gives up. An autoloader that maps the name to a different already-loaded file now re-includes it: // bootstrap.php
require __DIR__ . '/helpers.php'; // declares zzcollide()
require __DIR__ . '/other.php'; // declares otherfn(), now loaded
spl_autoload_register(static function (string $class): void {
if ($class === 'zzcollide') {
require __DIR__ . '/other.php'; // different loaded file -> redeclares otherfn()
}
}, true, true);
So 2.2.9's protection here was collateral - a side effect of declining on the name alone, which is the same over-breadth that broke every facade alias. This branch restores 2.2.8 behaviour for that contrived shape while fixing the reported one. I could not find a rule that covers both cheaply. Anything keyed on "the located file is already included" re-breaks the alias case, because the alias target's file is already included - that is precisely why the guard fired. Parsing the located file for symbols that already exist fails the same way: the target's class exists, so it would decline again. The only signal that separates the two is whether the file declares the function with this exact name, which is what If you would rather keep the broader decline and accept aliases staying broken until a better signal exists, say so and I will close this - but the Laravel and TYPO3 reports are real users, and the case this gives up needs an autoloader that re-includes an unrelated loaded file, which is what |
|
Compare this with #6287 What's better? |
|
Yours is better, on three counts I could measure - closing this one. I ran every case I had against the merged tip (
One thing worth knowing about the fixtures: Thanks for taking it further rather than patching my premise - and @hoetaek's diagnosis stands, it was the trapped read of the alias target all along, just with a deeper cause than either of us attributed it to. |
Fixes the Laravel half of phpstan/phpstan#15102, which is still red on the merged tip - reported, diagnosed and patched by @hoetaek in #6265; I verified it and ran the gates he flagged as decisive.
What my #6265 guard got wrong
wouldReIncludeALoadedFile()declined on any trapped read of an already-loaded file. But an aliasing autoloader reads the file of the class it aliases to:class_alias(\Repro\Target::class, 'File')autoloadsRepro\Target, and that file already being loaded is the normal case - and the precondition for success, sinceclass_alias()includes nothing. So the guard declined exactly the case it was written to keep working, anduse File;still reportedclass.notFound.Reproduced on the merged tip (
c10897b1f) with @hoetaek's portable repro - prependedclass_aliasautoloader,Filecolliding with the built-infile(), no Laravel involved - each run with its own emptytmpDir:class.notFoundThe narrowing
Only re-including the file that declares the function of that name can redeclare it, so the trapped read is compared against that file. A built-in has no declaring file, so there is nothing it could redeclare.
The
new ReflectionFunctionis deliberate runtime reflection - the hazard is a runtime redeclare - and it gets a baseline entry next to the twoAutoloadSourceLocatoralready has for the same rule.Why the e2e project did not catch it
e2e/bug-15102b(mine, from #6265)require_once'd its alias target in the bootstrap, soclass_alias()read no file and the guard never fired. It now autoloads the target through Composer the wayAliasLoaderdoes. Verified both ways:Verification
e2e/bug-14988(the redeclare fatal the guard exists for),bug-15102,bug-15102b,bug-12972b,bug-12972c: all exit 0.Closes the Laravel half of phpstan/phpstan#15102