Skip to content

Fix #478 - ALTER TABLE … MODIFY … ENUM('<reserved_keyword>') is being wrongly parsed #485

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

Merged
merged 5 commits into from
Jun 29, 2023

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Jun 21, 2023

Fixes: #478
Fixes: #234

@williamdes williamdes changed the title Fix 478 Fix #478 - ALTER TABLE … MODIFY … ENUM('<reserved_keyword>') is being wrongly parsed Jun 21, 2023
@niconoe-
Copy link
Contributor Author

@williamdes
Base branch is master. As this is a fix, do you want me to cherry-pick those commit into a new branch from 5.8.x?

@williamdes
Copy link
Member

@williamdes Base branch is master. As this is a fix, do you want me to cherry-pick those commit into a new branch from 5.8.x?

Nevermind, I will rebase this onto 5.x and do the merging stuff
For further non breaking changes please use 5.x branches ;)

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

That feels definitely better than adding an edge case

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 44.44% and project coverage change: -0.10 ⚠️

Comparison is base (88c8314) 97.02% compared to head (9838a1c) 96.93%.

Additional details and impacted files
@@             Coverage Diff              @@
##              5.8.x     #485      +/-   ##
============================================
- Coverage     97.02%   96.93%   -0.10%     
+ Complexity     2234     2233       -1     
============================================
  Files            69       69              
  Lines          5144     5150       +6     
============================================
+ Hits           4991     4992       +1     
- Misses          153      158       +5     
Impacted Files Coverage Δ
src/Components/ArrayObj.php 100.00% <ø> (ø)
src/Components/IndexHint.php 100.00% <ø> (ø)
src/Components/AlterOperation.php 96.68% <44.44%> (-3.32%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Hmm, now it's unknown tokens
will this make lint errors or is this just unsupported options or the alter statement?
Could the set or enum be included into an Expression object?

From what I understand this was already the behavior but it was just not that much explicit in the out files

@niconoe-
Copy link
Contributor Author

Hmm, now it's unknown tokens will this make lint errors or is this just unsupported options or the alter statement? Could the set or enum be included into an Expression object?

From what I understand this was already the behavior but it was just not that much explicit in the out files

Unfortunately, on the AlterStatement, there are no properties about the type of the column to alter in the current operation. As far as I know, we have :

  • options to define the type of operation (ADD, MODIFY, DROP, …)
  • field that contains basic field information, usually just the name
  • partitions irrelevant here
  • unknown for all tokens in the AlterOperation until the next operation or statement.

The expression representing the type of the column could be stored in the field property but that would have a huge impact, and even if I like the idea (as the parser will be closer to the actual statement), I'm not a fan of doing it in this PR, as it goes far beyond the problem this PR is trying to fix.

@niconoe-
Copy link
Contributor Author

Fixes also #234 (#478 was a kind of duplicate of #234 actually)

@williamdes williamdes linked an issue Jun 29, 2023 that may be closed by this pull request
@williamdes williamdes added this to the 5.9.0 milestone Jun 29, 2023
@williamdes williamdes self-assigned this Jun 29, 2023
@williamdes williamdes changed the base branch from master to 5.8.x June 29, 2023 09:55
@williamdes
Copy link
Member

williamdes commented Jun 29, 2023

Hi @niconoe-

I just noticed the company you work for is in your commit email, is it intended ?

I added 9838a1c to this PR

Can you check that I did not do a rebase mistake ?
Everything seems okay, let's merge ?

@niconoe-
Copy link
Contributor Author

I just noticed the company you work for is in your commit email, is it intended ?

😅 It's not, but that's OK, there's no trouble with that.

Everything's fine to me! Thanks a lot!

@williamdes
Copy link
Member

I just noticed the company you work for is in your commit email, is it intended ?

😅 It's not, but that's OK, there's no trouble with that.

Everything's fine to me! Thanks a lot!

Seems like that's since a long time on your contributions, feel free to change it on your git settings.
You can change this per repo ;)

Merging 🚀

@williamdes williamdes merged commit 274fc3a into phpmyadmin:5.8.x Jun 29, 2023
williamdes added a commit that referenced this pull request Jun 29, 2023
Pull-request: #485

Signed-off-by: William Desportes <williamdes@wdes.fr>
MauricioFauth added a commit to MauricioFauth/sql-parser that referenced this pull request Sep 18, 2023
- Fixes phpmyadmin#511
- Reverts phpmyadmin#485

Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
MauricioFauth added a commit to MauricioFauth/sql-parser that referenced this pull request Sep 19, 2023
- Fixes phpmyadmin#511
- Reverts phpmyadmin#485

Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
@niconoe- niconoe- deleted the fix-478 branch August 6, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants