-
Notifications
You must be signed in to change notification settings - Fork 585
Consult bootstrap-registered custom autoloaders only after the static source locators #6069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0e6950a
15b3d42
c9ba64d
d186cbd
3cdb0fa
3c7833b
f04e5dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| /vendor/ | ||
| composer.lock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <?php | ||
|
|
||
| require_once __DIR__ . '/vendor/autoload.php'; | ||
|
|
||
| // Registered *after* Composer's class loader: the third argument (prepend) | ||
| // defaults to false, so this ends up at the back of the spl_autoload queue. | ||
| // | ||
| // \other12972\MyClass is part of Composer's class map (see composer.json), so at | ||
| // runtime Composer's class loader resolves it first and this autoloader is never | ||
| // invoked for it - running `php real-world.php` therefore never throws. | ||
| // | ||
| // PHPStan must mirror that order: the Composer class map source locator has to | ||
| // resolve the class before this bootstrap autoloader is consulted. Analysing the | ||
| // project used to invoke this autoloader first, hitting the LogicException and | ||
| // crashing with an internal error that cannot happen at runtime. | ||
| spl_autoload_register(function($class) { | ||
| if ($class === \other12972\MyClass::class) { | ||
| throw new LogicException('this should not happen'); | ||
| } | ||
| }); | ||
|
Comment on lines
+3
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the current fix depends on the order in which autoloaders are registered within bootstrap scripts (and whether they are prepend'ed or not). in case we change the we would get back a similar runtime vs. static analysis inconsistency. can we detect in which order autoloaders are registered in the bootstrap scripts?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All verification passes. Here's a summary of what I did to address staabm's review. The review concernstaabm pointed out that the PR's fix (moving all bootstrap autoloaders to the back of the source-locator chain) depends on registration order: an autoloader registered before Composer's class loader — e.g. What I implementedYes — I now detect each bootstrap autoloader's position relative to Composer's
Tests
Verification
Both commits are pushed to |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "autoload": { | ||
| "classmap": [ | ||
| "src/" | ||
| ] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| parameters: | ||
| level: 9 | ||
|
|
||
| paths: | ||
| - src | ||
|
|
||
| bootstrapFiles: | ||
| - autoloader.php |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <?php | ||
|
|
||
| require 'autoloader.php'; | ||
|
|
||
| $root = new \Foo12972\MyRoot(); | ||
| $root->doBar(new \other12972\MyClass()); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <?php | ||
|
|
||
| namespace Foo12972; | ||
|
|
||
| use other12972\MyClass; | ||
|
|
||
| class MyRoot { | ||
| function doBar(MyClass $myClass):void {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <?php | ||
|
|
||
| namespace other12972; | ||
|
|
||
| class MyClass { | ||
| public function doSomething(): int | ||
| { | ||
| return 1; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| /vendor/ | ||
| composer.lock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| <?php | ||
|
|
||
| require_once __DIR__ . '/vendor/autoload.php'; | ||
|
|
||
| // Registered *before* Composer's class loader (third argument = prepend). | ||
| // At runtime this resolves \shared12972c\Thing before Composer ever sees it, | ||
| // so PHPStan must consult it before the Composer class map as well. | ||
| spl_autoload_register(function ($class) { | ||
| if ($class === \shared12972c\Thing::class) { | ||
| require __DIR__ . '/prepended/Thing.php'; | ||
| } | ||
| }, true, true); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <?php | ||
|
|
||
| namespace shared12972c; | ||
|
|
||
| class Thing { | ||
| public function composerOnly(): int | ||
| { | ||
| return 1; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "autoload": { | ||
| "classmap": [ | ||
| "classmap/" | ||
| ] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <?php | ||
|
|
||
| namespace consumer12972c; | ||
|
|
||
| use shared12972c\Thing; | ||
|
|
||
| function run(Thing $thing): int | ||
| { | ||
| return $thing->prependedOnly(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| parameters: | ||
| level: 9 | ||
|
|
||
| paths: | ||
| - consumer | ||
|
|
||
| bootstrapFiles: | ||
| - autoloader.php |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <?php | ||
|
|
||
| namespace shared12972c; | ||
|
|
||
| class Thing { | ||
| public function prependedOnly(): int | ||
| { | ||
| return 2; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <?php | ||
|
|
||
| require 'autoloader.php'; | ||
|
|
||
| $thing = new \shared12972c\Thing(); | ||
| echo $thing->prependedOnly(), "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment why/how this test case works and add a note about the autoload function is using prepend=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the review comment.
What I changed
e2e/bug-12972b/autoloader.php— added a comment above thespl_autoload_register()call explaining:false, so this autoloader lands at the back of thespl_autoloadqueue, after Composer's class loader. (This is the deliberate counterpart toe2e/bug-12972c, which passestrue, trueto prepend before Composer and already carried an equivalent note.)\other12972\MyClassis in Composer's class map (composer.json), so at runtime Composer resolves it first and this autoloader is never invoked for it —php real-world.phpnever throws.LogicException, and crashed with an internal error that cannot occur at runtime.Verification
make phpstan— No errorsmake tests— 21220 tests, 96743 assertions, OK (97 skipped)php -lon the changed file — cleanCommitted as "Document how the bug-12972b e2e autoloader reproduces the issue" and pushed to
create-pull-request/patch-89ch8pu.