Skip to content

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Feb 8, 2023

Fix #418

I also improved the build process of Alter statements by adding a trim on it. Adding unit tests about the ALTER DEFINER=user EVENT builder gave me the unexpected results that an unwanted space can left at the end of the query, and already existing unit tests about the build of any Alter statement seemed to add that space in the expected value for no other reason that a lack of trimming here.

To see how the DEFINER=user is now correctly handle, you can compare the .out files from the 2 commits of the PR as the 1st one is the current status (unknown tokens and parsing errors are detected), while the 2nd one doesn't have any unknown token or parsing error.

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Base: 97.25% // Head: 97.25% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e9cf491) compared to base (b95fed9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #421   +/-   ##
=========================================
  Coverage     97.25%   97.25%           
  Complexity     2196     2196           
=========================================
  Files            68       68           
  Lines          5057     5059    +2     
=========================================
+ Hits           4918     4920    +2     
  Misses          139      139           
Impacted Files Coverage Δ
src/Statements/AlterStatement.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 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.

Maybe a more complex definer would also be a good test
With @ or w/without backquotes ?

@williamdes williamdes changed the title Fix 418 Fix #418 - ALTER EVENT statement with DEFINER=user modifier fails to be parsed Feb 8, 2023
@niconoe-
Copy link
Contributor Author

niconoe- commented Feb 8, 2023

Maybe a more complex definer would also be a good test With @ or w/without backquotes ?

I'm on it, but I discovered something I wasn't expecting 😆 . I named my file tests/data/parser/parseAlterEventWithComplexDefiners.in, and guess what? that name contains "lex" string so the .out file generated is via the lexer and not the parser !!

I'll rename my file and start again, but I really do think this should be fixed on the Tools/TestGenerator.php file.

@niconoe-
Copy link
Contributor Author

niconoe- commented Feb 8, 2023

Maybe a more complex definer would also be a good test With @ or w/without backquotes ?

That's done. You can compare the commit 0ccef14 for the .out file preparation without the DEFINER=… management, with the commit e9cf491 for the updated .out file with the DEFINER=… management.

@williamdes
Copy link
Member

file generated is via the lexer and not the parser !!

😆 😱
We need to fix this detection one day

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.

Awesome !

@williamdes williamdes added this to the 5.8.0 milestone Feb 8, 2023
@williamdes williamdes self-assigned this Feb 8, 2023
@williamdes williamdes merged commit 3f3debd into phpmyadmin:master Feb 26, 2023
williamdes added a commit that referenced this pull request Feb 26, 2023
Pull-request: #421

Signed-off-by: William Desportes <williamdes@wdes.fr>
@niconoe- niconoe- deleted the fix-418 branch February 27, 2023 14:51
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.

ALTER EVENT statement with DEFINER=user modifier fails to be parsed
3 participants