Bad results when scanning code using traits

[phpstan]

Bad results when scanning code using traits

[ondrejmirtes]

And the issue is resolved! There was a problem when the same method was declared in both class and used trait. 6e3a590

So the dev-master now is what the traits support will look like in next stable version.

[ondrejmirtes]

The feedback PHPStan gives about your code is that it's easily breakable by using the trait by a wrong class. Take this code for example: https://3v4l.org/XQ0Y4

It will crash in runtime because you used the trait by a class without the doBar() method.

But consider this example: https://3v4l.org/F6W8K

This code won't even compile because you enforce in the trait that the target class must implement the doBar() method. The latter example is also something that PHPStan will be satisfied with.

So consider your codebase to adopt this approach to make it bullet-proof against wrong usage.

But your idea about a different approach to analysing traits is interesting and something to think about :) Thanks!

[JanTvrdik]

@ondrejmirtes I still think that using abstract methods in traits is a hack. I like to solution proposed by @ahungry a lot more.

Another possible approach is to use @method + @property annotations on trait and verify that the class using trait actually does have all the required methods and properties.

[ondrejmirtes]

I also like it but I proposed a solution that would work for him for now 😊 I'll try to look into the advanced analysis of traits soon.

…

On Tue, 6 Dec 2016 at 00:42, Jan Tvrdík ***@***.***> wrote: @ondrejmirtes <https://github.com/ondrejmirtes> I still <#9 (comment)> think that using abstract methods in traits is a hack. I like to solution proposed by @ahungry <https://github.com/ahungry> a lot more. Another possible approach is to use @method + @Property annotations on trait and verify that the class using trait actually does have all the required methods and properties. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuFNFWbtPyKm50Jl-YYKcfUYy9y70ks5rFKFwgaJpZM4LEWww> .

[ahungry]

Thanks for the responses - I hadn't thought of using abstract method declarations in the trait (good idea for most traits). Unfortunately, this actual use case involves a class constant, which, while properly guarded in the trait itself with defined checks, doesn't propagate down to phpstan.

General sample code to illustrate how this may look (granted no one would really use this, but I digress...):

trait FooTrait
{
    public function getFilePath(string $fileName): string
    {
        if (!defined('self::ASSET_PATH')) {
	    throw new Exception(
                __CLASS__
                . '::ASSET_PATH must be defined to use '
		. __METHOD__
            );
	}

	return realpath(self::ASSET_PATH . '/' . $fileName);
    }
}

class Bar
{
    use FooTrait;

    const ASSET_PATH = '/tmp';
}

// Obviously ok                                                                                                                                                          
echo Bar::getFilePath('file.txt');

class Baz
{
    use FooTrait;
}

// Throws the Exception as expected with message: PHP Fatal error: Uncaught                                                                                              
// Exception: Baz::ASSET_PATH must be defined to use FooTrait::getFilePath                                                                                               
echo Baz::getFilePath('file2.txt');

[ahungry]

I've prepared a pull request that updates the parsing process to handle traits under all their possible contexts.

I think this is better than annotations or abstracting in the trait.

Let me know what you think:

#23

[ahungry]

Sorry for the poor formatting, I didn't realize there are tabs for indentation - I may have to make another PR to run emacs M-x untabify across your codebase (half joking 😄 )

Edit: No idea what's going on with your Travis CI either - unit tests pass fine for me locally, but that process on Travis is getting a lot of errors when testing this PR.

[ondrejmirtes]

@JanTvrdik @ahungry Can you try out current master? I implemented the idea here: e59d160 (but use current dev-master in composer because later commits also affect this behaviour).

If you're OK with the results I will release 0.5.

[ondrejmirtes]

It works fine according to your reactions here #23 (comment) but I still need to figure out one issue, hope to do that soon. But since you are satisifed with the current results, I will close this now. Thanks!