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

Update variable name as per upgraded Mime_mail package to support PHP7.2 #12169

Merged
merged 2 commits into from
May 22, 2018

Conversation

seamuslee001
Copy link
Contributor

Overview

This changes the variable that is being set in Mail_Mime, this PR is dependent on https://github.com/civicrm/civicrm-packages/pull/205/files but that PR needs this PR to pass all unit tests. @eileenmcnaughton @monishdeb @totten @JoeMurray to support PHP7.2 we need to upgrade the Mail_Mime and Mail_MimeDecode packages. Packages PR 205 does this however this also changes a number of variables from having the _ prefix to declaring the private nature of them in the doc block. I have done a grep and this is the only reference to the headers this way in the core repo.

We will need to make sure we merge both PRs sequentially to and at the same time to make sure we don't have any problems

@seamuslee001
Copy link
Contributor Author

Test failure is related on this one but it is described as above

@monishdeb
Copy link
Member

Jenkins test this please

@monishdeb
Copy link
Member

This PR is dependent on civicrm/civicrm-packages#205 and I have already tested this PR as per my comment

@seamuslee001 @eileenmcnaughton I think we should add a pre upgrade 5.3 message in Civi to notify this package upgrade, what do you think?

@eileenmcnaughton
Copy link
Contributor

DId that last build fail AFTER you merged the other?

PHP Fatal error: Cannot access protected property Mail_mime::$headers in /home/jenkins/buildkit/build/core-12169-1ppy0/sites/all/modules/civicrm/api/v3/Mailing.php on line 587

@monishdeb re upgrade message - is there an action site owners need to take?

@monishdeb
Copy link
Member

monishdeb commented May 22, 2018

@eileenmcnaughton yes and updated the PR, this will fix the test failure. Which cite an issue that due to package upgrade some of the functionalities won't work like earlier. As in this case after upgrade, we cannot directly access the Mime variables (which are now protected), like here in order to fetch Subject use
Mail_mime->headers()) and you will find it in

Array (
    [MIME-Version] => 1.0
    [Content-Type] => multipart/alternative;
 boundary="=_20b15c6a9feff8d5843abc8f325de63b"
    [Reply-To] => r...@EXAMPLE.ORG
    [Return-Path] => b...@EXAMPLE.ORG
    [From] => "FIXME" <info@EXAMPLE.ORG>
    [Subject] => Sample CiviMail Newsletter
    [List-Unsubscribe] => <mailto:u...@EXAMPLE.ORG>
    [To] => <>
    [Precedence] => bulk
    [X-CiviMail-Bounce] => b...@EXAMPLE.ORG
)

So yes, it will be applicable to have a message for site owner to notify about the package upgrade

@eileenmcnaughton
Copy link
Contributor

@monishdeb I'm still confused - why does a site owner need to know about that - isn't the change handled within our codebase? What will the site owner need to do?

@monishdeb
Copy link
Member

I was thinking beyond the codebase, what if any extension/custom php directly uses the property or any other function of mime class which might break due to the upgraded package. Or could we ignore this for site owner?

@eileenmcnaughton
Copy link
Contributor

Well you could email the dev list to notify people. This isn't a site owner thing but a dev thing & one that is unlikely to hurt anyone I suspect.

We don't really support people accessing things like this directly - it's at own risk - but no harm in telling devs.

@monishdeb
Copy link
Member

Ok, will do it. I am merging this PR as it fixed the test failure and was earlier tested by me as mentioned above.

@monishdeb monishdeb merged commit 7d2bea5 into civicrm:master May 22, 2018
@eileenmcnaughton eileenmcnaughton deleted the mime_mail_upgrade branch May 22, 2018 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants