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.

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

Upgrade Mime_mail to fix issues with PHP7.2 #205

Merged
merged 1 commit into from
May 22, 2018
Merged

Upgrade Mime_mail to fix issues with PHP7.2 #205

merged 1 commit into from
May 22, 2018

Conversation

seamuslee001
Copy link
Contributor

No description provided.

@seamuslee001 seamuslee001 force-pushed the mimepart_subparts_array branch from 63dd901 to e933a79 Compare May 5, 2018 22:40
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

ping @eileenmcnaughton @totten @monishdeb

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001 seamuslee001 changed the title Declare that subparts in mimePart is an array by specifying default Upgrade Mime_mail to fix issues with PHP7.2 May 7, 2018
@seamuslee001 seamuslee001 mentioned this pull request May 7, 2018
Closed
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton this now includes an upgrade to the latest version of Mail_Mime

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@totten
Copy link
Member

totten commented May 15, 2018

jenkins, test this please

@seamuslee001 seamuslee001 mentioned this pull request May 15, 2018
Merged
@seamuslee001 seamuslee001 force-pushed the mimepart_subparts_array branch 2 times, most recently from 03b89c9 to be45ce8 Compare May 19, 2018 21:50
@seamuslee001
Upgrade Mail_Mime to latest version
408d5e1 
Upgrade Mimedecode as well

re-add fix for CRM-3133

Reremove pass by reference
@seamuslee001 seamuslee001 force-pushed the mimepart_subparts_array branch from 091db82 to 408d5e1 Compare May 19, 2018 23:57
@monishdeb
Copy link
Member

monishdeb commented May 22, 2018 •
edited
Loading

This PR picks the latest changes from Mail_mime and Mail_mimeDecodeFrom package and from code pov after I crosschecked what are the changes bring bought up during upgrade and how it will impact the Civi codebase, here's my report on code POV:

  1. Remove underscores from protected method and variables - upstream commit
    Except for the $mime->_headers Civi doesn't use any of the function/variables directly as I greped the universe. And @seamuslee001 has already submitted a PR Update variable name as per upgraded Mime_mail package to support PHP7.2 civicrm-core#12169 in this regard.
    Without this fix it shows a placeholder in preview header which was suppose to be Subject text as in:
    screen shot 2018-05-22 at 12 58 58 pm

  2. Drop PHP4 and add PHP7 support - upstream commit
    All the changes are safe and I have tested on php7.2 instance

  3. Added methods for creating text/calendar messages - upstream commit
    That's a nice feature for text/calendar message and can be utilised in core.

  4. Mail_mimePart file has similar changes as of 1 and 2

  5. Mail_mimeDecode changes makes sense however I didn't found any upstream commits to compare with. So I had to rely on UI test to check any unintended behaviour, although this package is being internally used by Mail_mime and as per my tests on php7.2 UI end and doesn't raise any concern from UI end.

Except the for the minor issue for which I there is already a core PR I didn't encountered any issue in php7.2

@monishdeb
Copy link
Member

monishdeb commented May 22, 2018 •
edited
Loading

Test failure will be fixed by civicrm/civicrm-core#12169 as confirmed on local, which got both these patches applied.

@monishdeb monishdeb merged commit 143f247 into civicrm:master May 22, 2018
@monishdeb monishdeb mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seamuslee001 @totten @monishdeb @civicrm-builder