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

Macro implements MethodReflection instead of BuiltinMethodReflection #298

Merged
merged 2 commits into from
Aug 17, 2019

Conversation

ondrejmirtes
Copy link
Contributor

This is fix for PHPStan 0.11.14. This PR changes what interface Macro implements, and some additonal cleanups stem from that.

This is closer to how PHPStan extensions should work. It's better to implement MethodReflection in a class directly rather than pass it through PhpMethodReflectionFactory.

I'm sorry about these BC breaks, it didn't occur to me that some extensions would be using the BuiltinMethodReflection interface. I want to have much clearer backwards compatibility boundary in the future, probably by tagging some classes and interfaces with @api. Everything else will be considered internal.

Right now, the rule of thumb is that backwards compatibility covers what's mentioned in the README.

@szepeviktor
Copy link
Collaborator

szepeviktor commented Aug 17, 2019

@nunomaduro What do you think? I think it's working.

@ondrejmirtes
Copy link
Contributor Author

I'm also fixing #291 here.

@nunomaduro
Copy link
Collaborator

Blindly trusting you on this. 😅

@nunomaduro
Copy link
Collaborator

@ondrejmirtes Shouldn't we bump the required phpstan version?

@patrickbrouwers
Copy link

patrickbrouwers commented Aug 17, 2019

@nunomaduro I'm getting the following exceptions after updating:

 Whoops\Exception\ErrorException  : Uncaught InvalidArgumentException: Expected string, array of parts or Name instance in vendor/nikic/php-parser/lib/PhpParser/Node/Name.php:234
Stack trace:
#0 vendor/nikic/php-parser/lib/PhpParser/Node/Name.php(26): PhpParser\Node\Name::prepareName(Object(Closure))
#1 vendor/nunomaduro/larastan/src/Methods/Pipes/BuilderLocalMacros.php(61): PhpParser\Node\Name->__construct(Object(Closure))
#2 vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(163): NunoMaduro\Larastan\Methods\Pipes\BuilderLocalMacros->handle(Object(NunoMaduro\Larastan\Methods\Passable), Object(Closure))
#3 vendor/nunomaduro/larastan/src/Methods/Pipes/ModelScopes.php(67): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(NunoMaduro\Larastan\Methods\Passable))

  at vendor/nikic/php-parser/lib/PhpParser/Node/Name.php:234
    230|         } elseif ($name instanceof self) {
    231|             return $name->parts;
    232|         }
    233|
  > 234|         throw new \InvalidArgumentException(
    235|             'Expected string, array of parts or Name instance'
    236|         );
    237|     }
    238|

Seems to be caused by stuff like:

Route::bind('myModel', static function (string $uuid): MyModel {
    return MyModel::onlyTrashed()->where('uuid', $uuid)->firstOrFail();

    // no difference if used:
    return MyModel::query()->onlyTrashed()->where('uuid', $uuid)->firstOrFail();
});

@ondrejmirtes
Copy link
Contributor Author

Not necessary to bump PHPStan version.

I'm gonna look into the exception, maybe I screwed up.

@ondrejmirtes
Copy link
Contributor Author

I haven't realized that ReflectionFunction constructor also accepted closuures, not just strings.

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

Successfully merging this pull request may close these issues.

4 participants