Skip to content

Tighten types that were reported by phpstan. #13110

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 9, 2024
Merged

Tighten types that were reported by phpstan. #13110

merged 10 commits into from
Dec 9, 2024

Conversation

biozshock
Copy link
Contributor

@biozshock biozshock commented Dec 24, 2023 •
edited by escopecz
Loading

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [ x ]
Deprecations? [ x ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ x ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

Tightened types and fixed phpstan errors.

There are still some PHPStan errors revealed by the type tightening or method overrides. Best solution as for me is to generate new baseline.

Steps to test this PR:

As there are lots of places changed, please test as much as you can.

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Check the installation works.

@biozshock biozshock requested a review from mabumusa1 as a code owner December 24, 2023 22:45
@codecov Codecov
Copy link

codecov bot commented Dec 24, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 63.28125% with 47 lines in your changes missing coverage. Please review.

Project coverage is 63.45%. Comparing base (426e96a) to head (d7884da).
Report is 1 commits behind head on 6.x.

Files with missing lines Patch % Lines
.../CampaignBundle/Command/TriggerCampaignCommand.php 66.66% 5 Missing ⚠️
...paignBundle/Command/UpdateLeadCampaignsCommand.php 58.33% 5 Missing ⚠️
...nnelBundle/Command/SendChannelBroadcastCommand.php 58.33% 5 Missing ⚠️
...alBundle/Command/MauticSocialMonitoringCommand.php 0.00% 4 Missing ⚠️
app/bundles/CoreBundle/Factory/MauticFactory.php 78.57% 3 Missing ⚠️
...es/UserBundle/Controller/Api/UserApiController.php 0.00% 3 Missing ⚠️
...es/LeadBundle/Controller/Api/LeadApiController.php 0.00% 2 Missing ⚠️
app/bundles/PointBundle/Model/TriggerModel.php 0.00% 2 Missing ⚠️
...undle/Scheduler/Command/ExportSchedulerCommand.php 50.00% 2 Missing ⚠️
...s/CampaignBundle/Controller/CampaignController.php 0.00% 1 Missing ⚠️
... and 15 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                6.x   #13110      +/-   ##
============================================
+ Coverage     63.35%   63.45%   +0.09%     
- Complexity    34735    34772      +37     
============================================
  Files          2282     2282              
  Lines        103826   103990     +164     
============================================
+ Hits          65784    65991     +207     
+ Misses        38042    37999      -43
Files with missing lines Coverage Δ
...es/CampaignBundle/Command/ValidateEventCommand.php 88.00% <100.00%> (+2.28%) ⬆️
...DashboardBundle/Controller/DashboardController.php 32.05% <100.00%> (-0.42%) ⬇️
app/bundles/EmailBundle/Entity/EmailRepository.php 85.01% <ø> (ø)
app/bundles/EmailBundle/Helper/MailHelper.php 78.24% <ø> (ø)
...e/MonitoredEmail/Processor/Bounce/BouncedEmail.php 100.00% <100.00%> (ø)
...bundles/FormBundle/Controller/PublicController.php 44.65% <100.00%> (-0.21%) ⬇️
...p/bundles/InstallBundle/Command/InstallCommand.php 67.90% <ø> (ø)
...egrationsBundle/Integration/ConfigurationTrait.php 100.00% <100.00%> (ø)
...dles/LeadBundle/Command/UpdateLeadListsCommand.php 98.38% <100.00%> (+0.05%) ⬆️
...p/bundles/LeadBundle/Controller/LeadController.php 60.09% <100.00%> (-0.22%) ⬇️
... and 41 more

... and 57 files with indirect coverage changes

@biozshock biozshock mentioned this pull request Jan 4, 2024
Closed
kuzmany
kuzmany requested changes Jan 5, 2024
View reviewed changes
@kuzmany kuzmany added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 5, 2024
escopecz
escopecz reviewed Jan 5, 2024
View reviewed changes
escopecz
escopecz reviewed Jan 5, 2024
View reviewed changes
Copy link
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.

This PR is improving the code, but the plan was to release M5.0.0 stable next week. If we'd merge this then we'd need to release new release candidate version as it contains BC breaks. What would you suggest to do here?

@kuzmany
Copy link
Member

kuzmany commented Jan 5, 2024

@escopecz As I see, it's baked on the 5.1 branch, so I assume we can go to 5.1.

@escopecz
Copy link
Member

escopecz commented Jan 8, 2024

Breaking changes can go to 6.x from now on. If someone wants to maintain that branch for a year or so, we can create it now.

@escopecz escopecz self-requested a review January 8, 2024 17:46
@escopecz escopecz changed the base branch from 5.1 to 5.x January 9, 2024 16:30
@escopecz
Copy link
Member

escopecz commented Jan 9, 2024

I changed the target branch from 5.1 to 5.x as we are getting back to our classic branching strategy where 5.x will be for features and 5.0 for bug fixes.

There is a conflict. Please resolve it.

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Jan 9, 2024
@biozshock biozshock force-pushed the phpstan-fix-types branch 4 times, most recently from e78afba to c93d3cd Compare January 10, 2024 08:54
@escopecz escopecz added enhancement Any improvement to an existing feature or functionality refactoring The change does not change behavior but improves the code and removed pending-feedback PR's and issues that are awaiting feedback from the author has-conflicts Pull requests that cannot be merged until conflicts have been resolved labels Jan 16, 2024
@escopecz
Copy link
Member

@biozshock can you either regenerate the baseline or add comment ignores to the code with the explanations why is the line ignored or add the ignores to phpstan configs so we could merge this? I'm fine with either of those.

@biozshock
Copy link
Contributor Author

@escopecz What is better or common in Mautic? I can do both of the solutions to get phpstan errors out of the way.

@escopecz escopecz requested a review from kuzmany January 24, 2024 13:29
escopecz
escopecz reviewed Jan 24, 2024
View reviewed changes
@biozshock biozshock force-pushed the phpstan-fix-types branch 3 times, most recently from 53ea0bd to ecc5237 Compare January 26, 2024 00:08
@biozshock biozshock force-pushed the phpstan-fix-types branch 4 times, most recently from 0eac6ac to 7584ccc Compare August 2, 2024 07:26
escopecz
escopecz reviewed Aug 2, 2024
View reviewed changes
escopecz
escopecz previously approved these changes Aug 9, 2024
View reviewed changes
Copy link
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.

Thank you! This is really great work. It reduces the tech debt in the baseline file by ~10%. 🎉

@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Aug 9, 2024
@escopecz escopecz added this to the 5.2 milestone Aug 9, 2024
@escopecz escopecz requested a review from fedys September 12, 2024 14:25
@escopecz
Copy link
Member

escopecz commented Dec 1, 2024

@biozshock could you please rebase on 6.x branch and let's get merge it there.

@escopecz escopecz removed this from the 5.2 milestone Dec 5, 2024
@biozshock biozshock changed the base branch from 5.x to 6.x December 8, 2024 21:54
@biozshock
Copy link
Contributor Author

@escopecz done. You probably need to merge the PR, as this is heading to a year long one :)

@escopecz escopecz added this to the 6.0 milestone Dec 9, 2024
escopecz
escopecz approved these changes Dec 9, 2024
View reviewed changes
Copy link
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.

Thank you!

@escopecz escopecz merged commit d5e4c5d into mautic:6.x Dec 9, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

@mabumusa1 mabumusa1 Awaiting requested review from mabumusa1

@kuzmany kuzmany Awaiting requested review from kuzmany

@fedys fedys Awaiting requested review from fedys

+1 more reviewer

@mallezie mallezie mallezie requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
enhancement Any improvement to an existing feature or functionality refactoring The change does not change behavior but improves the code
Projects
None yet
Milestone
6.0.0-alpha
Development

Successfully merging this pull request may close these issues.
4 participants
@biozshock @kuzmany @escopecz @mallezie