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

[10.x] Add database expressions with bindings #46358

Closed
wants to merge 4 commits into from

Conversation

tontonsb
Copy link
Contributor

@tontonsb tontonsb commented Mar 5, 2023

The goal of this PR is to add a safe method to provide non-atomic database expressions as attribute values in Eloquent.

This is the proposed syntax:

$object->location = DB::expression('ST_MakePoint(?, ?)', [56.97, 24.09]);
$object->save();
// executes: update "objects" set "location" = ST_MakePoint(?, ?) where "id" = ?
// with bindings [56.97, 24.09, $object->id]

To accomplish it I've added a new kind of expression that I've named ExpressionWithBindings. I made the new expression class somewhat compatible with the grammar-dependent ideas of #44784 by @tpetry.

While it would be pleasant to add an optional $bindings parameter to DB::raw and a getBindings() method to the existing interface, it would break compatibility with whoever is extending the class.

Instead I've opted to create a completely new interface, a new expression class and an DB::expression helper. In addition to leaving more code unaffected, that should also make people feel less incentivized to try this behaviour in other places (like where and order clauses) where the new tooling is not supported and other tools (like orderByRaw($expr, $bindings)) are already sufficient. Creating a completely new interface and methods also allows to make it all entirely natively typed.

@tontonsb tontonsb changed the title Add database expressions with bindings [10.x] Add database expressions with bindings Mar 5, 2023
@tpetry
Copy link
Contributor

tpetry commented Mar 6, 2023

That your new construct is kind of similar to the existing expressions (DB::raw) but still different as it can't be used in many placed (WHERE, ORDER BY) makes it a very complicated thing to add to Laravel. We would have to educate users that there are two very similar things but for different purposes. Which is not a big problem, but we're also talking about a very niche problem. And having two conflicting solutions for a niche problem is too complicated in my opinion.

Your idea is kind of solved by my original expressions PR for Laravel 9.x: I've added the ability to use quote() within expressions so an expression can transform any value to a database safe literal that can then be embedded into the expression code. And this will work everywhere where expressions work - and no further BC break is needed. I promised @taylorotwell to do it shortly after the 10.x release. I plan to finish tpetry/laravel-query-expressions this week and then build the PR for the next minor 10.x release.

@donnysim
Copy link
Contributor

donnysim commented Mar 6, 2023

From the ways I experimented with the query builder, I kind of wish all expressions carried their own bindings instead of being extracted into one common bindings array like $this->bindings['where']. It would make it so much easier to join builder segments - where, joins etc. where you could just remove the expression without having to worry what bindings were added by it and how to remove them etc..

@tontonsb
Copy link
Contributor Author

tontonsb commented Mar 6, 2023

kind of similar to the existing expressions (DB::raw)

@tpetry true, that could be solved by renaming it to something less similar like DB::insertable or new InsertableExpression (although I'm not a fan of the OOP syntax) to make the supported usecase more obvious.

Or I could work on supporting ExpressionWithBindings in wheres, selects and orderBys and then it would be the thing that's also returned by DB::raw (who'd have an optional $bindings param) and we wouldn't have the two factories 🤔 I think that could be done in a backwards compatible way if ExpressionWithBindings extends Expression and the old Expression is still supported for those who are already extending it or implementing the interface.

@donnysim I think I agree, but that would be a huge rewrite :)

@taylorotwell
Copy link
Member

Not sure I totally love this PR for reasons listed by @tpetry

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.

None yet

4 participants