Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL Server migrations fixes and improvements #843

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

tsdogs
Copy link
Contributor

@tsdogs tsdogs commented Jan 18, 2017

Q A
Is bugfix? yes
New feature? yes
Breaks BC? don't think so
Fixed issues When doing migration up and down SQL server constraints tend to block operations on tables and columns. This should fix all the issues.

@@ -36,7 +36,7 @@ public function down()
if ($this->dbType == 'sqlsrv') {
$this->dropIndex('{{%user_unique_username}}', '{{%user}}');
}
$this->alterColumn('{{%user}}', 'username', $this->string(255)->null());
$this->alterColumn('{{%user}}', 'username', $this->string(255), ' NULL');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line is changed, and shouldn't it be . instead of ,?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some strange reasons SQL server does not accept the ->null(), it creates a strange SQL like
DEFAULT NULL NULL string which is not correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any news on this one? Should I change something for you to include?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsdogs you should change , to . in the line I was talking about, because tests won't pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmeroff ok should be fixed now. I tested all up and down on SQL Server 2012 and it works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno why but tests still fail? though it's complaining about token table and last_login fields!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, there was a very ugly bug in the check of the db type. (though for some strange reason in my test environment migration with mariadb worked without errors).

@tsdogs
Copy link
Contributor Author

tsdogs commented Feb 10, 2017

Have you had time to check this?

SamMousa and others added 26 commits May 10, 2017 14:16
Not all timezones have hour offset, so we can not just divide offset
Include username & url in welcome email
Fix for accented letters in italian translation
Allow configuration of different database connection for all models.
* Tests no longer use deprecated package. Updated shields in README.

* Fixed typos

* Dont install codeception globally in travis.

* Use /tmp for assets
* add ability to filter by permission in case yii2-rbac module is used

* return empty array when there is no authManager

* allow to use anything that implements ManagerInterface; show filter in grid when it is DbManager

* code style
enricodetoma and others added 2 commits June 14, 2017 16:48
…me() gives an error on PHP 5.6).

Fix for bug where only admin users can be impersonated (while it should be that only NON-admin users can be impersonated).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants