What coding standard is this? I gather it's not PSR-2. I've had trouble with php-cs-fixer in the past suggesting changes that are actually breakages (it's used in scrutinizer), and I see there is one phpdoc mistake in the suggested diff (adds a . after a multiline code tag).

Could you fix whatever is breaking the build.

What coding standard is this? I gather it's not PSR-2.

The main rule activated is @Symfony, and refers to a series of rules that inclue PSR-2. You can inspect them here: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.2/src/RuleSet.php#L55
Take care to browse the repository at 2.2 branch, the latest one that supports PHP 5.5.

I've had trouble with php-cs-fixer in the past suggesting changes that are actually breakages

Since version 2 PHP-CS-Fixer distinguishes safe fixers from risky ones. But what does the tool consider risky? Risky is when and only when a user overwrites default PHP behaviours, for example is_null fixer converts is_null($variable) to null === $variable, and this could break compatibility if the user defined a custom namespaced is_null() function.
Apart from these edge cases, version 2 suppresses regex substitutions in favor of proper code tokenization, that led to a huge improvement avoding false-positives and false-negatives.

The fixers proposed here don't introduce any breakage, even though some of them are risky; after ending this discussion/review I'll apply the fixer and you'll see no test failing.

I see there is one phpdoc mistake in the suggested diff (adds a . after a multiline code tag).

The fixer applying this change is phpdoc_summary that has this purpose:
Phpdocs summary should end in either a full stop, exclamation mark, or question mark.
I don't see any breakage/mistake: can you point build line out please?

Could you fix whatever is breaking the build.

This PR integrates coding standards check in the build to force commiters adhere to the standards, but I intentionally avoided applying them for now because I expected this topic to be discussed, and to show you what changes, so for now the build intentionally fails.
As soon as we agree on the set of fixers to adopt and the way to check them, I'll push a final commit that applies those changes and the PR build will pass.

I've fixed just about everything flagged. The few things left are annoying - php-cs-fixer and PHPStorm seem to have different definitions of Symfony's coding standard. php-cs-fixer's approach is the more annoying, imposing inconsistent spacing on param descriptions, and requiring an enormous number of blank lines in docblocks - this is one reason I've disliked Symfony's standard in the past. PHPStorm doesn't seem to format params with phpdoc nullable types, like ?string correctly.

There are a few of the examples with the trailing . problem I mentioned remaining - they are really pointless and would look silly, plus be prone to provoking copy/paste errors, so I'm really not keen on forcing them for the sake of an arbitrary standard. Here's an example of what it would result in:

echo 'hi';

.

As you can see - just silly.

I've fixed just about everything flagged.

I appreciate that you took this PR advice to fix the current code.

But my intention with this PR is wider.

1. It is good for a project to choose and adopt a standard for writing the code, so it's easier to read for everyone
2. Standards are boring to check and apply manually, computers instead are good at it: a dedicated tool can check and fix everything automatically
3. For a standard to be called so, it must be mandatory for everyone: it must be checked at the build status, so every commit from the maintaners and every pull-requests from contributors are granted to adhere to the standard

Instead, in the commit 31493b6

1. I don't see the .php_cs file where you can specify the standards adopted by this project
2. I don't see any changes in the .travis.yml file in order to force the standard ad the build level

The result is that the next commit from you or from contributors can be written without rules, and either you have to check them manually (boring and error-prone) or accept the code standard of someone else.

Moreover, I am a bit sad that you committed a part of this PR without actually discussing and merging this very PR. As a sign of gratitude for a contributor that spent his/her time improving your project, it is appreciated to discuss the topic and to merge the code proposed by him/her, so the contributor can become part of the code through commits authoring.

There are a few of the examples with the trailing . problem I mentioned remaining

1. phpdoc_summary fixer that adds the point is just one of the hundred fixers enable, we can easily disable this if you don't like it
2. All fixers are well tested, here you can see the phpdoc_summary test: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.5/tests/Fixer/Phpdoc/PhpdocSummaryFixerTest.php I honestly highly doubt there is a bug in it

Here's an example of what it would result in:

The code that follows this statement, echo 'hi';, is not affected by phpdoc_summary fixer: can you please provide the before and after of the code you consider silly?

Hold on a sec! The intention is to accept your PR, and it's fully appreciated that you're contributing - however, since it needs to pass before committing, the things it's going to check need to be fixed beforehand, so that's what I started on. That way your PR will pass tests before merging, rather than after. This project already used PSR-2 as the coding standard, and nearly all the changes suggested by php-cs-fixer are for the Symfony standard; what's new is your config change to enforce the rules, which is very welcome.

The only thing that overlaps with your PR was the addition of the php-cs-fixer package in composer.json, which I'd added locally and committed by accident - I've pushed a reversion of that change, so it should apply cleanly again - if not, please rebase.

This is the code that caused the extra .:

 * <code>
 * $mail->Debugoutput = new myPsr3Logger;
 * </code>

and this is the diff php-cs-fixer suggests, using your config file:

@@ @@
      * $mail->Debugoutput = new myPsr3Logger;
-     * </code>
+     * </code>.

phpdoc used to recommend using code or pre tags in docblocks for inline code examples (why these tags are there), and it's simply inappropriate to add a trailing full stop after it. That said, phpdoc now recommends markdown instead, so I've converted the code tags to use that, which avoids the problem, though see below for a similar complication.

For some reason it's also suggesting an incorrect change for a namespaced class that is imported correctly, as far as I can see:

@@ @@
      *
-     * @uses   SMTP
+     * @uses   \SMTP

There are some inappropriate capitalisation changes too:

@@ @@
      * @param string        $basedir  Absolute path to a base directory to prepend to relative paths to images
-     * @param bool|callable $advanced Whether to use the internal HTML to text converter
-     *                                or your own custom converter @see PHPMailer::html2text().
+     * @param bool|callable $advanced whether to use the internal HTML to text converter
+     *                                or your own custom converter @see PHPMailer::html2text()

This is flagged for the trailing ., which is fine, but it also makes a capitalisation change that's just wrong. Oddly, it doesn't suggest the same change on the previous line, which uses exactly the same capitalisation pattern. I think that's a bug.

It doesn't accept a : as an end of sentence marker, and suggests an inappropriate fix:

@@ @@
      * By default PHPMailer will use `echo` if run from a `cli` or `cli-server` SAPI, `html` otherwise.
-     * Alternatively, you can provide a callable expecting two params: a message string and the debug level:
+     * Alternatively, you can provide a callable expecting two params: a message string and the debug level:.

You can't resolve that one by removing the blank line before the code block as it will fail to render the markdown, and it then suggests a trailing ```. as a fix - which is back to the same problem that the code tag had. I think this is a bug too.

I absolutely agree that code style should be enforced - but it's a bit pointless if the enforcement mechanism introduces errors of its own.

since it needs to pass before committing, the things it's going to check need to be fixed beforehand, so that's what I started on. That way your PR will pass tests before merging, rather than after.

I'm sorry I didn't explained myself enough. It was never my intention to have the build passing after merging, it should never happen. The build process is there for this very reason: a PR needs to pass before merge. So why I am still leaving the build failing (for now)?
When a project is inspected for the first time by a new coding standard, often a lot of files are marked with changes, and the diff of the commit is very large.
If I had to push a PR that simultaneously:

1. Introduces a coding standard and inserts it on Travis
2. Applies the coding standard

It would be much harder for you (the maintaner) to inspect the Files changed tab on Github and to understand the changes of the former and the ones of the latter.

Without immediately applying the fix:

1. Files changed tab on Github consists of a few files, easy to read and discuss
2. I can prove Travis build matrix is correctly adapted, because it is shown where it fails and why

So, when you'll tell me the configuration is ok to you, I'll apply the fix, the build will pass and then, and only then, you will agree whether or not merging the PR

About the config:

1. I agree with your point against the dot fixer; I disabled phpdoc_summary
2. Regarding \SMTP, most active PHP projects right now tend to use FQCN in docs, in this case it should be PHPMailer\PHPMailer\SMTP, and so it adds the backslash; this project follows another convention, so I disabled the related php_unit_fqcn_annotation fixer
3. Still the build is failing due to phpdoc_align fixer, that align:

@@ @@
      *
-     * @param string   $host    The hostname to connect to
-     * @param int|bool $port    The port number to connect to
-     * @param int|bool $timeout The timeout value
+     * @param string   $host        The hostname to connect to
+     * @param int|bool $port        The port number to connect to
+     * @param int|bool $timeout     The timeout value
      * @param string   $username
      * @param string   $password
      * @param int      $debug_level

That I consider valid, so if now the config is ok to you, I'll apply the fix, push a new commit, the PR should pass and you can merge it.

composer.lock is removed

I care a lot that you note the second commit of this PR removes composer.lock. This is because:

1. We must check coding standards on the lowest PHP version
2. PHP-CS-Fixer supports PHP 5.5 till v2.2
3. PHP-CS-Fixer v2.2 can't be installed on later PHP version because of dependencies version resolution conflicts.

This is not an issue, and instead is good. Composer documentation says Commit Your composer.lock File to Version Control, but the statement only refers to applications, not to libraries.

If you commit the composer.lock on PHP 5.5, Travis will always use the locked dependencies to run the build, but on newer PHP versions you may miss new improvements or new changes, and you can't adapt to it as soon as they happen.
If you commit the composer.lock, you may end up in the future with a dependency considered unsecure/broke without bugfixes, and you must spend days to adapt to the newer version.
If you don't commit it, you catch the eventual forward-compatibility break as soon as they appear.

Can you explain what the difference is between the php versioning and the matrix approach?

That \SMTP fix still seems buggy to me - it doesn't flag the same thing for Exception used elsewhere, and that is especially important since that name is in both global and local namespaces. If I set it to PHPMailer\PHPMailer\SMTP, PHPStorm actually flags it as an error because it knows it's already in the PHPMailer\PHPMailer namespace, so it would be referring to PHPMailer\PHPMailer\PHPMailer\PHPMailer\SMTP! I would understand better if it was suggesting \PHPMailer\PHPMailer\SMTP (with a leading \) if it wants unambiguous FQCNs.

I agree about the param label indenting being out - that's a difference between php-cs and PHPStorm for the Symfony standard - I'll report that as a bug in PHPStorm. It seems to be different when some params are missing labels - it only makes the params that have descriptions line up, and ignores the length of param names that don't have descriptions (i.e. it doesn't consider hypothetical descriptions). I'm not that keen on adding all that white space anyway - it leads to silly line wrapping if you have a short line length limit and long var names, making things distinctly less readable when you're looking at the code, and it makes no difference to rendered phpdoc output.

I'm familiar with committing composer.lock for projects to ensure compatibility, but as you say, that should apply to whole projects rather than individual libraries. It should fail as early as possible, especially across multiple PHP versions, so I'm entirely with you on removing it for that reason.

Can you explain what the difference is between the php versioning and the matrix approach?

Withing matrix you can have specific env variable, so we can tell with a variable to do the code-coverage or the php-cs fixing only on one particular version of PHP. This approach:

1. reduces the build time, because those operations are done once instead of five times
2. removes unnecessary operations: code-coverage can be uploaded on code-quality tools like scrutinizer only once for every commit, and php-cs fixing must be done on the lower version to ensure compatibility, no need to check it on latest

That \SMTP fix still seems buggy to me

Symfony standards suppose users always use FQCN, and so the fixer adds the root backslash to avoid IDE like PHPStorm doing a mess. As you said, the fixer expects you to write PHPMailer\PHPMailer\SMTP so it can add the missing root \. By the way it's just a convention: this project follows another convention, an that's fine.

I've applied the fix and pushed the commit, if you like it the PR is ready to be merged.

Thanks for all that!