fix: skip non-native methods

[calebdw]

Hello!

This prevents ALL non-native methods (not just those from annotations) from being converted to first class callables:

This change resulted in the following phpstan error:

[TomasVotruba]

Thanks 👍

Can you also add test fixture that verifies this change? Just so we don't accidentally change it in the future.

[calebdw]

It's already testing by the existing annotation fixture---I didn't want to write a whole PHPStan extension just to test something other than a AnnotationMethodReflection (e.g., Laravel's macro system)

[TomasVotruba]

No need for extension. What is "non-native method" apart the @method?

[calebdw]

This is a Macro:

MenuItem::macro('reportable', static function (string $class): MenuItem {
    if (! class_exists($class) || ! is_a($class, Reportable::class, true)) {
        throw new RuntimeException("Invalid reportable class [{$class}].");
    }

    $instance = resolve($class);

    return MenuItem::make($instance->name())
        ->canSeeWith($instance::permissions())
        ->path($instance->route());
});

See: https://github.com/larastan/larastan/blob/3.x/src/Methods/MacroMethodsClassReflectionExtension.php

[TomasVotruba]

So in short, the reportable() does not exist?

[calebdw]

Yes, it's not an actual method, it's a closure that is called through __call: https://github.com/laravel/framework/blob/12.x/src/Illuminate/Macroable/Traits/Macroable.php

Note, this does not actually result in a php error, so I'm not sure why phpstan added a check for it. I'm thinking about just globally ignoring callable.nonNativeMethod, but I figured that rector should not make changes that result in phpstan errors

[TomasVotruba]

I see. How would you implement it if no Rector nor PHPStan would exists?

[calebdw]

I'm not sure what you're asking? Implement what?

[TomasVotruba]

The change this rule is doing. How would you upgrade code yourself manually, without any tooling.

[calebdw]

I would be fine with this upgrade in favor of the first class callable, however, other people might not like it since it throws phpstan errors.

This PR just ensures that an actual method exists on the declaring class and that the method in not a macro or annotation

[TomasVotruba]

Allright, lets go this way then.

We still need a test fixture to backup this change.

[TomasVotruba]

Just checking, as there is no feedback in past 2 weeks. Any plan to finish this?
I want to avoid staling failing PRs here.

[calebdw]

Yes, I plan to, but it's Christmas and I'm sick---I haven't had time yet to write a custom phpstan extension just to test this.

If you'd like to merge now then feel free, there's an existing passing test that covers this.

[calebdw]

To be transparent, I've just globally ignored these phpstan errors in my config as I don't care to enforce them and it doesn't cause any sort of runtime error

Not sure these phpstan rules were added in the first place...

[TomasVotruba]

I see, let's give this a go then 👍

[calebdw]

Sounds good, thank you sir and have a Merry Christmas!!

[TomasVotruba]

Thank you 👍 Merry Christmas to you and your family 🎄