Fixing parser errors when adding columns with SET type

[iifawzi]

Hi, This PR should fix #168 and the issues shown #337

I will clarify the changes in a review.

Testing in PMA too.

[codecov]

Codecov Report

Merging #344 (88d7f3e) into QA (09fefbf) will decrease coverage by 0.02%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##                 QA     #344      +/-   ##
============================================
- Coverage     99.80%   99.78%   -0.03%     
- Complexity     1911     1914       +3     
============================================
  Files            63       63              
  Lines          4596     4605       +9     
============================================
+ Hits           4587     4595       +8     
- Misses            9       10       +1     

Impacted Files	Coverage Δ
src/Components/AlterOperation.php	98.97% <90.00%> (-1.03%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09fefbf...88d7f3e. Read the comment docs.

[iifawzi]

That's what I've noticed, I've also added some comments in the code, to make things clearer in the future

[williamdes]

💯 Oh yeah, this looks neat and readable
Codecov seems to complain about a non covered line for some reason

[williamdes]

While trying to create some particular query I found this one made by phpMyAdmin on table charset change

ALTER TABLE `d` DEFAULT CHARSET=hp8 COLLATE hp8_english_ci;

Errors:

#1: Missing comma before start of a new alter operation. (near "DEFAULT CHARSET" at position 16)
#2: Unrecognized alter operation. (near "DEFAULT CHARSET" at position 16)

Keeping this to add a new test before merging

And another bunch of them

$ ./bin/lint-query --query 'ALTER TABLE `d` CHARSET=hp8'
#1: Unrecognized alter operation. (near "" at position 0)
$ ./bin/lint-query --query 'ALTER TABLE `d` CHARSET=hp8;'
#1: Unrecognized alter operation. (near ";" at position 27)
$ ./bin/lint-query --query 'ALTER TABLE tbl_name CONVERT TO CHARACTER SET charset_name;'
$ ./bin/lint-query --query 'ALTER TABLE `d` CHARACTER SET utf8;'
#1: Unrecognized alter operation. (near ";" at position 34)
$ ./bin/lint-query --query 'ALTER TABLE `d` CHARACTER SET utf8 COLLATE utf8_general_ci;'
#1: Unrecognized alter operation. (near ";" at position 58)

Would you mind if I add them into QA and then you rebase/squash ontop of it ?

[iifawzi]

Interesting, gonna figure out how.

Would you mind if I add them into QA and then you rebase/squash ontop of it ?

Sure, feel free to add them.

[williamdes]

Interesting, gonna figure out how.

Would you mind if I add them into QA and then you rebase/squash ontop of it ?

Sure, feel free to add them.

Done as 09fefbf

[iifawzi]

It seems like all new added tests aren't related to the changes we introduced in this PR.
They are all caused because of

sql-parser/src/Components/AlterOperation.php

Lines 311 to 313 in b18edd6

	if ($ret->options->isEmpty()) {
	$parser->error('Unrecognized alter operation.', $list->tokens[$list->idx]);
	}

looks like the fix should be somewhere in the OptionsArray::parse method because it return empty array. I will try to investigate more :')

sql-parser/src/Components/AlterOperation.php

Line 236 in b18edd6

$ret->options = OptionsArray::parse($parser, $list, $options);

Also, if i'm not mistaken, parseAlterTableCharacterSet4.in is passing with no errors - after this fix -

[williamdes]

🚢

[williamdes]

phpstan complains about this possible null value, added a check in 6ad0bb6

 ------ ------------------------------------------------------------------- 
  Line   src/Components/AlterOperation.php                                  
 ------ ------------------------------------------------------------------- 
  302    Cannot access property $value on PhpMyAdmin\SqlParser\Token|null.  
  305    Cannot access property $value on PhpMyAdmin\SqlParser\Token|null.  
 ------ ------------------------------------------------------------------- 

[williamdes]

All tools pass but phpcs is nuts (only on master branch): https://github.com/phpmyadmin/sql-parser/runs/3344576860#step:7:10

Fixed, it was ridiculous: cf9cb91

[williamdes]

All good, but unable to have the last line covered if ($token->value === 'CHARACTER SET') { searching

Fixed in fd1552e 🎉

[iifawzi]

Happy to see everything fixed! 🥳🥳❤️