-
-
Notifications
You must be signed in to change notification settings - Fork 394
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
Less ambitious fix to BuiltinMethodReflection woes #299
Conversation
…lection" This reverts commit dbe3bf7.
Thanks for looking into a fix! |
0bfc6b8
to
12405c1
Compare
@ondrejmirtes Can you confirm that your changeset is this? v0.3.19...HEAD. Comparing with the previous code we had. |
Yes, looks right to me. The PHPStan version bump probably isn’t necessary.
Please run it on a real Laravel project before tagging the next version,
just to be sure 😊 Thank you!
On Sun, 18 Aug 2019 at 23:41, Nuno Maduro ***@***.***> wrote:
@ondrejmirtes <https://github.com/ondrejmirtes> Can you confirm that your
changeset is this? v0.3.19...HEAD
<v0.3.19...HEAD>.
Comparing with the previous code we had.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#299>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAZTOAZGU7CCEM2PTB6G4DQFG6ZJANCNFSM4IMQP4JQ>
.
--
Ondřej Mirtes
|
Bumping to the latest version of phpstan anyway. Thanks for this. |
CI runs Larastan on the default Laravel app https://github.com/nunomaduro/larastan/blob/master/tests/laravel-test.sh#L6 |
@szepeviktor Thanks for that buddy! |
Yeah but that wasn’t enough when I broke it yesterday with the macro
closures or what 😊
On Sun, 18 Aug 2019 at 23:54, Nuno Maduro ***@***.***> wrote:
@szepeviktor <https://github.com/szepeviktor> Thanks for that buddy!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#299>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAZTOHAEIGJWCX3Z5GNR2LQFHAK3ANCNFSM4IMQP4JQ>
.
--
Ondřej Mirtes
|
@nunomaduro Should we add 2 tests for the recent two failed releases? What do you think? |
I think the Laravel project should be commited here so we can easily add
more stuff to be tested in it.
On Mon, 19 Aug 2019 at 04:28, Viktor Szépe ***@***.***> wrote:
@nunomaduro <https://github.com/nunomaduro> Should we add 2 tests for the
recent two failed releases? What do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#299>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAZTOCPYLLDLOCLZEXMHT3QFIAMPANCNFSM4IMQP4JQ>
.
--
Ondřej Mirtes
|
I probably got too ambitious with the last fix.
The problem is that I didn' realize that
localMacros
don't contain strings but anonymous functions. So I reverted this change and did the minimal work required to fix the actual issue.PHPStan currently does not have a better way of converting anonymous function (as in the actual closure in memory, not just a reflection) to
ParametersAcceptor
.This should probably have some kind of test but since I'm not familiar with Laravel, I don't know how the code that's touched by these changes looks like in a Laravel app.
I hope that following versions of PHPStan will be less turbulent for you 👍