Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Adding a fix for #267. #268

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

Adding a fix for #267. #268

wants to merge 7 commits into from

Conversation

rarog
Copy link

@rarog rarog commented Sep 27, 2017

In #267 I described the incorrect behaviour, when generating DDLs with characters in name that are used for escaping on different plattforms. This is an attempt to fix it.
I tested the behaviour and it corrects the script generation with the base (PostgreSQL, SQLite) and MySQL escaping behaviour.

MSSQL seems to behave like standard despite having separate escaping, so I didn't touch it. I was able to test with driver Pdo_SqlServer, perhaps it uses the escaping with brackets, if Sqlsrv driver is used, but I couldn't test it, because I don't have the corresponding extension installed. I get the RuntimeException with message The Sqlsrv extension is required for this adapter but the extension is not loaded. @alextech Can you test the example in #267 and add the [] brackets to every named element? If it uses the brackets for escaping and is wrong, is it fixed, if you change in Zend\Db\Adapter\Platform\SqlSserver $quoteIdentifierTo to

    protected $quoteIdentifierTo = '[]';

At least from my tests SQL Management Studio escapes the opening bracket in names by adding the closing bracket to it, when I generate a CREATE TABLE script.

Copy link
Contributor

@mwillbanks mwillbanks left a comment

Choose a reason for hiding this comment

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

Please see my comment on the abstract, then also you're missing unit tests for these changes, lastly, you absolutely need to include links to the relevant portions of the specification documents explaining the preferred method of quoting.

@@ -19,7 +19,7 @@
/**
* @var string
*/
protected $quoteIdentifierTo = '\'';
protected $quoteIdentifierTo = '""';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you change the default, this is absolutely a MASSIVE BC BREAK. I advise you to please no do that especially considering this could have been extended.

Copy link
Author

@rarog rarog Oct 20, 2017

Choose a reason for hiding this comment

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

I could leave it as it is, but in that case I'd comment it, that it's only for BC reasons and contradicts the ANSI SQL standard.
I can't name any SQL implementation that uses single quotes for escaping if identifiers. The single quote is for strings.

I attach a wall of text aka TL;DR:

In chapter 5.2 <token> and <separator> of
Information Technology - Database Language SQL it's defined as

         <delimited identifier> ::=
              <double quote> <delimited identifier body> <double quote>

         <delimited identifier body> ::= <delimited identifier part>...

         <delimited identifier part> ::=
                <nondoublequote character>
              | <doublequote symbol>

         <nondoublequote character> ::= !! See the Syntax Rules

         <doublequote symbol> ::= <double quote><double quote>

         <delimiter token> ::=
                <character string literal>
              | <date string>
              | <time string>
              | <timestamp string>
              | <interval string>
              | <delimited identifier>

and same is recommended by Apache docs:

Ordinary identifiers are identifiers not surrounded by double quotation marks. Delimited identifiers are identifiers surrounded by double quotation marks.

By standard DB/2, Oracle, PostgreSQL and SQLite use double quote for identifier escaping. Sybase and MS SQLServer default to quoting via brackets [] but for example SQLServer does support the double quote when SET QUOTED_IDENTIFIER is ON, and it is on, in ANSI mode:

When SET ANSI_DEFAULTS is ON, SET QUOTED_IDENTIFIER is enabled.

MySQL/MariaDB understand by default the backtick, but also do support double quote in ANSI mode:
SET sql_mode = 'ANSI'; enables it for the running session.

@rarog
Copy link
Author

rarog commented Oct 20, 2017

I'll work on unit test next week. Thx for the first review!

@ezimuel ezimuel added this to the 3.0.0 milestone Nov 28, 2017
@rarog
Copy link
Author

rarog commented Apr 28, 2018

@mwillbanks I finally found time to add the unit tests. Are they ok in that form?

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#65.

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.

5 participants