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

PHP 8.2 support #12469

Merged
merged 22 commits into from Feb 6, 2024
Merged

PHP 8.2 support #12469

merged 22 commits into from Feb 6, 2024

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Jun 12, 2023

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? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

This PR contains the minimal changes to get Mautic working on PHP 8.2 and get the CI green.

The UTF8 conversions aren't necessary since Mautic 3 when we started using UTF8MB4 encoding. I think it's safe to get rid of it. The methods used for it were removed in PHP 8.2.

The ext-iconv should be required as we use it in the code without any checks if it is installed or not.

Old description:

This PR is just to test if there is some BC break required to support PHP 8.2. Based on the PHPUNIT output there doesn't seem to be. Many deprecations and failed tests are due to Swiftmaier which is going to be removed in #11613. So we can do this any time after that is merged. Even after M5.0.0 release.

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Gitpod was updated to PHP 8.2 as well so you can test it on this newer PHP version. There is nothing specific to test. Everything should work as before.
  3. One thing I'd recommend to focus on is sending emails with some UTF8 characters like emojis.

@escopecz escopecz added enhancement Any improvement to an existing feature or functionality blocked Something blocks this PR/issue (e.g. waiting for another PR to be merged) labels Jun 12, 2023
@gnovaro
Copy link

gnovaro commented Dec 20, 2023

Any update about this?

@escopecz
Copy link
Sponsor Member Author

Not on the top of my list at the moment. Feel free to take over.

@escopecz escopecz force-pushed the php8.2 branch 2 times, most recently from 7902f0f to 9b68e0b Compare December 21, 2023 09:58
@escopecz
Copy link
Sponsor Member Author

Test failures on PHP 8.2 reduced from Errors: 31, Failures: 70 to Errors: 0, Failures: 9. I really have to move to other issues although I'd like to get this done. If anyone has a free evening, please take over.

@mautibot
Copy link

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/update-5-0-1/30474/4

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (5d75ab3) 59.30% compared to head (9d69788) 59.27%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #12469      +/-   ##
============================================
- Coverage     59.30%   59.27%   -0.04%     
+ Complexity    33387    33386       -1     
============================================
  Files          2210     2210              
  Lines         99789    99782       -7     
============================================
- Hits          59182    59147      -35     
- Misses        40607    40635      +28     
Files Coverage Δ
...dles/CoreBundle/Controller/ExceptionController.php 86.95% <100.00%> (ø)
app/bundles/CoreBundle/Doctrine/Type/ArrayType.php 90.90% <ø> (-1.10%) ⬇️
app/bundles/CoreBundle/Helper/InputHelper.php 91.17% <100.00%> (ø)
...pp/bundles/CoreBundle/IpLookup/ExtremeIpLookup.php 90.90% <ø> (ø)
app/bundles/CoreBundle/IpLookup/GeobytesLookup.php 77.77% <ø> (ø)
app/bundles/CoreBundle/IpLookup/GeoipsLookup.php 90.47% <ø> (ø)
app/bundles/CoreBundle/IpLookup/IpinfodbLookup.php 91.66% <ø> (ø)
app/bundles/CoreBundle/IpLookup/IpstackLookup.php 86.95% <ø> (ø)
app/bundles/CoreBundle/IpLookup/TelizeLookup.php 84.61% <ø> (ø)
...tBundle/EventListener/DynamicContentSubscriber.php 85.71% <100.00%> (ø)
... and 7 more

@escopecz escopecz added ready-to-test PR's that are ready to test php Pull requests that update Php code and removed blocked Something blocks this PR/issue (e.g. waiting for another PR to be merged) labels Jan 12, 2024
@escopecz escopecz marked this pull request as ready for review January 12, 2024 19:55
@RCheesley
Copy link
Sponsor Member

Could you update the DDEV configuration so folks will be testing it on 8.2 as well?

@escopecz escopecz force-pushed the php8.2 branch 3 times, most recently from 10defe8 to bdd5310 Compare January 12, 2024 20:33
kuzmany
kuzmany previously approved these changes Jan 12, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I did basic tests
Nice job 👍
Tested on ddev

@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jan 12, 2024
@escopecz escopecz added this to the 5.1.0 milestone Jan 12, 2024
@escopecz
Copy link
Sponsor Member Author

A followup PR: #12469

@escopecz escopecz requested a review from mollux January 15, 2024 11:04
@kou
Copy link
Contributor

kou commented Jan 30, 2024

I'm interesting in PHP 8.2 support because Debian GNU/Linux bookworm (the current stable) uses PHP 8.2.

@mollux It seems that your review is a blocker of this PR. Could you take a look at this?

@escopecz
Copy link
Sponsor Member Author

escopecz commented Feb 2, 2024

This PR needs a rebase but I'm not going to do that until I someone will ensure that they will make the second test/review because I don't want to keep this PR up to date and wait for someone who will actually approve the PR for the second time.

@escopecz escopecz added the needs-rebase PR's that need to be rebased label Feb 2, 2024
Copy link

@gnovaro gnovaro left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@escopecz escopecz 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 needs-rebase PR's that need to be rebased labels Feb 5, 2024
@escopecz
Copy link
Sponsor Member Author

escopecz commented Feb 5, 2024

Thanks for the approval! Rebased. Will merge once the CI is green.

@escopecz
Copy link
Sponsor Member Author

escopecz commented Feb 5, 2024

Ok, there is some new issue. I'll have to look at that later.

@escopecz escopecz added the WIP PR's that are not ready for review and are currently in progress label Feb 5, 2024
@kou
Copy link
Contributor

kou commented Feb 6, 2024

How about this like 33e4863 ?

diff --git a/app/bundles/ReportBundle/Tests/Controller/ReportControllerFunctionalTest.php b/app/bundles/ReportBundle/Tests/Controller/ReportControllerFunctionalTest.php
index 171b071f8a..1f0b54fa2d 100644
--- a/app/bundles/ReportBundle/Tests/Controller/ReportControllerFunctionalTest.php
+++ b/app/bundles/ReportBundle/Tests/Controller/ReportControllerFunctionalTest.php
@@ -225,7 +225,7 @@ class ReportControllerFunctionalTest extends MauticMysqlTestCase
 
         $dom     = new \DOMDocument('1.0', 'utf-8');
 
-        $dom->loadHTML(mb_convert_encoding($content, 'HTML-ENTITIES', 'UTF-8'), LIBXML_NOERROR);
+        $dom->loadHTML(mb_encode_numericentity($content, [0x80, 0x10FFFF, 0, 0xFFFFF], 'UTF-8'), LIBXML_NOERROR);
         $tbody = $dom->getElementById('reportTable')->getElementsByTagName('tbody')[0];
         $rows  = $tbody->getElementsByTagName('tr');
 

@escopecz
Copy link
Sponsor Member Author

escopecz commented Feb 6, 2024

@kou would you be able to push such fix as a PR against this branch on my fork? I'm swamped and cannot get to this any time soon.

@kou
Copy link
Contributor

kou commented Feb 6, 2024

Sure!
escopecz#21

escopecz and others added 2 commits February 6, 2024 10:12
mb_convert_encoding(): Handling HTML entities via mbstring is deprecated
@escopecz escopecz removed the WIP PR's that are not ready for review and are currently in progress label Feb 6, 2024
@escopecz escopecz merged commit ca468e6 into mautic:5.x Feb 6, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any improvement to an existing feature or functionality php Pull requests that update Php code ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants