Unable to execute query for trigger

[tunder]

After updating from version 0.1.2 to latest dev-master executing queries like this

$this->execute("
    CREATE TRIGGER `name` BEFORE UPDATE ON `table`
        statement 1 goes here;
        statement 2 goes here;
    END;;
");

does not work anymore.

Cause:
Ruckusing_Migration_Base::split_query
This feature is useless. Having multiple sql queries inside the same string. Writing migrations is the developer job and we know that (at least) mysql doesn't support it. Why should you? This will be the source of many bugs.

Some examples where this function will fail:
triggers, stored procedures, etc.
query like: insert into table set field = 'string \'quote1; quote2\'';

[ruckus]

You are right, it has proven to be a source of bugs and confusion. I can no longer remember why it was included in the first place. I vaguely recall the use-case being that one should be able to slurp in a SQL file of commands and execute them via Ruckusing.

But I think at this point its causing more harm than good.

Anyone else want to weigh in?

[silverslice]

Performing multiple queries may be usefull to load initial dump from PhpMyadmin for example.

As for me, this feature should be implemented in special method like multiExecute with native adapter method (multi_query for mysqli), if it is possible.

[tunder]

This multiExecute may be ok if all the adapters support it.
In my opinion ruckus is a cli tool and so is mysql command. For initial import I am using mysql client command line tool then ./vendor/bin/ruckus.php db:migrate wrapped up in a deploy script (php, bash, bat, etc).
Maybe programmers are getting too lazy and want their tools to do everything for them. Just saying ;)
Anyways, method Ruckusing_Migration_Base::split_query is an example of bad coding (logic and performance wise - iterate every char in the statements) copied from stack overflow (even have the link in it's phpdoc) and it should be removed as soon as possible.

[kevcam4891]

I agree with this sentiment, but I don't really have a horse in this
race...not to big a user of triggers.

Ionut DINU mailto:notifications@github.com
March 6, 2015 at 7:55 AM

This |multiExecute| may be ok if all the adapters support it.
In my opinion ruckus is a cli tool and so is mysql command. For
initial import I am using mysql client command line tool then
./vendor/bin/ruckus.php db:migrate wrapped up in a deploy script (php,
bash, bat, etc).
Maybe programmers are getting too lazy and want their tools to do
everything for them. Just saying ;)
Anyways, method Ruckusing_Migration_Base::split_query is an example of
bad coding (logic and performance wise - iterate every char in the
statements) copied from stack overflow (even have the link in it's
phpdoc) and it should be removed as soon as possible.

—
Reply to this email directly or view it on GitHub
#154 (comment).

Ionut DINU mailto:notifications@github.com
March 5, 2015 at 3:15 PM

After updating from version 0.1.2 to latest dev-master executing
queries like this

$this->execute("
CREATE TRIGGER name BEFORE UPDATE ON table
trigger body here
END;;
");

does not work anymore.

It might be related to the Ruckusing_Migration_Base::split_query method.

—
Reply to this email directly or view it on GitHub
#154.

[tunder]

Any idea when can this be fixed? I'd like to update my composer library to the latest version as it have a lot of improvements and bug fixed.

[ruckus]

Hi @tunder - would this PR help?

#156

[tunder]

hi. it is a start but...
function execute from lib/Ruckusing/Migration/Base.php should use query as it returns boolean or array of rows in case sql is select or show. the change done by @silverslice is using multi_query in all cases. i'm not sure if this is not overhead also (using multi when you don't need it)
we should have executeMulti as separate method and allow the dev to pick which one to use.

[silverslice]

@tunder,

1. Both postgresql and sqlite extensions support multiple queries without special methods. The same is true for PDO, I'm not sure that it is overhead.
2. According to documentation "the execute() method is intended for queries which do not return any data, e.g. INSERT, UPDATE or DELETE", so if you don't use undocumented feature, this patch doesn't break your code.
3. I'm not against execute_multi method, but removing this feature from execute method breaks backwards compatibility. If ruckusing follows semantic versioning specification, we need to increase major version. If @ruckus is ready to start 2.* branch, I can move multiple queries feature into execute_multi method.

[tunder]

@silverslice,

1. for running one query using multi there is an overhead, even if it is small, but i'm ok with it because the migrations are run only once on deploy.
2. i'm not using any undocumented feature, but others might use it and their code will break. one thing to consider is adding a test for getting last insert id after using one (or more) insert query with multi_query. i'm not using this but others might.
3. i'm a strong believer in keeping backwards compatibility, but not in this case. i see no point in keeping it for a broken feature that never worked as intended. this will not affect me in any way so i'm ok with whatever versioning notation @ruckus wants to consider or whether or not he wants to keep backwards compatibility for this one.

anyways, nice job @silverslice. thank you for taking from your time to fix this. i was too busy to help. next time maybe.

@ruckus,
all in all, this patch is infinitely better than what we have right now. so you have green light from me.

[ruckus]

Thanks everyone for your comments, its really appreciated. Thank you @silverslice for your PR.

It sounds like the best course of action is:

1. Merge in this PR
2. Not bother with a 2.x release. Maybe I am missing something here but this PR seems to not break backwards compatability, its still possible to feed multiple queries to execute() - this PR is all about handling them in an intelligent manner, specifically by offloading them to the underlying driver. Given there is no BC-breakage I dont see a reason to do a 2.x release.

If I a mistaken and there is BC-breakage then please chime in and I will do a 2.x release.

Sorry if I am being obtuse and the answer is staring me in the face.

Thank you again!