Skip to content
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

ExtendedMethodReflection #1403

Merged
merged 1 commit into from Jun 7, 2022
Merged

ExtendedMethodReflection #1403

merged 1 commit into from Jun 7, 2022

Conversation

ondrejmirtes
Copy link
Member

FYI @rvanvelzen @jrmajor This is my answer to have we can extend what method can tell us about themselves without breaking backward compatibility.

Right now MethodReflection is being used in custom MethodsClassReflectionExtension so we can't add new methods on it. But that doesn't prevent us to work with ExtendedMethodReflection in PHPStan internals.

I imagine that methods like acceptsNamedArguments() and getAsserts() could be added on ExtendedMethodReflection.

@ondrejmirtes
Copy link
Member Author

FYI @canvural I'm getting this error in Larastan:

 ------ --------------------------------------------------------------------- 
  Line   Rules/ModelProperties/ModelPropertyStaticCallRule.php                
 ------ --------------------------------------------------------------------- 
  149    Instanceof between PHPStan\Reflection\ExtendedMethodReflection and   
         NunoMaduro\Larastan\Reflection\EloquentBuilderMethodReflection will  
         always evaluate to false.                                            
 ------ --------------------------------------------------------------------- 

But I don't think I'm doing a BC break here and I don't think you need to do this:

        if ($methodReflection instanceof EloquentBuilderMethodReflection) {
            $methodReflection = $methodReflection->getOriginalMethodReflection();
        }

I think you can continue passing yours EloquentBuilderMethodReflection into modelPropertiesRuleHelper and it should work...

@ondrejmirtes ondrejmirtes merged commit 778308d into 1.7.x Jun 7, 2022
@ondrejmirtes ondrejmirtes deleted the extended-method branch June 7, 2022 11:56
@canvural
Copy link
Contributor

canvural commented Jun 7, 2022

@ondrejmirtes Thanks, I'll take a look.

@canvural
Copy link
Contributor

canvural commented Jun 9, 2022

This was a little bit of BC break I guess. Because previously extensions could return any class that implements MethodReflection But now it'll always be WrappedExtendedMethodReflection But then one could argue you should always work with interfaces not concrete classes 😅 Yeah, correct.

Temporarily I solved this by making EloquentBuilderMethodReflection implement ExtendedMethodReflection instead of MethodReflection (I know bad solution, but it was fastest way for now) Later I'll investigate why EloquentBuilderMethodReflection and getOriginalMethodReflection was needed. It was to carry some information between extensions, but I forgot the root cause.

@ondrejmirtes
Copy link
Member Author

Larastan does this:

        if ($methodReflection instanceof EloquentBuilderMethodReflection) {
            $methodReflection = $methodReflection->getOriginalMethodReflection();
        }

So $methodReflection->getOriginalMethodReflection() is passed further to modelPropertiesRuleHelper because EloquentBuilderMethodReflection itself doesn't work there? Why it doesn't work there?

@canvural
Copy link
Contributor

canvural commented Jun 9, 2022

Honestly no idea! I'll try to dig deeper when I have time.

@ondrejmirtes
Copy link
Member Author

So that's the root cause here - EloquentBuilderMethodReflection should answer everything the same way the wrapped getOriginalMethodReflection() does, and it should work fine :)

@canvural
Copy link
Contributor

canvural commented Jun 9, 2022

If that was the case I'd use the original method reflection directly I guess. There are some differences. For example return type can be different between the original method reflection and EloquentBuilderMethodReflection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants