Handle Carbon macros

[kylekatarnls]

Hi,

I would like to propose a compatibility support for Carbon macros, it uses the same public API as Laravel macros but is different internally.

I tested the code above in Laravel apps and it works fine.

But I couldn't get a correct unit tests I mimicked other ones but if I revert my change in source, the CarbonExtension test still pass while I should get "undefined method" PHPStan errors.

If you're OK to include this feature, could you help me to write this unit test?

[szepeviktor]

Very well done.

Could you make the test pass?

[kylekatarnls]

@szepeviktor See my comment. On my local machine the tests are all green rather I tests with or without my change, and in Travis-CI it's like ignored. But in real apps, it's working. I can't guess more how the feature test is supposed to work.

[kylekatarnls]

I guess tests are not compatible with Windows as running php artisan code:analyse inside the directory "laravel" created by laravel-test.sh output the content of ./vendor/bin/phpstan instead of running it. However I can run php artisan code:analyse with no troubles in real apps.

[kylekatarnls]

I found what was not compatible with Windows:

• starts_with($path, DIRECTORY_SEPARATOR) is Unix root only, this means --paths currently does not work with absolute paths on Windows, I fixed it.
• tests compare paths assuming directory separator is "/"
• tests analyze() method calls the shell/bat in vendor/bin, it should call the PHP bin in vendor/phpstan/phpstan/bin which is universal

Now I get the tests working on my local machine, I will check the change has not the same effect in the test app than in my local app.

[kylekatarnls]

Now, it needs a way to declare PhpMethodReflection as "static", when a macro is explicitly static like in the test example:

Carbon::macro('foo', static function (): string {
    return 'foo';
});

Such macros should be callable both statically or dynamically. And not only in Carbon but also in the Laravel Macroable stuff.

[kylekatarnls]

I see $this->isStatic = false; inside the Macro which is incorrect. They can be both. And we can consider a best practice to use static function to declare them as static, but even without the static statement, macros are callable statically, so they should more likely be considered static by default.

[ondrejmirtes]

It should be marked as static and ignore “dynamic call to static method” in phpstan.neon.

On Wed, 21 Aug 2019 at 11:13, Kyle ***@***.***> wrote: I see $this->isStatic = false; inside the Macro which is incorrect. They can be both. And we can consider a best practice to use static function to declare them as static, but even without the static statement, macros are callable statically, so they should more likely be considered static by default. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#301>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZTOE7XWSZKV3Y5BOWDPTQFUBLPANCNFSM4IN5FDKA> .

-- Ondřej Mirtes

[kylekatarnls]

It should be marked as static and ignore “dynamic call to static method” in
phpstan.neon

It would also apply to non-macros methods so I'm not a fan of this fix. I would rather say macros are static. At least they should be when the explicit statement is added.

I think it already is, as calling dynamically the static method do not raise errors.

I proposed here a softer change that will only mark static macros if they are created with static closures. If behavior is changed for all macros it can be an other PR/discussion.

[kylekatarnls]

@szepeviktor seems like there is a broken hook:
continuous-integration/styleci/push Expected — Waiting for status to be reported

But as I just updated the branch with master it means it's the same exact code than for the continuous-integration/styleci/pr check.

[kylekatarnls]

This is a fix for 443f0fe
In order to have successful unit tests.