-
Notifications
You must be signed in to change notification settings - Fork 123
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
Enable basic PHPSTAN checks #239
Conversation
@dennisameling do you need me to test this or is it just a case of getting the green tick to merge? 🤓 ✅ |
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.
Found 1 line that doesn't/didn't make sense and suggested change. Let me know if it make sense. Otherwise this is good to go 👍
@@ -471,7 +471,7 @@ public function testCampaignContactEditEvent() | |||
$date = new \DateTime($log['triggerDate'], new \DateTimeZone('UTC')); | |||
$this->assertEquals($log['triggerDate'], $date->format('c')); | |||
} else { | |||
$this->assertFalse(false, 'Event ID not recognized in the log.', var_export($event, true)); | |||
$this->assertFalse(var_export($event, true), 'Event ID not recognized in the log.'); |
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.
This doesn't make much sense. The previous version did not make sense either. I expect the intent was to fail the test if this assert will run. How about this:
$this->assertFalse(var_export($event, true), 'Event ID not recognized in the log.'); | |
$this->fail('Event ID not recognized in the log.'.var_export($event, true)); |
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.
@dennisameling WDYT?
@escopecz some conflicts with this PR - suggest that we have someone from @mautic/core-team pick this up if we want to merge it? |
Yep, it'd be great to get this sorted |
Closing in favour of #325 - thanks for the efforts folks! |
Sets up basic PHPSTAN checks. Setting the level higher than 1 causes quite some errors/warnings, so I'm leaving it at 1 for now.