-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
Test Carbon mixin #1857
base: 2.x
Are you sure you want to change the base?
Test Carbon mixin #1857
Conversation
Are you also using PHPStan strict rules? |
No. I am using https://github.com/phpstan/extension-installer, so I double checked that the strict rules package is not present in |
If you're using the extension-installer then the Carbon extension should be automatically installed. Try testing the Carbon mixin without Larastan, and if the issue still persists then it's something on their end. |
@@ -1,6 +1,8 @@ | |||
includes: | |||
- ../extension.neon | |||
- ../vendor/nesbot/carbon/extension.neon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests I added in Larastan pass with or without the setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, we shouldn't be testing their extension in our tests---personally I'm of the mindset that Larastan should remove support for the Carbon mixins as Carbon provides their own extension for their own mixins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment as to why I think it is useful for testing: d606311
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is necessary/required---our job is to test our own extension, not what other users might install in their application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that Larastan does not exist in isolation, thus a more integration-like test setup is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see that argument when running the e2e tests, but not when testing the code specific to this package
Could you fix another typo? ./tests/Type/data/model-properties.php:80 "overriden" |
Thanks for the feedback. I tried adding the following to my "extra": {
"phpstan/extension-installer": {
"ignore": [
"nesbot/carbon"
]
}
} The resulting errors are exactly the same as before. As I have outlined in the second update to the description, the update from larastan v2.8.1 to v2.9.0 is indeed the single change that causes the issue to appear. Not saying larastan is wrong or buggy, but that triggers the error. |
tests/Type/GeneralTypeTest.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily removed to allow CI to pass and run on all versions.
Here are the included extensions
|
Which version of Carbon are you on? Also, you should try analyzing the Carbon mixin without Larastan using only the Carbon extension |
# Conflicts: # tests/Type/GeneralTypeTest.php
39d60e2
to
c4d0ee6
Compare
Changes
I am trying to reproduce an issue in a project of mine that uses Carbon mixins.
After updating from larastan v2.8.1 to v2.9.0errors such as the following started appearing:Update: the issue also exists with v2.8.1, sorry. I am digging further to find what caused the issue.Second update: I tried updating the dependencies one by one. It was indeed the update from Larastan that was responsible.
The code under question:
The relevant part of the mixin:
It is registered in a service provider like this:
I can confirm that the mixin is recognized, as changing the method name to something nonsensical triggers the following error:
Unfortunately, I am unable to reproduce the issue in a test case here. Can you help?
Breaking changes
Not yet.