-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
Conversation
Test failure is related on this one but it is described as above |
Jenkins test this please |
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? |
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? |
658169b
to
afa5a47
Compare
@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
So yes, it will be applicable to have a message for site owner to notify about the package upgrade |
@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? |
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? |
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. |
Ok, will do it. I am merging this PR as it fixed the test failure and was earlier tested by me as mentioned above. |
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