Silently fail when comments not supported in migrations.

[SamMousa]

What steps will reproduce the problem?

1. Use SQLite as a backend.
2. Create a table with a column that has a comment.

   What is the expected result?

Create the column as specified.

What do you get instead?

An error: Exception 'yii\base\NotSupportedException' with message 'yii\db\sqlite\QueryBuilder::addCommentOnColumn is not supported by SQLite.'

I'm not 100% sure if failing silently is the best solution / should be the default behavior. It would be nice to have something that indicates this. For example adding a second parameter:

$this->integer()->comment('Comment is nice but not required', true);

Where the true indicates that it can fail silently in case comments are not supported by the backend.

[githubjeka]

Why not use try..catch ?

[SamMousa]

Because then I do not know what is not supported; it could be the data type, a default value or whatever other configuration I'm using. The point is that comments typically are for improved maintainability but are not strictly required for running your code.

[samdark]

It makes sense.

[SamMousa]

@samdark, how should I implement this?

• Name of the second parameter.
• Default value true or false

[klimov-paul]

It makes sense.

I am usure: silent fail for missing DB feature is risk prone. If developer writes migration, which adds a comment to the field, it is better to draw his attention to the fact comment adding is not possible. He may rely on this comment somewhere in this program (rare, but possible case).

Although, comment addition seems not crucial feature.

Perhaps we should introduce a warning for the migration, which can notify that comment is not supported by the engine, but still allow code to run.

[samdark]

Although, comment addition seems not crucial feature.

That's why I said it's OK.

Yes. A warning is a good idea.

[SamMousa]

Well isn't it the choice of the develop automatically if we provide the second parameter? If I depend on this comment anywhere else in my code I'm not gonna have it silently fail.

That way the developer writing the codes makes the decision whether it is a requirement (ie fail to run the migration if not supported) or just optional.

--> So the default would then be current behavior, so you have to explicitly disable the error on a per-comment basis:

$this->integer()->comment('a nice number', true);

function comment($comment, $silentFail = false)
{
....
}

[AnikanovD]

@SamMousa Second parameter looks as hotfix.

I'm agree with @klimov-paul. When i use SQLite, it means that i understand "SQLite is not support comments". Migration can say to developer about incompatibility, but should not interrupt executing.

[SamMousa]

@AnikanovD isn't the whole point that as a developer I abstract away from the storage layer?

What if I create an extensions that has migrations? I do not know what storage backend other developers that use my extension might use... When I add comments to fields to improve "documentation" I still want people to be able to use my extension on SQLite...

The storage backend is configuration, code and configuration are not done by the same people / team.

[AnikanovD]

@SamMousa Your reasoning is clear to me. I do not dispute the ability to run the migration.

...but should not interrupt executing.

I have a question. How to implement warning instead exception? I'm not sure that adding second parameter is the right solution.

• How to check param silentFall in Migration::createTable()?
• Looks unclear

$this->createTable('some_table', [
    'horse' => $this->integer()->comment('fail safe', true),
    'battery' => $this->integer()->comment('fall, but why only here?'),
    'staple' => $this->integer()->comment('already fail safe', true),
    'correct' => $this->string()->comment('why i need always pass param silentFail?', true),
]);

P.S. I almost did not communicate in English. My posts may look aggressive, but it's not. Sorry)

[SamMousa]

@AnikanovD I can handle some aggression ;-)

Anyway, the developer writing the migration should decide if the migration should fail in case comments are not available. So if for any reason I have a system where the comments must be present than I do not silent fail. If I just add them for convenience then I can enable silent fail and anyone running the migration should see that comments are not supported. Warnings could be implemented by just writing output I guess, since that's the approach migrations currently use. @samdark what do you think?

[rob006]

We could add additional param to Migration:

public $dbRequirements = [];

If you require comments you set

public $dbRequirements = [
    'comments',
];

in your migration, then exception will be thrown. Otherwise only warning in output will be displayed.

[SamMousa]

That's definitely possible, though as far as I know we don't want to change default / current behavior. Also personally I think it's easier to do it in the comment call.

[rob006]

We can reverse this solution and create param like notRequiredDbFeatures which will be empty by default and you need insert comments into it to ignore unsupported comments exceptions.

Or something more sophisticated:

protected $defaultDbRequirements = [
    'comments' => true,
    'transactions' => true,
    ...
];

public $dbRequirements = [
    'comments' => false,
];

dbRequirements override settings from defaultDbRequirements - you can create app\components\Migration and set this requirements in application level, then you don't need repeat this on every comment() call.

[SamMousa]

I think that's overkill. On Jul 5, 2016 2:13 PM, "Robert Korulczyk" notifications@github.com wrote:

We can reverse this solution and create param like notRequiredDbFeatures which will be empty by default and you need insert comments into it to ignore unsupported comments exceptions.

Or something more sophisticated:

protected $defaultDbRequirements = [ 'comments' => true, 'transactions' => true, ...];public $dbRequirements = [ 'comments' => false,];

dbRequirements override settings from defaultDbRequirements - you can create app\components\Migration and set this requirements in application level, then you don't need repeat this on every comment() call.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yiisoft/yii2/issues/11841#issuecomment-230461284, or mute the thread https://github.com/notifications/unsubscribe/AAhYzZKnzzzP8zsE1Ep0fMOS6suRqqJvks5qSkpMgaJpZM4I_7d3 .

[SamMousa]

How about instead adding another function. That might even be better practice than the extra argument; it also "scales" better to other things that might be optional.

