Skip to content

Resolving PHP 8.0 warnings #14664

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 2 commits into from
Mar 4, 2025
Merged

Resolving PHP 8.0 warnings #14664

merged 2 commits into from
Mar 4, 2025

Conversation

escopecz
Copy link
Member

@escopecz escopecz commented Mar 3, 2025 •
edited
Loading

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

Description

This PR resolves several PHP warnings that we could see in our logs. Such as:

PHP Warning: Invalid argument supplied for foreach() in app/bundles/LeadBundle/Entity/LeadListRepository.php on line 839

PHP Warning: array_key_exists() expects parameter 2 to be array, bool given in app/bundles/LeadBundle/Helper/ContactRequestHelper.php on line 12

The changes here are checking if the variable is type of the PHP function expect to avoid these warnings.

📋 Steps to test this PR:

We don't have a steps to test per se as this PR is resolving warnings from logs. But based on the code changes we can test:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a form and submit it. Check it works as before.
  3. Execute bin/console mautic:segments:stat
  4. Create a landing page and check the tracking works as before.

All these actions are tested by existing functional tests.

@ts-navghane @escopecz
Merge pull request mautic#2106 from acquia/MAUT-10872_MAUT-10873_MAUT…
de978f4 
…-10875_MAUT-10883_MAUT-10887_MAUT-10922_MAUT-8624

Fix warnings for PHP8.0 upgrade
@escopecz escopecz added unforking Used for PRs in the Acquia's unforking initiative bug Issues or PR's relating to bugs refactoring The change does not change behavior but improves the code labels Mar 3, 2025
@escopecz escopecz added this to the 6.0.0-RC milestone Mar 3, 2025
@escopecz escopecz mentioned this pull request Mar 3, 2025
Closed
@escopecz escopecz changed the title Merge pull request #2106 from acquia/MAUT-10872_MAUT-10873_MAUT-10875… Resolving PHP 8.0 warnings Mar 3, 2025
@escopecz escopecz added ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 3, 2025
@escopecz escopecz moved this to 🦸🏻 Needs 2 tests in Open Source Fridays Mar 3, 2025
@escopecz escopecz requested review from a team, fedys and dadarya0 and removed request for a team March 3, 2025 12:07
fedys
fedys approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@fedys fedys left a comment

Choose a reason for hiding this comment

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

Thanks @escopecz! The code changes look good.

@codecov Codecov
Copy link

codecov bot commented Mar 3, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.67%. Comparing base (bad5c51) to head (8862004).
Report is 3 commits behind head on 6.x.

Files with missing lines Patch % Lines
...p/bundles/FormBundle/Controller/AjaxController.php 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                6.x   #14664   +/-   ##
=========================================
  Coverage     64.67%   64.67%           
  Complexity    34675    34675           
=========================================
  Files          2273     2273           
  Lines        103593   103596    +3     
=========================================
+ Hits          66994    66996    +2     
- Misses        36599    36600    +1
Files with missing lines Coverage Δ
...p/bundles/LeadBundle/Entity/LeadListRepository.php 76.89% <100.00%> (+0.07%) ⬆️
...bundles/LeadBundle/Helper/ContactRequestHelper.php 89.77% <100.00%> (ø)
...p/bundles/FormBundle/Controller/AjaxController.php 18.98% <0.00%> (-0.50%) ⬇️

... and 1 file with indirect coverage changes

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review and removed ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 3, 2025
@escopecz escopecz moved this from 🦸🏻 Needs 2 tests to ⏳︎ Needs 1 more test in Open Source Fridays Mar 3, 2025
@dadarya0
Copy link
Contributor

dadarya0 commented Mar 4, 2025

Tested , working fine :-

  1. Form submission is working fine, verified form submitted and contact has been created with provided input value
image
  1. mautic:segments:stat command is working as expected
image
  1. Landing page tracking is working as expected.
image

dadarya0
dadarya0 approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@dadarya0 dadarya0 left a comment

Choose a reason for hiding this comment

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

Tested , working fine

@dadarya0 dadarya0 added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review labels Mar 4, 2025
@escopecz escopecz merged commit 1fba7e8 into mautic:6.x Mar 4, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from ⏳︎ Needs 1 more test to 🥳 Done in Open Source Fridays Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedys fedys fedys approved these changes

+1 more reviewer

@dadarya0 dadarya0 dadarya0 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged refactoring The change does not change behavior but improves the code unforking Used for PRs in the Acquia's unforking initiative
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
6.0.0-rc
Development

Successfully merging this pull request may close these issues.
4 participants
@escopecz @dadarya0 @fedys @ts-navghane