Refactor according to PSR-4

[tasso85]

I would suggest that the library should be refactored to comply to PSR-4 (http://www.php-fig.org/psr/psr-4/), so that there will be no longer need for a dedicated autoloading, but the library could easily be integrated with most frameworks providing their own.

[Synchro]

I don't think this is possible while still maintaining backwards compatibility back to 5.0, for example we can't use namespaces.

[tasso85]

You could use pseudo-namespaces like for example:

class PHPMailer -> class PHPMailer_Mailer
class POP3 -> class PHPMailer_Pop3
class SMTP -> class PHPMailer_Smtp
class phpmailerException -> class PHPMailer_Exception

and then name the files:
Mailer.php
Pop3.php
Smtp.php
Exception.php
All to be placed in folder PHPMailer.

This is what I do when I include this library in a project.

[Synchro]

That's not possible without breaking BC. Breaking BC (bearing in mind we have probably hundreds of millions of users) for something as trivial as this is really pointless - there are far bigger architectural changes that would come first - see #88. You might be able to achieve something similar in your own projects by creating wrappers that would mean you don't need to rename things in future.

[programster]

Could PHP mailer not use a new branch or release for this. E.g. I see that phpmailer is currently on 5.2.10, why not have a 5.3 release that requires PHP 5.3 (ties in nicely) and can therefore make use of namespaces.

Worth mentioning that 5.3 is so old now that it is no longer "currently supported" and the earliest supported release is now 5.4

[debugteam]

5.4 has ended last month too...

and today i wondered again when i typed \PHPM that there was no autosuggestion for namespace PHPMAILER and i have to look up the examples folder...

Anyone who wants backwards compatibility could always use an old version. We want PSR-4 NOW! 👍 )

[tasso85]

I would agree with @debugteam

Also, if you have any decent IDE, doing a search and replace would require just seconds.

[Synchro]

I think I agree with this now - ignoring the major rewrite possibilities mentioned in #88, I think it's about time to add namespace support, and whatever else is beneficial in later PHP versions. I suggest we go for PHP 5.4 compatibility (since that's what's needed by the existing XOAUTH2 classes), and I think we should call it PHPMailer 5.4 to avoid confusion!

So, who's up for a nice fat pull request?

[debugteam]

I would leave the "whatever else" part to "someone else" and just include the namespaces:

My suggestion would be:

namespace PHPMailer\PHPMailer

for this files:

        "class.phpmailer.php",
        "class.phpmaileroauth.php",
        "class.phpmaileroauthgoogle.php",
        "class.smtp.php",
        "class.pop3.php",

And

namespace PHPMailer\PHPMailer\extras

for:

        "extras/EasyPeasyICS.php",
        "extras/ntlm_sasl_client.php"

... composer.json, assuming we move it up one folder....

"autoload": {
    "psr-4": {
        "PHPMailer\\PHPMailer\\": "phpmailer"
    }
},

[Synchro]

If we're going to the inconvenience of breaking things, we may as well break things properly and not release something half-baked.

I think namespacing should be combined with renaming files and altering the folder structure to the PSR-4 default conventions, so class.smtp.php -> SMTP.php.

If you look at what's going on in the xoauth branch, you'll see there's about to be a proliferation of classes, and it may be better to hold off merging that lot in until we've restructured the files (and they all require PHP 5.4 anyway). I never have liked the class.* filenames.

I'm thinking of a structure something like:

lib/
    PHPMailer.php
    SMTP.php
    POP3.php
    extra/
        EasyPeasyICS.php
    XOAuth2/
        Provider.php
        Provider/
            Google.php
            Yahoo.php
            Outlook.php

This would also separate all the functional parts from docs, examples, tests etc.

It would also be a good opportunity to improve the way that the SMTP class is loaded - at the moment there's a workable mechanism for injecting a class, but it's only usable via a subclass, which isn't pretty (but it works for BC). The POP3 class is worse.

Bundling EasyPeasyICS makes much less sense in a composer-driven context; it should be moved into its own project. I've never seen anyone use the NTLM class, and I have a suspicion that it doesn't work anyway - the original class is from PHPClasses, so it might be better to unbundle and load it via composer too.

[debugteam]

Maybe it is possible to unbundle XOAuth2 as well?

[Synchro]

XOAuth2 itself isn't bundled. The core classes are loaded from composer and these classses wrap them to implement XOAUTH2 in PHPMailer. I don't see much value in breaking them out as they are not usable outside PHPMailer (unlike EasyPeasyICS).

