Skip to content

Conversation

@PVince81
Copy link
Contributor

This fixes alter table commands that didn't quote the old column name

Please review @bartv2 @DeepDiver1975 @bantu

Fixes #7415

I've tested this twice.

This fixes alter table commands that didn't quote the old column name
@PVince81
Copy link
Contributor Author

and @karlitschek

@PVince81
Copy link
Contributor Author

@felixboehm can you test migration with this PR with MSSQL ? With a bit of luck it can solve its migration issue as well.

@PVince81
Copy link
Contributor Author

@DeepDiver1975
Copy link
Member

can you test migration with this PR with MSSQL ? With a bit of luck it can solve its migration issue as well.

I'll test this on mssql ....

@karlitschek
Copy link
Contributor

Thanks guys

@DeepDiver1975
Copy link
Member

I'll test this on mssql ....

This will not help with the MSSQL issue reported by @felixboehm - I'm working on a fix for this

@PVince81
Copy link
Contributor Author

Just tested on Oracle, and Oracle has a different error which happens regardless of this PR.
I'll raise a separate ticket for the Oracle issue.

@ghost
Copy link

ghost commented Feb 26, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3372/

@PVince81
Copy link
Contributor Author

Here is the Oracle issue: #7435

I'll tick MSSQL and Oracle since we tested them

@PVince81
Copy link
Contributor Author

@bartv2 @DeepDiver1975 @karlitschek @bantu this important fix still needs to be reviewed.
Thanks.

@PVince81
Copy link
Contributor Author

or @icewind1991

@DeepDiver1975
Copy link
Member

@PVince81 I assume we can write a unit test for this.
All we need is a table with a columns key where we change the size - this should trigger the exact same error message from my understanding.

Let's see if I can build something ....

@DeepDiver1975
Copy link
Member

#7439 is failing - I'll merge the unit tests now in here ...

@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor Author

Nice PR juggling 😉
Curious to see the result.

@ghost
Copy link

ghost commented Feb 26, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3386/

@DeepDiver1975
Copy link
Member

Failing unit tests are unrelated

@DeepDiver1975
Copy link
Member

👍

@PVince81
Copy link
Contributor Author

This needs another reviewer, required for 6.0.2 RC2 later today, thanks

@DeepDiver1975
Copy link
Member

@butonic @icewind1991 @bantu @bartv2 review maybe?

@butonic
Copy link
Member

butonic commented Feb 27, 2014

Testing now

@butonic
Copy link
Member

butonic commented Feb 27, 2014

👍 tests worked locally. jenkins failed because of etag asserts

butonic added a commit that referenced this pull request Feb 27, 2014
[stable6] Also quote old column name during DB migration
@butonic butonic merged commit d93b5af into stable6 Feb 27, 2014
@PVince81 PVince81 deleted the stable6-quote-oldcolumname branch February 27, 2014 12:17
@PVince81
Copy link
Contributor Author

Will prepare the forward port to master on the other PR

@PVince81
Copy link
Contributor Author

Forward port is here: #7439

@lock lock bot locked as resolved and limited conversation to collaborators Aug 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants