CRM-20561 Use composer for Net_SMTP and Net_Socket and patch for TLS and CiviCR…

[seamuslee001]

…M Custom Error

[seamuslee001]

ping @totten @xurizaemon

I believe this covers all of Civi's customisations and ports the TLS patch from upstream to 1.6.x versions. We need 1.6.x for php compatibility with our minimum requirements

[xurizaemon]

filename typo here (execption should be exception)

[xurizaemon]

filename typo here (smpt should be smtp)

[xurizaemon]

Thanks @seamuslee001 for doing this, really appreciate it.

Can I suggest we clean up the filename typos (execption, smpt) to avoid future confusion? Otherwise looks good visually.

[seamuslee001]

@xurizaemon @totten this is a diff of the 2 files in their composer form v packages form https://gist.github.com/seamuslee001/87fc83eed8c1f2b27e9dc1807584914f

[xurizaemon]

I thought when I ran into this bug whether to raise again the PHP5.3 compatability question. (Opted not to because - if I recall - the last civicrm-dev thread on it felt like it lacked direction or a clear decision.)

Instead of just using the current Net_SMTP, we're doing this stuff to bridge the gap btw PHP5.3 and PHP7. I question whether it's worth the effort. At some point we will have to let PHP5.3 go.

I opted to not tackle that when I saw that Net_SMTP 1.7 was PHP7 and 1.6 was 5.3 compatible, but now I feel bad that @seamuslee001 has had to put (even a moment of) time into maintaining support for both.

[seamuslee001]

@xurizaemon the problem tl/dr is that unless we drop 5.3 as a minimum and therefore increase PHP on the test box then we need to maintain the compatibility but really its a small change IMO, I would also note that this work is already done with the packages version its just making sure we aren't missing anything we have already put into our code base

[xurizaemon]

I get that, and I do appreciate that you did do it. It was 4:30am when I realised so my enthusiasm was super low. But no-one is paying us to suffer the death of tiny cuts that is maintaining old PHP compatibility, so I say the sooner awful old code is gone the better.

At the time it seemed like we'd maybe not have had a few months of TLS issues if we'd tracked more current Net_SMTP, so I was feeling a bit sore about tracking old releases of packages on this front!

[xurizaemon]

Also @seamuslee001 thanks for the reminder that "upgrading test box" is currently a reason to not change PHP requirements. This reminds me that at conf there was general agreement partners would benefit from having readier access to their own test infra, so I can usefully put energy into that (& eventually reduce requirements to support old PHP versions).

[seamuslee001]

@xurizaemon Also makes me think (the TLS issue) about if this is something that the LTS will encounter as well and certainly 4.6 won't be dropping 5.3 requirement

[xurizaemon]

Yeah, LTS will be similarly affected and will need either this PR or civicrm/civicrm-packages#184

[mickadoo]

It might be worth noting that composer recommends using the caret themselves, but this would mean all versions under 2.0 would be allowed.

[seamuslee001]

hi @mickadoo allowing anything under 2.0 would be a problem as version 1.7 specifies as part of the composer.json that min PHP is 5.4 which > our min php of 5.3 and php 5.3 is what is installed on the test boxes

[mickadoo]

Looks like a positive change moving library management to composer for this 👍

I left a comment about the versioning, but maybe it's intentional.

It seems to me that dropping support for 5.3 wouldn't be a radical move since it reached its end of life almost 3 years ago, but I can see from the comments it might not be so easy.

[totten]

I like the concept.

IMHO, there's one simple thing blocking merge: we need someone to say, "On my test/dev system, I have applied this patch (and the corresponding removal patch for civicrm-packages) and successfully sent some emails over SMTP."

[xurizaemon]

I'd like to test this, but I want to confirm the expected steps to correctly test, and don't see them here or on CRM-20561. Is it:

• Apply the patch
• Remove packages Net/SMTP, Net/Socket
• Run composer update

@seamuslee001 @totten is that correct?

[seamuslee001]

@xurizaemon that should be right but you could replace composer update with composer install as it should be clean but composer update will also do the job.

[seamuslee001]

report from @xurizaemon

looking now
8:32 AM
can't reproduce the original bug  
8:32 AM
not sure - maybe some other fix got merged - i don't think https://github.com/civicrm/civicrm-packages/pull/184 did

Just commenting PR 184 is one of the patches that gets applied as part of this PR in composer. I was also able to send a test email using AUG's test site @totten

[xurizaemon]

Re the above comment @seamuslee001 - that was when testing without the PR, via ElasticEmail.

I want to reproduce the TLS auth bug without the fix applied, then confirm this PR fixes it.

It's sending emails even though AFAICT none of the following relevant PR are merged yet:

• [needs-rebase] Net_SMTP, Auth_SASL, Net_Socket - Load via composer #7361
• CRM-18408 - composer.json - Update ca-config #10329
• CRM-20561 Use Composer to install Auth_SASL #10385 < this one was
• CRM-20561 Use composer for Net_SMTP and Net_Socket and patch for TLS and CiviCRM Custom Error #10387
• CRM-20561 Use composer to provide Auth_SASL #10388
• (WIP) CRM-18408 - Net_SMTP - Use CA_Config for SSL options civicrm-packages#182
• (WIP) CRM-18408, update for TLS issues seen in PHP5.6 civicrm-packages#183
• Update for PHP5.6.x changes with regard to TLS. civicrm-packages#184

Will update if able to repro the original problem then confirm the fix is good - don't feel I've sufficiently tested to give thumbsup yet, sorry!

[seamuslee001]

Jenkins re test this please

[totten]

We did some more updates following closer review of the diff between old and new code.

Also, tested with sending individual mails and bulk mails via mailcatcher. That doesn't exercise all the edge-cases of authentication, but it does address the risk that (eg) the move to composer might mess up classloading.

[seamuslee001]

@totten tests have passed