[Synchro]

Also, gmail is by far the most common SMTP mail gateway I've seen with PHPMailer, so it makes sense to include support for it by default since XOAUTH2 is essentially a requirement for gmail now.

[Synchro]

I've just pushed a first draft of PHPMailer 5.4 to the 5.4 branch. It declares a PHPMailer\PHPMailer namespace, uses a structure similar to what I suggested, but I've not yet merged in the changes from the xoauth branch.

Suggestions and PRs welcome!

[Synchro]

I've been merging the changes into the xoauth branch, so it shouldn't be too painful when it's merged back in.

I've been having trouble getting the POP-before-SMTP tests to pass - it's a fairly difficult thing to get working as it's highly dependent on local and travis config and relies on creating child processes via shell_exec from within PHPUnit. It's working OK for me locally, but I can't get Travis to cooperate. Anyone got any suggestions?

[H1X4Dev]

Why are you even supporting PHP5.0 wtf man. Just make branches and move them there.. Use PHP7 for main branch, i dont understand whats wrong with that. Everywhere its 5.5++

[Synchro]

No it's not. This is a library not an app, so there is very little need to upgrade to the bleeding edge for anything since all it does is reduce the usability of the library. The project has been PHP 7 compatible for ages, but 5.4 was the last time that relevant significant language changes were made in PHP, so that's what we're doing here. In the mean time, help or go away and stop whining.

[tpraxl]

Hello Marcus,

I just had a deeper look into the source code, originally because I wanted to use PHPMailer to generate a valid eml from an HTML-template with image-assets.

I just checked out branch 5.4 and read some of your discussions about future plans for PHPMailer. My humble suggestions for refactoring after looking into it:

Create a Builder for PHPMailer
So that the user is able to write

$builder = new PHPMailBuilder();
$mail =
            $builder
                ->setBody('<the body>')
                ->setAlternativeBody('text')
                //... more
                ->build(); // possible alternatives to build:
               //
               // or – instead of build:
               // ->getClassicPHPMailer();
               // -> getPHPMailer5_4();
               // -> getEMLGenerator();
               // -> getTransportController() ....;

That would make the creation-code slightly more complex for beginners, but it could enhance the readability.

It would also allow you to split the enormous class PHPMailer into separate classes – each with a clear and possibly simple concern. These smaller and more focused classes would be easier to test and exchange if needed.
It would also allow you to get rid of the public fields in PHPMailer, while keeping property-access for the builder (if that should be desired). That may simplify debugging and further development.

Separate sending and protocols from creating the mailbody and headers
If you split PHPMailer into smaller – more focused – classes, you could easily provide the ability to extend PHPMailer or adapt its behavior at runtime while keeping compatibility with legacy code. I'm suggesting this, because I'd really love to use your eml-generation combined with a css-inliner, but possibly without all the other stuff.

Either way: I would suggest testdriven refactoring piece after piece.

While I had a look into your sources, I started working on small improvements.

• I provided a phpunit.xml in order to allow for testdriven refactoring
• I added .php_cs to allow for PSR2-fixing via php-cs-fixer
• I added generated files that popped up in my project folder to .gitignore

I also started to implement the builder and a test for it, but stopped in order to discuss it first.
Not sure how much time I can spend on this, but I would help here and there if you agreed to the suggestions.
Let me know if you're interested.

[Synchro]

What you describe is pretty impossible without major BC breaks, something more for a 6.0 rather than a minor .x release that we're aiming for in 5.4 - see #88 for longer-term ambitions.

Personally I think "fluent" interfaces are terrible - they enhance readability in one small section of code at the expense of making the rest of it very difficult, particularly when it comes to non-fatal error handling, because the only option available in a fluent sequence is exceptions, which are a very blunt instrument and do not allow elegant recovery. Because of that, most fluent systems only use it in trivial sequential setter calls where there are few consequences to errors, and thus they are no more than syntactic sugar that's not actually very helpful.

There is already a phpunit.xml file that's used by the test suite? Generally don't try and add every little anomaly to .gitignore files - for example .idea or .netbeans folders should be in your global git config, not per project - or were you referring to something else?

There's nothing stopping you using a CSS inliner in the current version - I use one myself - and you can already get a .eml finished message using getSentMIMEMessage(). You can't currently send a pre-assembled message, and that is certainly something that could be achieved with better separation as you describe, but even then, with SMTPUTF8, message assembly is dependent on the remote server so it's not a clean division.

