Added missing comments to phpDocs

[evgwed]

Hello! I very often create my own migrations using the vendor/bin/phinx create CreateTestTable command and it creates the next class.

<?php

use Phinx\Migration\AbstractMigration;

class CreateTestTable extends AbstractMigration
{
    /**
     * Change Method.
     *
     * Write your reversible migrations using this method.
     *
     * More information on writing migrations is available here:
     * http://docs.phinx.org/en/latest/migrations.html#the-abstractmigration-class
     *
     * The following commands can be used in this method and Phinx will
     * automatically reverse them when rolling back:
     *
     *    createTable
     *    renameTable
     *    addColumn
     *    renameColumn
     *    addIndex
     *    addForeignKey
     *
     * Remember to call "create()" or "update()" and NOT "save()" when working
     * with the Table class.
     */
    public function change()
    {

    }
}

This code is correct and simple, even contains a detailed PHPDocs.

I want to create a new table test and add an integer numeric field "num" to it.

    public function change()
    {
        $this->table('test')
            ->addColumn('num', 'integer')
            ->create();
    }

This will also be a valid code, migration is correctly executed. But comments in the PHPDocs will be incorrect. It will be necessary to add the following text to the commentary.

/**
     * @throws \InvalidArgumentException
     * @throws \RuntimeException
*/

And these actions must be repeated each time the addColumn method is used in the migration. If you do not do this, my code editor (PHPStorm) swears at the line with the call to the addColumn function.

And most developers are faced with this problem when writing their migrations. I propose to simplify the writing of migrations and to indicate this text in advance in the comments. This does not make the migration code complex, but it helps when writing migrations.

I propose the simplest and most obvious solution to this problem.

[dereuromark]

There is also a even simpler solution: Set the PHPStorm (or other IDE) to the proper setting: Only primary level (0) for checking. By default using anything higher usually exposes a risk of false positives or forgetting things when refactoring and is considered not the best practice and unreliable even :)
Without a deep and code sniffer backed solution this is always suuuper error prone.

So while I don't say this here is bad, I think this can easiest be fixed in the IDE itself + code sniffer.
Primary level is a good and clean way to consistently verify throws annotations.
We have a sniffer that actually does verify and fix this in https://github.com/spryker/code-sniffer - you are welcome to try it.

[codecov-io]

Codecov Report

Merging #1285 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1285   +/-   ##
=======================================
  Coverage   74.83%   74.83%           
=======================================
  Files          35       35           
  Lines        4777     4777           
=======================================
  Hits         3575     3575           
  Misses       1202     1202

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 924b3c2...a85e3be. Read the comment docs.

[evgwed]

@dereuromark
The use of your solution is possible, but not in all teams and not in every project. Your solution suggests doing more and installing an additional package, or drowning out checks that allow you to write cleaner code.
Thanks for the feedback.

Checking the code goes not only on the comments, but since the IDE highlights this place, as wrong, I want to fix this with minimal effort.

[dereuromark]

Actually, no :) A code sniffer should always be already installed. You could just copy and paste the file and assert then without additional baggage even. But you seemed to have missed my point then.

[evgwed]

In the change method, only the following methods are allowed:

• createTable
• renameTable
• addColumn
• renameColumn
• addIndex
• addForeignKey

Only the method addColumn from this list causes exceptions. This method is one of the most popular when creating new tables. Therefore, I propose to add relevant comments to the phpDocs.

/**
     * Change Method.
     *
     * Write your reversible migrations using this method.
     *
     * More information on writing migrations is available here:
     * http://docs.phinx.org/en/latest/migrations.html#the-abstractmigration-class
     *
     * The following commands can be used in this method and Phinx will
     * automatically reverse them when rolling back:
     *
     *    createTable
     *    renameTable
     *    addColumn
     *    renameColumn
     *    addIndex
     *    addForeignKey
     *
     * Remember to call "create()" or "update()" and NOT "save()" when working
     * with the Table class.
     *
     * @throws \InvalidArgumentException
     * @throws \RuntimeException
     */

@dereuromark Do you think this is superfluous?

@chinpei215 @robmorgan @shadowhand @rquadling @cyrusboadway @coatesap

[rquadling]

You could simply overwrite the default class/template and add your own comments.

From my teams phinx.php script (your's could be phinx.yml) ..

        'migration_base_class' => \OurProject\Migration\AbstractMigration::class,
        'templates' => [
            'file' => './lib/OurProject/Migration/Phinx/Templates/Master.template',
        ],

sort of thing.

[chinpei215]

I think the root cause of the problem is that two useless @throws annotations are written in the doc comment of Table::addColumn(). The first one is thrown when the adapter is not set for some unknown reasons, and the second one is thrown when the column type is invalid. I don't think users want to wrap their code with try-catch blocks to catch those exceptions. So I think those @throws annotations are useless.

I think we should use @throws annotations only for exceptions that users should catch. A typical example of those exceptions would be something like a FileNotFoundException. Let's assume there is a FileReader class, and you want to open a file by using it. In this situation, you should handle at least an error when the file doesn't exist. And if @throws FileNotFoundException is written in the doc comment of FileReader::__construct(), it can tell you to write a try-catch block by yourself.

In conclusion, I would like to suggest removing those useless @throws annotations instead. For now, those annotations seem to be written only for making CI satisfied.

[dereuromark]

Closing as throws should really mainly be for the ones implementable, not the real error cases not to be handled.