Skip code changes on unresolvable/unknown classes

[staabm]

adding test cases which show the bug described in rectorphp/rector#8093

[staabm]

if we don't know the parent class (because e.g. its not autoloadable), we should be pessimistic and do no changes

[samsonasik]

throw Exception to be validated in the rules can be the way to go:

try {
   parentGuard->...
} catch (ClassParentNotAtoloadedException) {

}

[staabm]

thanks for the suggestion. I have a different idea.. let me show it to you :)

[staabm]

I expect some delay until Roave/BetterReflection#1359 will be implemented in PHPStan. therefore I implemented the suggested workaround for now

[staabm]

@samsonasik should be good to go

[staabm]

this method now returns true/false when we can make a final decision or null when classes involved are not known and we can't judge

[samsonasik]

just return true here, so it "marked" as has parent so bool can be returned.

[staabm]

I don't think its a good idea to handle "we don't know" the same as "sure, the method exists".

for our callers, as they exist right now, this is acceptable, but it might suprisingly break in the future as soon as someone compares the result of this method against true .

callers might handle the situation differently, depending on the method exists or not.

[samsonasik]

I prefer to create separate service for param guard handling, eg: ClassMethodParamTypeOverrideGuard.

Something like ClassMethodReturnTypeOverrideGuard::shouldSkipClassMethod() do

rector-src/packages/VendorLocker/NodeVendorLocker/ClassMethodReturnTypeOverrideGuard.php

Line 51 in 73112a2

public function shouldSkipClassMethod(ClassMethod $classMethod, Scope $scope): bool

[samsonasik]

@staabm just checked the getParentClassMethod() usages in rules, there are various usage directly, which cause exception, I think try catch should be in consumer instead of in hasParentClassMethod() as that cause double call.

I will create new PR for that.

[samsonasik]

hm.., the service is named ParentClassMethodTypeOverrideGuard, which has vs get should only validate instead of getting in real use case, so I prefer return bool true for hasParentClassMethod(), and return null for getParentClassMethod() when no parent at all.

[staabm]

handling UnresolvableClassException at the caller site could indeed improve the current situation 👍

[samsonasik]

I create PR

• [TypeDeclaration] Use return bool for ParentClassMethodTypeOverrideGuard::hasParentClassMethod(), return ?MethodReflection for ParentClassMethodTypeOverrideGuard::getParentClassMethod() #4703 for it,

return bool only should be the way to go as it only validate, not a "real" verification, we then return ?MethodReflection for getParentClassMethod()

[samsonasik]

@TomasVotruba let's merge it so we can improve in separate PR, thank you @staabm