-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor SQL Server keyword dictionaries and MsSQL leftovers #272
Conversation
Hello, thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link: |
I forgot to mention that this PR also sets the new SQLServer2012Platform as default for the driver. |
changing the default platform is a BC break, isn't it ? |
*/ | ||
class MsSQLKeywords extends KeywordList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should keep the existing class for BC (but you can make it extend the new SQLServerKeywords)
Might be, but in fact the 2012 platform just adds functionality and doesn't change or remove anything compared to 2008. So anyone who relied on 2008 until now should still be fine. |
Readded MsSQLKeywords extending SQLServerKeywords and marked it as deprecated. Also added new SQL Server keyword classes to ReserveWordsCommand. |
@deeky666 as far as I know SQL Server 2012 supports OFFSET and FETCH(LIMIT in normal db engines) in queries which is not supported by SQL Server 2008 thats why SQLServer2008Platform needs a messy logic to handle limit queries. Changing SQL Server 2012 to a default for a driver will be BC break, maybe not now cuz OFFSET FETCH is not implemented yet but I think it will be in the future. |
'NULLIF', | ||
'OF', | ||
'OFF', | ||
'OFFSETS', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be OFFSET
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! My bad, sorry ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind ;)
Nope, SQL Server 2012 supports |
So if my application depends on SQLServer2008 platform because its default now and because I'm using SQL Server 2008 R2 it will crash after update which changes default platform to SQLServer2012. Right? |
I don't think so because SQLServer2012 inherits the functionality of SQLServer2008 and doesn't change or remove any functionality of it. It only adds sequence support and some keywords. |
Of course if we add |
And dont you think that OFFSET FETCH support will be nice feature? Just check how limit query looks in SQL Server 2008 |
Yeah I have seen that. I'm about to implement the new functionality for SQL Server 2012. But then we definitely have to use SQLServer2008 as default platform for the driver for BC. |
@deeky666 It adds features, meaning the ORM can start using them as they are advocated as supported. But if you updated Doctrine in your app running SqlServer 2008, the new default will break your app. |
Ok I think got your point now. I will revert the default platform for BC. Then in general (just for understanding) what kind of update strategy does Doctrine use here concerning BC? Are BC breaks allowed in new minor releases like 2.4 or 2.5? |
no |
Refactor SQL Server keyword dictionaries and MsSQL leftovers
Each Microsoft SQL Server version has its own specific reserved keyword dictionary. Therefore each platform version should implement the correct dictionary. This PR adds dedicated dictionaries for Microsoft SQL Server 2000 (base), 2005, 2008 and 2012. Moreover all the "mssql" leftovers are refactored to fit the "sqlserver" term.