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

refactor(phpstan): Remove deprecations related to execute\(\) method #12684

Merged
merged 2 commits into from Sep 6, 2023

Conversation

scyzoryck
Copy link
Contributor

@scyzoryck scyzoryck commented Aug 27, 2023

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

Description:

Hi! My first PR here.
Fixes for phpstan errors:

Call to deprecated method execute() of class Doctrine\DBAL\Query\QueryBuilder:  
Use {@see executeQuery()} or {@see executeStatement()} instead.                 

Steps to test this PR:

  1. Run phpstan
  2. Run automated tests.

@scyzoryck scyzoryck changed the title refactor(phpstan): Remove deprecations related to execute\(\) method Draft: refactor(phpstan): Remove deprecations related to execute\(\) method Aug 27, 2023
@scyzoryck scyzoryck changed the title Draft: refactor(phpstan): Remove deprecations related to execute\(\) method refactor(phpstan): Remove deprecations related to execute\(\) method Aug 27, 2023
@scyzoryck
Copy link
Contributor Author

It looks like tests are now passing failing tests are now passing on my local. However I was unable to run all of them :(

@RCheesley
Copy link
Sponsor Member

Thanks for the PR @scyzoryck and welcome to the community!

I've set the automated test suite running, and will ping some of the developers to review your proposed changes!

@scyzoryck
Copy link
Contributor Author

Thanks! I will rebase the branch & fix the last issues from tests later today :)

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #12684 (ded6ca2) into 5.x (9906356) will decrease coverage by 0.01%.
The diff coverage is 48.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #12684      +/-   ##
============================================
- Coverage     58.49%   58.49%   -0.01%     
  Complexity    33555    33555              
============================================
  Files          2177     2177              
  Lines        101591   101588       -3     
============================================
- Hits          59423    59420       -3     
  Misses        42168    42168              
Files Changed Coverage Δ
...bundles/ChannelBundle/Entity/MessageRepository.php 19.56% <0.00%> (ø)
...undles/EmailBundle/Entity/StatDeviceRepository.php 0.00% <0.00%> (ø)
.../IntegrationsBundle/Sync/Helper/SyncDateHelper.php 44.18% <0.00%> (ø)
...es/LeadBundle/Entity/StagesChangeLogRepository.php 30.76% <0.00%> (ø)
app/bundles/LeadBundle/Entity/TagRepository.php 34.69% <0.00%> (ø)
...dles/LeadBundle/EventListener/ReportSubscriber.php 74.24% <0.00%> (ø)
app/bundles/LeadBundle/Form/Type/FilterTrait.php 25.53% <0.00%> (ø)
app/bundles/LeadBundle/Model/ListModel.php 63.36% <0.00%> (ø)
...tificationBundle/Entity/NotificationRepository.php 10.67% <0.00%> (ø)
...ndles/NotificationBundle/Entity/StatRepository.php 0.00% <0.00%> (ø)
... and 69 more

mabumusa1
mabumusa1 previously approved these changes Aug 31, 2023
Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

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

I love this PR, it is good to go.

It was on my pipeline but never had the time to do it.

Thanks @scyzoryck this is a very strong start, we hope we can get more contribution from you

@mabumusa1
Copy link
Member

@scyzoryck Please fix CS fixer then we are good to go, I will ask others to review this PR

@mabumusa1 mabumusa1 added pending-test-confirmation PR's that require one test before they can be merged refactoring The change does not change behavior but improves the code code-review-passed PRs which have passed code review labels Aug 31, 2023
@scyzoryck
Copy link
Contributor Author

Thanks for feedback. Rebased & code style fixed. I need to finish setting up environment with docker to have all tests working on my local. :)
I know it is a lot of changes in different files, but I had no idea how to approach it in the most easy way for code review. If you have any suggestion for the future contributions, let me know!

Best, scyzoryck.

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.

I went through the code changes and confirmed that the executeQuery method is used for select queries and executeStatement method for the insert, update and delete queries.

Since this PR touches every part of Mautic we must trust our test suite to scream if there is something wrong. And it's not screaming, so approving 👍

@escopecz escopecz merged commit f856245 into mautic:5.x Sep 6, 2023
13 of 14 checks passed
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label Sep 6, 2023
@escopecz escopecz added this to the 5.0-beta milestone Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review refactoring The change does not change behavior but improves the code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants