Missing ntlm_sasl_client.php - it's a mistery

[Claudiu Ceia]

​https://code.google.com/a/apache-extras.org/p/phpmailer/source/browse/trunk/extras/ntlm_sasl_client.php is missing from wp-includes. Wanted to put up a diff but looks like there's a bit of friction and I only noticed this as a result of pre-compiling the wordpress code base for use with HHVM (so I'm not really relying on it to work).

See: ​https://github.com/WordPress/WordPress/blob/master/wp-includes/class-smtp.php#L366

Thanks

The NTLM authentication protocol is unsecure, and ​no longer recommended for use by Microsoft, and we ​have decided to intentionally leave this feature removed from the WordPress distribution.

Please consider using a custom email plugin or dropin if you must still use NTLM.

@bpetty - I don't need to use it, but the SMTP class requires it. I think you guys should drop in a replacement, as it stands it's a bug that will affect any callsite of the SMTP class.

​https://github.com/WordPress/WordPress/blob/master/wp-includes/class-smtp.php#L366

Replying to Claudiu Ceia:

@bpetty - I don't need to use it, but the SMTP class requires it. I think you guys should drop in a replacement, as it stands it's a bug that will affect any callsite of the SMTP class.

I'm not drawing the same conclusions. Could you explain further? The SMTP class works just fine provided SMTP::Authenticate() is called with an $authtype of LOGIN (default) or PLAIN. By default, the PHPMailer class calls Authenticate with an auth type of "LOGIN" (by passing ""). The public variable PHPMailer::$AuthType can be used to set this type. Very simply, just don't set it to NTLM.

The SMTP class works just fine provided SMTP::Authenticate() is called with an $authtype of LOGIN (default) or PLAIN.

Right. However, I would classify it as a bug if a system allows me to do something and then breaks. I think there are a couple of things you guys should consider:

1) People that call SMTP::Authenticate() by passing NTLM and didn't update to the latest version. Sure, it's not supported - the whole case block should be deleted and throw a meaningful exception would be the best option IMO.

2) If the above doesn't match your roadmap / plans, What about just dropping in an empty file with that name and comment on what people need to do if they really want to use NTLM? At least the code won't throw a weird exception about not finding a file that is required but actually never meant to be there in the first place. I still think #1 is the best option though.

Background: I'm trying to get Wordpress to run on ​HHVM. It works fine, with a couple of lines changed. However, HHVM has an option to precompile the code. The pre-compilation step fails because it can't find that file. I can fork it and add the class, or customize the SMTP class - but I think the approach could be better in the SMTP class without me having to fork it.

Well, one of the big issues with adding it now is that we've never had it in there before, so we also never had to deal with the issue of supporting something that was was supported in the past - but shouldn't be because no-one should be using it at this point. If we add it now, we lose that. Fact is, it never worked before, so there's really no concern with never adding it in there now.

What would be a better solution to your HHVM problem would be to submit a patch to PHPMailer upstream that knows to conditionally check for that file since it is actually supposed to be an optional "extra" feature anyway. In fact, this really needs to happen before we consider adding our own custom stubbed file for it in WordPress (just like we advise users not to modify core files, we try not to modify included 3rd party files either ourselves - and this would count as one). We don't include any of the "extra" files that come with PHPMailer right now actually, and have never really needed to for many of the same reasons.

Doesn't HHVM have some other way to get around issues like this as well? I'd think it would be very common with all sorts of PHP applications with dynamically included PHP files.

Well, one of the big issues with adding it now is that we've never had it in there before, so we also never had to deal with the issue of supporting something that was was supported in the past - but shouldn't be because no-one should be using it at this point

I don't see the need to support that switch branch then... is there something I'm missing?

What would be a better solution to your HHVM problem would be to submit a patch to PHPMailer upstream that knows to conditionally check for that file since it is actually supposed to be an optional "extra" feature anyway.

That won't fix the issue with HHVM I'm afraid (actually, it conditionally requires that file as well). The problem is that during pre-compilation HHVM can't know if that condition will be reached or not - it only knows at runtime. The only way it can compile the source correctly is by making sure the file exists.

There are two modes for how HHVM can treat pre compiled code:

1) Always use the pre-compiled package and never check the source (much faster)
2) Fallback to an actual file if it can't find it in the pre-compiled package. (fast, but not as fast as it can be)

I guess my question is, is the expectation that WordPress users would be adding the files they need for SMTP to work? Shouldn't NTLM be killed completely?

There are two modes for how HHVM can treat pre compiled code:

1) Always use the pre-compiled package and never check the source (much faster)
2) Fallback to an actual file if it can't find it in the pre-compiled package. (fast, but not as fast as it can be)

Completion: for #2 it works fine with HHVM, it will just be a runtime exception if that code path is reached, just like it normally does. My question is, why would that code path be there if it's not supported and we want to kill it? I don't see the reason.

Replying to Claudiu Ceia:

There are two modes for how HHVM can treat pre compiled code:

1) Always use the pre-compiled package and never check the source (much faster)
2) Fallback to an actual file if it can't find it in the pre-compiled package. (fast, but not as fast as it can be)

Completion: for #2 it works fine with HHVM, it will just be a runtime exception if that code path is reached, just like it normally does. My question is, why would that code path be there if it's not supported and we want to kill it? I don't see the reason.

Here's the thing that bugs me... if this is a problem, then it's also certainly a problem for the other files we don't include as well. This includes extras/htmlfilter.php, since PHPMailer also conditionally includes that when the appropriate method is called in class-phpmailer.php too, and we don't bundle that either. Just adding ntlm_sasl_client.php wouldn't be enough to fix your problem with HHVM, we'd also need to include the rest of the files as well.

If you're able to get around that with HHVM, there's certainly a way to get around it with ntlm_sasl_client.php too.

I should clarify actually that the current version of PHPMailer doesn't include htmlfilter.php, but the latest version that we're close to bundling with WordPress does. However, there's other extras that are in the current version like class.html2text.inc that we don't include.

This isn't the only 3rd party library we do this with either. We also trim down the new ID3 library too.