Incorrect parse of SQL file with double semicolon

[jaroslavtyc]

Symfony messenger generated following SQL migration for PostgreSQL

CREATE
OR REPLACE FUNCTION notify_messenger_messages() RETURNS TRIGGER AS
$$
BEGIN
    PERFORM
pg_notify('messenger_messages', NEW.queue_name::text);
RETURN NEW;
END;
$$
LANGUAGE plpgsql;;
DROP TRIGGER IF EXISTS notify_trigger ON messenger_messages;;
CREATE TRIGGER notify_trigger
    AFTER INSERT OR
UPDATE
    ON messenger_messages
    FOR EACH ROW
    EXECUTE PROCEDURE notify_messenger_messages();;

\Nextras\Migrations\Drivers\BaseDriver::loadFile parse it incorrectly considering as a valid delimiter only single ;, which results into empty SQL command

$q = substr($content, $queryOffset, $queryLength); // $q = ''

[JanTvrdik]

I don't quite understand where the extra semicolons are coming from. I see a single semicolon in Symfony source code

[hrach]

Could we test & optionally fix this & integrate through https://github.com/nextras/multi-query-parser/?

[jaroslavtyc]

I don't quite understand where the extra semicolons are coming from. I see a single semicolon in Symfony source code

Good question.

I thought that it is some Postgres specific weirdness in a function definition, but it works with a single semicolon too.

...

I have found the reason - it comes from Sylis migration src/Sylius/Bundle/CoreBundle/Migrations/Version20230419092354.php where is a part with semicolons already present

$this->addSql('CREATE OR REPLACE FUNCTION notify_messenger_messages() RETURNS TRIGGER AS $$
            BEGIN
                PERFORM pg_notify(\'messenger_messages\', NEW.queue_name::text);
                RETURN NEW;
            END;
        $$ LANGUAGE plpgsql;');
        $this->addSql('DROP TRIGGER IF EXISTS notify_trigger ON messenger_messages;');
        $this->addSql('CREATE TRIGGER notify_trigger AFTER INSERT OR UPDATE ON messenger_messages FOR EACH ROW EXECUTE PROCEDURE notify_messenger_messages();');

and that is doubled by Doctrine itself in \Doctrine\Migrations\Generator\ConcatenationFileBuilder::buildMigrationFile

$string .= $query->getStatement() . ";\n";

Conclusion

• It is a problem of non-defensive logic in Doctrine, I will create an issue there :rocket:
• :boom: It is still an issue for nextras/migrations too because query with double semicolon is absolute valid (even if weird) and it should work
  • my proposal is to simply send all semicolons to the engine to let it decide if it is valid valid in PostgreSQL:

# SELECT 1 AS semicolon_madness;;;
 semicolon_madness 
-------------------
                 1
(1 row)

[jaroslavtyc]

Could we test & optionally fix this & integrate through https://github.com/nextras/multi-query-parser/?

Note that nextras/multi-query-parser currently fails to parse it too (version v1.0.0, cbec8dcd4904b999b9d138a70776df87ee5ce962)

[JanTvrdik]

It is a problem of non-defensive logic in Doctrine, I will create an issue there

I would say the main issue is that the Sylius migration is just wrong. It should not contain the terminating semicolon.

double semicolon is absolute valid

Double semicolon has no meaning in postgre. I guess we can ignore the empty statement.

[jaroslavtyc]

I would say the main issue is that the Sylius migration is just wrong. It should not contain the terminating semicolon.

In this part I disagree. Standard semicolon at the end of a query should be simply silently removed by a PHP library, not causing crashes.

I guess we can ignore the empty statement.

Yup, that sounds like a best clean solution :clap: