Feature ansi mode, partial solution to #284

[smarek]

This is very trivial, and I have no idea, what might have been broken by these changes, but phpunit does not complain about anything, so you definitely haven't written enough tests 😆 /s

It allows you to retrieve current identifiers quoting char from Context::getIdentifierQuote() based on current SQL Mode set and seems to work well with CLI tools such as lint and highlight-query

There are still few places, where the backtick is hard-coded, and I'm not sure if I should update these pieces of code with new context-aware solution, but that I probably leave to someone who knows this code-base better.

I've tested this with sample SQL query from: phpmyadmin/phpmyadmin#15821 (comment)

Fixes: #284

[williamdes]

Thank you for the PR @smarek
I will maybe write tests and then rebase your PR onto the new tests to see them fail ;)

[codecov]

Codecov Report

Merging #294 into QA will decrease coverage by 0.32%.
The diff coverage is 84%.

@@            Coverage Diff             @@
##               QA     #294      +/-   ##
==========================================
- Coverage     100%   99.67%   -0.33%     
- Complexity   1874     1883       +9     
==========================================
  Files          63       63              
  Lines        4540     4557      +17     
==========================================
+ Hits         4540     4542       +2     
- Misses          0       15      +15

[smarek]

@williamdes thank you, also because I'm dumb, could you put up some guide, how to create new test? I can certainly provide SQL input data (such as tests/data/lexer/lexAnsi.in), but I don't know how to generate the tests/data/lexer/lexAnsi.out without writing custom code, or is that how things should be done around here?

[williamdes]

Maybe you could be searching for https://github.com/phpmyadmin/sql-parser/blob/master/CONTRIBUTING.md#testsuite ?

[smarek]

geez, yes, thank you very much

[smarek]

Ok, I'm still not sure what I'm doing, but i've added ANSI_QUOTES generator and tests logic, and single case for lexer and parser (same test data)
This might be good for starting up the testcases needed to merge this PR

[williamdes]

The patch looks good to me
Can you rebase all the commits onto QA branch or do you want me to do it ?

[smarek]

Please do the rebase and updates you mentioned in code review, it will be faster and easier for both of us

[williamdes]

It should be okay now
I had to close/re-open to trigger TravisCI

[williamdes]

Thank you @smarek for your contribution !

[smarek]

Cheers, however did you test it ? I mean standalone parsing / re-assembling seemed to work for me, but there were more places in code that I did not touch, is merging this complete solution ?

[williamdes]

Cheers, however did you test it ? I mean standalone parsing / re-assembling seemed to work for me, but there were more places in code that I did not touch, is merging this complete solution ?

I did not make a real-world test but the code and tests looked good to me
And you tested it :)
And the CLI option you added convinced me everything works fine as expected

[smarek]

Ok, do you want me to provide test environment? I can and i gladly will

But me fixing the tests to work, does not mean it works and is complete solution

As i wrote in phpmyadmin/phpmyadmin#15821 (comment) there is a great number of issues with phpmyadmin (suddenly) when using this patched version against ANSI sql_mode DB-server, and I'd expect phpmyadmin to be kind-of agnostic to sql_mode given that this should handle sql-parser library, but hey, I expect you to at least test the changes, once we get to fix the phpmyadmin in ANSI mode, am I right?

[williamdes]

Ok, do you want me to provide test environment? I can and i gladly will

But me fixing the tests to work, does not mean it works and is complete solution

As i wrote in phpmyadmin/phpmyadmin#15821 (comment) there is a great number of issues with phpmyadmin (suddenly) when using this patched version against ANSI sql_mode DB-server, and I'd expect phpmyadmin to be kind-of agnostic to sql_mode given that this should handle sql-parser library, but hey, I expect you to at least test the changes, once we get to fix the phpmyadmin in ANSI mode, am I right?

Yes of course, go ahead ;)
I will do more testing on my end and will review your new PR

[williamdes]

It broke https://travis-ci.org/github/williamdes/phpmyadmintest/jobs/634895744#L368

[williamdes]

                // Using appropriate quotes.
                if (($compat === 'MSSQL') || ($sql_backquotes === '"')) {
                    Context::$MODE |= Context::SQL_MODE_ANSI_QUOTES;
                }

On the main repo

[williamdes]

diff --git a/src/Context.php b/src/Context.php
index 8792117d..0124d88d 100644
--- a/src/Context.php
+++ b/src/Context.php
@@ -417,7 +417,7 @@ abstract class Context
         }
         if ($str[0] === '@') {
             return Token::FLAG_SYMBOL_VARIABLE;
-        } elseif ($str[0] === self::getIdentifierQuote()) {
+        } elseif ($str[0] === '`') {
             return Token::FLAG_SYMBOL_BACKTICK;
         } elseif ($str[0] === ':' || $str[0] === '?') {
             return Token::FLAG_SYMBOL_PARAMETER;
diff --git a/src/Lexer.php b/src/Lexer.php
index a56641e8..3cf0d187 100644
--- a/src/Lexer.php
+++ b/src/Lexer.php
@@ -935,7 +935,7 @@ class Lexer extends Core
         $str = null;
 
         if ($this->last < $this->len) {
-            if (($str = $this->parseString(Context::getIdentifierQuote())) === null) {
+            if (($str = $this->parseString('`')) === null) {
                 if (($str = $this->parseUnknown()) === null) {
                     $this->error(
                         'Variable name was expected.',

Fixes the test

[williamdes]

@ibennetch

[williamdes]

https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_ansi_quotes

Treat " as an identifier quote character (like the ` quote character) and not as a string quote character. You can still use ` to quote identifiers with this mode enabled. With ANSI_QUOTES enabled, you cannot use double quotation marks to quote literal strings because they are interpreted as identifiers.

So both are now identifiers

[williamdes]

@smarek I had to revert part of your work because Lexer::parseString was to complicated to fix with a small amount of time
I hope to see ANSI support back into the parser soon
fc3917d

[smarek]

@williamdes how's it going with re-reverting the changes?

[williamdes]

@williamdes how's it going with re-reverting the changes?

It was reverted partially, would you agree to work on implementing another time the reverted part of the feature?