Fix stub parent class not loaded. #7110
Conversation
|
It looks like stub files are always loaded last, and changing that causes a bunch of tests to break. Not sure how much work it would take to fix that, although that seems like it would be the preferable solution. I'm guessing it was done this way to make sure actual classes were preferred over stubs? |
|
What are you trying to fix anyway?
It's the opposite, actually. Stubs are loaded last to make sure they override everything they need to override. |
I noticed this in #7107 after disabling the soap extension, |
The issue is not supposed to be emitted for stubs. Otherwise you would not be able to override wrong signatures coming from libraries/reflection - and it's one of the main problems solved by stubs. |
It shouldn't be emitted when checking the stub itself, but shouldn't it be emitted when checking a non-stubbed class that extends a stubbed class? |
|
Ok, gotcha. Yes, it should be emitted in this case. |
| private function populateDataFromTraits( | ||
| private function populateDataFromTrait( |
There was a problem hiding this comment.
I pulled the loops out of these functions because in a previous version I was running them individually on specific dependencies. I later realized that line 295 already reruns the entire thing anyway, but I left the changes here because there were other improvements done as well.
| private function populateOverriddenMethods( | ||
| ClassLikeStorage $storage | ||
| ClassLikeStorage $storage, | ||
| ClassLikeStorageProvider $storage_provider | ||
| ): void { |
There was a problem hiding this comment.
This was pulled out of populateDataFromImplementedInterfaces when I removed the loop from it, so that it could be done afterward. I think it's actually easier to understand this way, one issue that took me a while to figure out is that $storage->class_implements may already have had items set elsewhere (from Reflection I believe), so this is actually populating overridden methods that were set elsewhere as well as from this class.
| @@ -601,255 +691,152 @@ function ($constant) { | |||
| $storage->pseudo_methods += $parent_storage->pseudo_methods; | |||
| } | |||
|
|
|||
| private function populateInterfaceDataFromParentInterfaces( | |||
| private function populateInterfaceData( | |||
There was a problem hiding this comment.
This is common code pulled out of populateInterfaceDataFromParentInterfaces and populateDataFromImplementedInterfaces.
AndrolGenhald
force-pushed
the
bugfix/stub-parent-class
branch
from
8ceb0e2
to
ae169c9
Compare
|
Seems ok to me. If @weirdan approves, we can merge |
|
@weirdan Any update on this? |
| @@ -373,7 +373,7 @@ class ClassLikeStorage | |||
| public $initialized_properties = []; | |||
|
|
|||
| /** | |||
| * @var array<string> | |||
| * @var array<string, true> | |||
There was a problem hiding this comment.
This is BC break. Good thing that the PR already targets master.
There was a problem hiding this comment.
Oh, good point, I'd completely forgotten about plugins. I'll take more care about that in the future.
There was a problem hiding this comment.
I don't think that change is really needed anymore, but unless you object I don't really see a need to change it back. It seems better to me for it to be a set rather than a list, and array<string, true> seems like the closest thing PHP has to a native string set without using the ds extension.
Or maybe it'd be better to change it back and methodically update a lot of these types of properties to be more set-like in a separate pull request instead?
Looks like I actually did end up using it to fix indirect circular references. Still, is changing more of these to be set-like something we should consider?
|
Sorry, I have been busy with v5 changes. I'll review it this week. |
|
@AndrolGenhald can you rebase this? We'll merge it like that I think. |
AndrolGenhald
force-pushed
the
bugfix/stub-parent-class
branch
from
ae169c9
to
3510f55
Compare
|
@orklah Done. There are some CS errors locally due to empty files, looks like Edit: Just a moment, I'll add a BC break note as well. |
fixed, sorry |
orklah
added
the
release:fix
|
Thanks! |
Fix stub file classes not being correctly referenced as parent classes in non-stub files, preventing checks ensuring property and function types match.