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

MSSQL TOP Limit style support #116

Closed
wants to merge 5 commits into from
Closed

MSSQL TOP Limit style support #116

wants to merge 5 commits into from

Conversation

numkem
Copy link
Contributor

@numkem numkem commented Apr 12, 2013

As requested, this is a manual merge of the code that was in pull request #37 with support of multiple connections and forcing the use of the TOP style limit for the dblib driver.

It is currently in use in it's current form in one my projects so I can attest that it's working.

…on/idiorm without tests.

- Added detection and configuration option for mssql style 'select top'
- Added mssql style top clauses
- Added use of limit top style for the dblib driver.
@treffynnon
Copy link
Collaborator

Thank you for the pull request, but there are still a number of things that are off in this code. Some old code has been added back in (WHERE_FRAGMENT, etc.) and some newer code has been removed (Firebird).

Also the indentation of your code is not quite right. Please have tabs expand to four spaces.

@treffynnon
Copy link
Collaborator

Oh and we need some tests for this new feature please. 😄

@numkem
Copy link
Contributor Author

numkem commented Apr 15, 2013

This should fix the issues you saw in the previous commit. I added some tests the same way it was done with the other ones but just for MSSQL since most of the test requires a working LIMIT.

@numkem
Copy link
Contributor Author

numkem commented Apr 15, 2013

Still working on it... Somehow the tests exploded after double checking after the push... Sorry about that.

@numkem
Copy link
Contributor Author

numkem commented Apr 15, 2013

I'm running into the issue that if I put my new tests with the others it seems like the fake PDO that is created during setUp() is shared accross all tests.

Have you ever run into that issue?

Sebastien Bariteau added 2 commits April 15, 2013 16:19
Created 2 new static methods reset_config() and reset_db(). They are
used in tests to make sure to reset what the static methods uses in
between tests.
@numkem
Copy link
Contributor Author

numkem commented Apr 15, 2013

Ok I found my problem. It's caused because the tearDown() method in tests were not actually resetting everything.

The _config arrays used by the static methods were not being reset so changes done in the first test would carry over the others and since they are share the same connection name, things would go bad.

As for _db putting a null as a PDO object would cause some issues since the LIMIT and character quoting methods try to use the set PDO object to get the driver name causing an error.

I've added 2 static methods named reset_config() and reset_db() that set the 2 arrays to an empty array making the transition between tests correct.

Thanks!

@treffynnon
Copy link
Collaborator

OK so I have completed a manual merge of this feature into a new feature branch of Idiorm. You'll notice that I made a number of changes to your code in the process. See: https://github.com/j4mie/idiorm/tree/feature.mssql_limit

Please could you try the code for me and let me know how it goes? If it works for you then I will go ahead and merge it into develop.

As a side note I have removed your code in the protected static function _detect_identifier_quote_character($connection_name) { method as this should be setup via configuration as discussed in issue #114 and the SQL Server 2000 manual: http://msdn.microsoft.com/en-us/library/aa224033(v=sql.80).aspx

Quoted identifiers are valid only when the QUOTED_IDENTIFIER option is set to ON. By default, the Microsoft OLE DB Provider for SQL Server and SQL Server ODBC driver set QUOTED_IDENTIFIER ON when they connect. DB-Library does not set QUOTED_IDENTIFIER ON by default. Regardless of the interface used, individual applications or users may change the setting at any time. SQL Server provides a number of ways to specify this option. For example, in SQL Server Enterprise Manager and SQL Query Analyzer, the option can be set in a dialog box. In Transact-SQL, the option can be set at various levels using SET QUOTED_IDENTIFIER, the quoted identifier option of sp_dboption, or the user options option of sp_configure.

@numkem
Copy link
Contributor Author

numkem commented Apr 16, 2013

Thank you for the merge!

I did that change to _detect_identifier_quote_character because dblib will always use FreeTDS and can only talk to Sybase/MS SQL. The change makes the "auto detection" works in the way that I wont break on first use. The real way to do the quotations would be to put them into [brackets]. I understand that making it works the way I did could be deciving to the user who would think that the quotation is done the right way and using reserved names would be safe.

#114 was when using odbc and since dblib doesn't, I figured it would be nice to have it working out of the box.

I tested with idiorm the way it is and with the configuration for the quotes character and everything works great!

If we want dblib to have the same behavior as odbc regarding quotes (needing the config). We probably should put it in the documentation or add the information to #114.

Thanks!

@treffynnon
Copy link
Collaborator

Could you share the configuration you have setup to get this working? I can
then use that to add the information to the manual. Would rather have
accurate information in there than my guesses! :)

@numkem
Copy link
Contributor Author

numkem commented Apr 16, 2013

Of course!

First of all you need to have the dblib driver. Some distributions have it prepackaged but you might have to get it from sources here. You will also need to install FreeTDS from your distro.

Once you have dblib loaded and it shows up in php -m, you can configure idiorm.

It's pretty straightforward, its just like mysql but the driver name is different and you need to set the quote character to an empty string.

ORM::configure('dblib:host=sqlhost;dbname=database_name');
ORM::configure('username', 'username');
ORM::configure('password', 'password');
ORM::configure('identifier_quote_character', '');

This of course works with multiple connection. That's how I'm currently using it.

@treffynnon
Copy link
Collaborator

Thank you for the extra information. Just wanted to let you know this will be merged into develop soon. I am a little busy at the moment, but it's not forgotten! 😄

@ghost ghost assigned treffynnon May 13, 2013
@samsong
Copy link

samsong commented Jun 5, 2013

Why not automatically set identifier_quote_character for mssql / dblib?

@samsong
Copy link

samsong commented Jun 5, 2013

Suggest changing _detect_identifier_quote_character statement,

switch case mssql returns " for the quote character.
should make it empty string.

@treffynnon
Copy link
Collaborator

@samsong The correct identifier quote character is " so that will not be changed.

See my notes from a comment above for how to correctly configure your DB library:

...this should be setup via configuration as discussed in issue #114 and the SQL Server 2000 manual: http://msdn.microsoft.com/en-us/library/aa224033(v=sql.80).aspx

Quoted identifiers are valid only when the QUOTED_IDENTIFIER option is set to ON. By default, the Microsoft OLE DB Provider for SQL Server and SQL Server ODBC driver set QUOTED_IDENTIFIER ON when they connect. DB-Library does not set QUOTED_IDENTIFIER ON by default. Regardless of the interface used, individual applications or users may change the setting at any time. SQL Server provides a number of ways to specify this option. For example, in SQL Server Enterprise Manager and SQL Query Analyzer, the option can be set in a dialog box. In Transact-SQL, the option can be set at various levels using SET QUOTED_IDENTIFIER, the quoted identifier option of sp_dboption, or the user options option of sp_configure.

@treffynnon
Copy link
Collaborator

I have merged these changes and added documentation in commit: baff6ed

@treffynnon treffynnon closed this Aug 28, 2013
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.

3 participants