Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Refactor DELETE query bindings #29165

Merged
merged 1 commit into from
Jul 15, 2019
Merged

[5.8] Refactor DELETE query bindings #29165

merged 1 commit into from
Jul 15, 2019

Conversation

staudenmeir
Copy link
Contributor

We have to remove select bindings from DELETE queries to fix cases like this:

class Post extends Model
{
    protected $withCount = ['comments'];

    public function comments()
    {
        return $this->morphMany(Comment::class, 'commentable');
    }
}

Post::where('id', 1)->delete();

// expected
delete from "posts" where "id" = 1

// actual
delete from "posts" where "id" = 'App\Post'

#22285 fixed this for MySQL, #22298 for SQLite.

#22285 also put join bindings first, but that's not necessary. They always come before where bindings.

This PR removes the unnecessary code and moves the fix up to the parent class. This also fixes queries on PostgreSQL and SQL Server.

Copy link
Contributor

@seriquynh seriquynh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@driesvints
Copy link
Member

Removing the public methods is technically a breaking change. Maybe mark them as deprecate them in this PR and remove them in a separate PR to master?

@staudenmeir
Copy link
Contributor Author

@driesvints I don't see how this could be a breaking change. I'm not removing the method, the parent implementation still exists.

@driesvints
Copy link
Member

@staudenmeir I see. Sorry, I didn't realize that. This isn't indeed isn't a breaking change.

@taylorotwell taylorotwell merged commit ff38578 into laravel:5.8 Jul 15, 2019
@staudenmeir staudenmeir deleted the delete-bindings branch July 15, 2019 14:07
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.

4 participants