Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

dennisameling
Copy link
Member

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.

@dennisameling dennisameling requested a review from a team February 24, 2021 19:40
@cla-bot cla-bot bot added the cla-signed label Feb 24, 2021
@dennisameling dennisameling added the chore Tasks that relate to maintaining this Github repository label Feb 24, 2021
@RCheesley
Copy link
Sponsor Member

@dennisameling do you need me to test this or is it just a case of getting the green tick to merge? 🤓 ✅

Copy link
Sponsor Member

@escopecz escopecz left a 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.');
Copy link
Sponsor Member

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:

Suggested change
$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));

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RCheesley
Copy link
Sponsor Member

@escopecz some conflicts with this PR - suggest that we have someone from @mautic/core-team pick this up if we want to merge it?

@escopecz
Copy link
Sponsor Member

escopecz commented May 3, 2023

Yep, it'd be great to get this sorted

@RCheesley
Copy link
Sponsor Member

Closing in favour of #325 - thanks for the efforts folks!

@RCheesley RCheesley closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Tasks that relate to maintaining this Github repository cla-signed
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants