Fix way end of functions, procedures and triggers' bodies is identified

[Tithugues]

@williamdes reported that

sql-parser/tests/Parser/ParserLongExportsTest.php

Line 59 in 4bfd15c

public function testParsephpMyAdminDump(): void

was incorrect.
Indeed, in this file https://github.com/phpmyadmin/sql-parser/blob/4bfd15c40c6e7cec3592095cd405f52931af1b10/tests/data/parser/parsephpMyAdminExport1.in, the variables defined in the last comments where not reported (as we can see in the unit test).

This is due to the fact that when parsing functions, procedures and triggers, the sql-parser includes everything in the body of the statements.

With the current fix, this is not the case anymore. This has an impact on existing data test sets which seemed not to be valid.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d576334) 97.04% compared to head (599c597) 97.04%.

Additional details and impacted files

@@            Coverage Diff            @@
##              5.8.x     #438   +/-   ##
=========================================
  Coverage     97.04%   97.04%           
- Complexity     2216     2218    +2     
=========================================
  Files            69       69           
  Lines          5103     5107    +4     
=========================================
+ Hits           4952     4956    +4     
  Misses          151      151           

Impacted Files	Coverage Δ
src/Statements/CreateStatement.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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[niconoe-]

@williamdes Code coverage is yelling but at unchanged files, like it did a few weeks ago with some PR of mine. I think therefore we shouldn't consider those "failures" and this PR is OK and could be merged, wdyt?

Thanks 🙂

[williamdes]

@williamdes Code coverage is yelling but at unchanged files, like it did a few weeks ago with some PR of mine. I think therefore we shouldn't consider those "failures" and this PR is OK and could be merged, wdyt?

Thanks 🙂

Hi
Sorry for not replying earlier
I reviewed the contents and was concerned that the DELIMITER would maybe mess up with how we use the parser to read procedures at phpMyAdmin
So I need to test that or someone could test?
Bu it's okay for me, I think I can merge this in the following days
And maybe release the version?
We could do a 5.8.0-rc1 maybe
So we could push more stuff to 5.8 before the next phpMyAdmin release?

PS: that's okay for codecov

[Tithugues]

@williamdes Code coverage is yelling but at unchanged files, like it did a few weeks ago with some PR of mine. I think therefore we shouldn't consider those "failures" and this PR is OK and could be merged, wdyt?
Thanks slightly_smiling_face

Hi Sorry for not replying earlier I reviewed the contents and was concerned that the DELIMITER would maybe mess up with how we use the parser to read procedures at phpMyAdmin So I need to test that or someone could test? Bu it's okay for me, I think I can merge this in the following days And maybe release the version? We could do a 5.8.0-rc1 maybe So we could push more stuff to 5.8 before the next phpMyAdmin release?

PS: that's okay for codecov

Hi,

Thanks for the answer.

I think I can provide an pretty clear example about what's happening with triggers, procedures and functions.
Let's go with the file tests/data/parser/parsephpMyAdminExport1.in. As you can see, there are many statements. I'd like to focus on the first trigger…
Add the next 2 lines at the very end of the \PhpMyAdmin\SqlParser\Parser::parse function:

        $body = $this->statements[2]->statements[6]->body;
        die(var_dump(implode(' ', array_column($body, 'token'))));

Now, let's parse the .in file, using the bin/lint-query.
Here is the result:

INSERT   INTO   monitoring__times_mirror 
 ( `idServer` ,   `time` ,   `totalTime` )   VALUES ( new . idServer ,   new . time ,   new . totalTime ) 
 $$ 
 DELIMITER   ; 
 DELIMITER   $$ 
 CREATE   TRIGGER   `deleteTimes`   AFTER   DELETE   ON   `monitoring__times`   FOR EACH ROW   DELETE   FROM   monitoring__times_mirror 
 WHERE   `idServer` = old . idServer 
 AND   `time` = old . time 
 AND   `totalTime` = old . totalTime 
 $$ 
 DELIMITER   ; 
 DELIMITER   $$ 
 CREATE   TRIGGER   `updateTimes`   AFTER   UPDATE   ON   `monitoring__times`   FOR EACH ROW   UPDATE   monitoring__times_mirror 
 SET   `idServer` = new . idServer , 
 `time` = new . time , 
 `totalTime` = new . totalTime 
 WHERE   `idServer` = old . idServer 
 AND   `time` = old . time 
 AND   `totalTime` = old . totalTime 
 $$ 
 DELIMITER   ; 

 --
 
 -- Index pour les tables déchargées 
 --
 

 --
 
 -- Index pour la table `monitoring__times` 
 --
 
 ALTER   TABLE   `monitoring__times` 
   ADD   UNIQUE KEY   `idServer`   ( `idServer` , `time` )   USING   BTREE   COMMENT   'Unique idServer/time' , 
   ADD   KEY   `INDEX_totalTime`   ( `totalTime` )   USING   BTREE   COMMENT   'Index for totalTime column' ; 

 --
 
 -- Contraintes pour les tables déchargées 
 --
 

 --
 
 -- Contraintes pour la table `monitoring__times` 
 --
 
 ALTER   TABLE   `monitoring__times` 
   ADD   CONSTRAINT   `monitoring__times__idServer`   FOREIGN KEY   ( `idServer` )   REFERENCES   `monitoring__servers`   ( `id` ) ; 
 COMMIT ; 

 /*!40101   SET   CHARACTER_SET_CLIENT = @OLD_CHARACTER_SET_CLIENT   */ ; 
 /*!40101   SET   CHARACTER_SET_RESULTS = @OLD_CHARACTER_SET_RESULTS   */ ; 
 /*!40101   SET   COLLATION_CONNECTION = @OLD_COLLATION_CONNECTION   */ ; 

As you can see, everything after the beginning of the trigger (next statements, comments, …) are considered as being part of the first trigger…

Let's do the exact same test but on the fix branch now. The result is:

INSERT   INTO   monitoring__times_mirror 
 ( `idServer` ,   `time` ,   `totalTime` )   VALUES ( new . idServer ,   new . time ,   new . totalTime )

Please let me know if you need any additional information.

[williamdes]

Thank you for all your contributions @Tithugues !

[williamdes]

I reviewed the contents and was concerned that the DELIMITER would maybe mess up with how we use the parser to read procedures at phpMyAdmin

Oh yeah I was right, phpMyAdmin needs fixing after this PR

If you view a complex procedure that has a ; the procedure ends at the end of the line
In edit procedures/functions view.

The fixes:
phpmyadmin/phpmyadmin@a88c9a3
and
phpmyadmin/phpmyadmin@99881de
Using test data: a3a699d