[5.5] fix a bug where model's delete method won't work if withCount is not empty.

[abdumu]

This fixes the bug that makes delete query receive two params instead of one if withCount array is not empty.

Before:

array:2 [▼
  0 => array:3 [▼
    "query" => "select `posts`.*, (select count(*) from `comments` where `posts`.`id` = `comments`.`commentable_id` and `comments`.`commentable_type` = ?) as `comments_count` f ▶"
    "bindings" => array:2 [▼
      0 => "App\Post"
      1 => "14"
    ]
    "time" => 6.3
  ]
  1 => array:3 [▼
    "query" => "delete from `posts` where `id` = ?"
    "bindings" => array:1 [▼
      0 => "App\Post"
      1 => "14"
    ]
    "time" => 0.84
  ]
]

After:

array:2 [▼
  0 => array:3 [▼
    "query" => "select `posts`.*, (select count(*) from `comments` where `posts`.`id` = `comments`.`commentable_id` and `comments`.`commentable_type` = ?) as `comments_count` f ▶"
    "bindings" => array:2 [▼
      0 => "App\Post"
      1 => "14"
    ]
    "time" => 6.3
  ]
  1 => array:3 [▼
    "query" => "delete from `posts` where `id` = ?"
    "bindings" => array:1 [▼
     0 => "14"
    ]
    "time" => 0.84
  ]
]

[Dylan-DPC-zz]

Can you write a test with this testcase ?

[abdumu]

I added a test.

This is the PR when withCount was introduced #19154

[taylorotwell]

Eh, I don't like adding this seemingly random parameter to newQueryWithoutScopes. @themsaid do you have a more elegant solution?

[themsaid]

@taylorotwell the new approach he took is much cleaner, it's the same approach we take while preparing the update query.

[taylorotwell]

How does this affect MySQL DELETE statements that use a join: https://dev.mysql.com/doc/refman/5.7/en/delete.html

See "Multi Table Deletes".

[abdumu]

Is it different than the test 'testDeleteWithJoinMethod' that is already written in DatabaseQueryBuilderTest.php ?

[themsaid]

@abdumu In the current code you remove the join bindings from the query, what happens if there's a delete query that uses joins? With this code it'll fail since the join bindings were removed.

[abdumu]

I didn't remove join bindings.

[themsaid]

Check https://github.com/laravel/framework/pull/22239/files#diff-bf9c2a600451a26e98eba7874fb841caR772

[abdumu]

Check https://github.com/laravel/framework/pull/22239/files#diff-bf9c2a600451a26e98eba7874fb841caR775

This is the same way you did it in prepareBindingsForUpdate, to make join bindings first.

[themsaid]

@abdumu yes and I believe it's wrong too, we'll need some integration tests similar to Illuminate\Tests\Integration\Database\EloquentWithCountTest to make sure these cases work.

• Update statement withCount that has joins.
• Delete statement withCount that has joins.

Can you do that?

[abdumu]

I just found out that Grammar.php/compileDelete query doesn't respect joins and Laravel send joins where bindings to it. That is not an issue if MySQL is the engine because MySqlGrammar.php/compileDelete respect joins.

I'm sorry I'm not sure I can test multiple engines with different approach. but I'll keep digging.

[taylorotwell]

Where are we with this? @themsaid?

[themsaid]

@taylorotwell I've tested it for MySQL and it works but I'm not sure about Postgres and sql server, they seems to have a unique syntax for delete statements with joins, MySQL needs the join bindings to be sorted first, postgres seems to require the where bindings first and then join bindings, and also seems that postgres doesn't remove select bindings.

tl;dr
The PR is unmergable without someone who can test it on Postgres and sql server, sorry I don't have large experience in either.

[themsaid]

A safe thing we could do is to return the exact input in Grammar:prepareBindingsForDelete and then in MySQL's Grammar:prepareBindingsForDelete we do the changes, that way we're sure rest of engines will receive the same input as before.

[themsaid]

@abdumu can you check my last comment regarding the method implementation in different grammars?

[abdumu]

withCount is meant for select queries, having counts in delete, update and then trying to remove the counts is absurd !

Since we are using newQueryWithoutScopes in performDeleteOnModel, save, ...
I think the right way to do it, is by setting $this->withCount to [], and re-assign it to the previous value after calling newQueryWithoutScopes.

Also, Sqlite Grammar is ignoring joins completely so no point of the integration tests ! (I might do some work later on this in another PR)

[themsaid]

Closing this since it's a possibly breaking change, please re-submit on master so we can prevent breaking other grammars.

[themsaid]

I've opened a PR with my suggested change in #22285