For example:

$this->string(5)->optionalComment('cool stuff');

This also means it will not be breaking backwards compatibility except in the rarest of cases.

[AnikanovD]

Definition of requirements is beautiful but not useful. How many requirements we can define? Maybe i'm too strict. But. If talk about comments, then developer should understand that it have only informational purpose. Any use in the logic, is already questionable. For this reason, we can give the possibility to complete migrate. Behaviour will be changed, but to better. Or just insert if ($this->db->driverName == 'sqlite') {throw new ...} to migration. I will be frustrated, if during a migration an error occurred, due to the small package I just added and it uses comments for documentation.

[SamMousa]

@AnikanovD What do you think about my proposal for using optionalComment as a separate function?

[AnikanovD]

@SamMousa Good. It does not affect current behaviour. If it also shows the warning, then I would like to know how.

[SamMousa]

@AnikanovD I don't have an implementation; I think your implementation is good, I just propose instead of altering behavior of current comment function (which was my original idea) and instead create a separate function that calls the original.

function optionalComment($comment) {
    try {
        $this->comment($comment);

    } catch (..) {
        echo "\n Comments not supported by backend.\n";
   }
}

[AnikanovD]

@SamMousa Initially I also thought, but when I looked out the code, I found difficulty. We can't output a warning when setting a comment in QueryBuilder, ColumnSchemaBuilder. So it is possible only in Migration.

[SamMousa]

@samdark Would it be okay without warning?

--> The warning isn't really required if the developer uses something like optionalComment right?

[samdark]

Yes.

[AnikanovD]

@SamMousa I think we discussed this issue in detail. It was interesting)

[SamMousa]

:) @AnikanovD do you want to create a new PR that implements optionalComment in:

• All SchemaColumnBuilder (sub)classes.
• All QueryBuilder (sub)classes. --> here it should be called something like addOptionalCommentOnColumn Please also add tests to the existing test classes.

If not, let me know and I'll make time for it! Thanks for the discussion!

[AnikanovD]

@SamMousa Don't understand about addOptionalCommentOnColumn

[SamMousa]

@AnikanovD The function in QueryBuilder is called addCommentOnColumn, so for the optional variant it woudl be called addOptionalCommentOnColumn!

[AnikanovD]

@SamMousa It's not necessary, because optionalComment() has no effect on ColumnSchemaBuilder::$comment.

[h311ion]

Why don't just add some database-specific behavior if it is only affecting SQLite DB? IMO it's much better than write database-specific migration code. If you decided to use optionalComment - you can just discard it completely, no?

[SamMousa]

@h311ion Because we don't want to change the current behavior, in case anyone depends on having the comments saved we would not want to silently "not save" them.

Adding optionalComment is basically adding database-specific behavior for SQLite; this allows users of the Yii framework to still write database-independent code (which is the point of the database abstraction layer).

[samdark]

Seems @AnikanovD misunderstood the issue. ColumnSchemaBuilder is fine as is https://github.com/yiisoft/yii2/commit/01c4dde2026bf106cce2c379bcc5d7f60b79dc5d.

The issue is with non-supported QueryBuilder methods which throw an exception.

• alterColumn
• addPrimaryKey
• dropPrimaryKey
• addCommentOnColumn
• addCommentOnTable
• dropCommentFromColumn
• dropCommentFromTable

If we want to solve comments part only, the following methods should be added:

• addCommentOnColumnIfSupported
• addCommentOnTableIfSupported
• dropCommentFromColumnIfSupported
• dropCommentFromTableIfSupported

I'm not so happy with it.

[SamMousa]

Why not?

On Aug 5, 2016 12:39 AM, "Alexander Makarov" notifications@github.com wrote:

Seems @AnikanovD https://github.com/AnikanovD misunderstood the issue. ColumnSchemaBuilder is fine as is 01c4dde https://github.com/yiisoft/yii2/commit/01c4dde2026bf106cce2c379bcc5d7f60b79dc5d .

The issue is with non-supported QueryBuilder methods which throw an exception.

• alterColumn
• addPrimaryKey
• dropPrimaryKey
• addCommentOnColumn
• addCommentOnTable
• dropCommentFromColumn
• dropCommentFromTable

If we want to solve comments part only, the following methods should be added:

• addCommentOnColumnIfSupported
• addCommentOnTableIfSupported
• dropCommentFromColumnIfSupported
• dropCommentFromTableIfSupported

I'm not so happy with it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yiisoft/yii2/issues/11841#issuecomment-237717818, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhYzerpeg1CkoJ8qOSxG9RiXxv60LMRks5qcng9gaJpZM4I_7d3 .

[samdark]

It's kinda OK to do but then we probably should add alike method for anything i.e. alterColumnIfSupported etc. All these extra methods may introduce extra confusion.

@yiisoft/core-developers what do you think?

[rob006]

@samdark What about https://github.com/yiisoft/yii2/issues/11841#issuecomment-230461284?

[SamMousa]

@samdark one difference with alterColumnIfSupported is that comments are in essence not functional parts of your application.

[samdark]

Yeah, non-essentiality makes sense. Let's add these methods for comments only.

[SamMousa]

@AnikanovD will you make a new implementation / update your PR? If not let me know and I will make one (could be ~10 days before I have time).

[AnikanovD]

@SamMousa okay, i will try.

[mgrechanik]

Just encountered this problem with Yii 2.0.20. It looks weird that migration fails because of the field comments, who are mostly for decoration there