The thing I could really use some help with is the oauth2 integration - you'll find it in the xoauth branch, which is based on the 5.4 branch, and I'd like to merge it back in before release - in fact it's all that's holding up 5.4. I find OAuth utterly incomprehensible at the best of times - every implementation seems to be entirely different and incompatible, so abstracting it is proving painful, and even when it's working perfectly it's horrible for users and developers alike, but we have no choice but to do it because gmail is imposing it.

[tpraxl]

What you describe is pretty impossible without major BC breaks

Oh.. well.. you are the one that knows the code best. I believe that you could build the legacy Mailer as well as a freshly designed Mailer with this approach only by choosing which build()-method to call after setting the parameters. I proposed the builder as one layer of abstraction.

Personally I think "fluent" interfaces are terrible - they enhance readability in one small section of code at the expense of making the rest of it very difficult, particularly when it comes to non-fatal error handling, because the only option available in a fluent sequence is exceptions, which are a very blunt instrument and do not allow elegant recovery.

The fluent interface was meant for the builder only. It validates when build is called – and should throw a meaningful Exception, that can be caught and handled – usually just by setting another value to a certain property of the builder and calling build again. Admittedly, that would require the user to separate the call $builder->build() from the fluent api and put it into a try/catch block.

There is already a phpunit.xml file that's used by the test suite?

There's a travis.phpunit.-xml.dist. Do you mean that? I needed to provide a phpunit.xml in order to make the cli-call phpunit working.. Meant for small and fast unit tests and testdriven refactoring.

Generally don't try and add every little anomaly to .gitignore files - for example .idea or .netbeans folders should be in your global git config, not per project - or were you referring to something else?

Yes, I was referring to files like

• cacertfile.pem
• cakeyfile.pem
• certfile.pem
• certfile.txt
• dkim_private.key
• keyfile.pem
• keyfile.txt
• Check for SMTP class before blindly requiring it #1#travis.log, Added Timeout support to SMTP class when reading from remote server #2#travis.log, Change README to markdown and reorganize #3#travis.log

There's nothing stopping you using a CSS inliner in the current version - I use one myself - and you can already get a .eml finished message using getSentMIMEMessage().

That was my original plan. In order to understand how PHPMailer works, I wanted to create UnitTests and that was the point when I got deeper into the sourcecode and the discussions here. It's what I plan to do. Thanks for the hint.

You can't currently send a pre-assembled message, and that is certainly something that could be achieved with better separation as you describe, but even then, with SMTPUTF8, message assembly is dependent on the remote server so it's not a clean division.

I believe that it is possible to separate concerns even then, but I'm not an expert on mail protocols and I don't know these concerns as well as you do. Also, I really don't want to bother you.

The thing I could really use some help with is the oauth2 integration

Sorry, I'm not interested and I could not help you there, even if I was.
I wish you good luck with it!

Kind regards and thanks for your answers.

[programster]

Personally I think "fluent" interfaces are terrible - they enhance readability in one small section of code at the expense of making the rest of it very difficult, particularly when it comes to non-fatal error handling, because the only option available in a fluent sequence is exceptions, which are a very blunt instrument and do not allow elegant recovery. Because of that, most fluent systems only use it in trivial sequential setter calls where there are few consequences to errors, and thus they are no more than syntactic sugar that's not actually very helpful.

+1 Fluent interfaces are evil.

[Synchro]

This is pretty much done in the 5.5 branch (so-called because it now requires PHP 5.5, though only because the dependencies do). I'm closing this ticket to reflect that, please open new tickets for anything you find amiss.

[tasso85]

Why did you choose to use a "double" namespace PHPMailer\PHPMailer? Wasn't a single one sufficient?

[Synchro]

Because there are other projects in the PHPMailer org on GitHub, there may be more in future, and they should have their own consistent namespaces. Other projects do the same thing.

[Synchro]

This is an old ticket. The next version is called 6.0 because of BC breaks that I don't want imposed on automatic semver updates. 5.2 is still 5.2, and still provides its own autoloader by default, though of course you can also load it using composer. 6.0 remains unreleased because I'm still awaiting confirmation that line break issues in #953 are resolved.

[manix]

@Synchro yeah sorry about that, I noticed the broken links and commented before I actually looked at the branches. I hope that gets resolved soon because it is a little confusing having the older version in master. Best!

[Synchro]

Not confusing at all - 5.2 is current master and current release version. As soon as it's released, 6.0 will become master. This is the same pattern as pretty much every other package. 5.2 will move to 5.2-stable.