Backticks inserted into wrong place when executing DB::raw() expressions alongside ->withSum(...)

[morganarnel]

Laravel Version

10.42.0

PHP Version

8.2.12

Database Driver & Version

MySQL 8.2.0 on linux/arm64

Description

Recently upgraded to Laravel 10 from 9 - have substituted our DB::raw() expressions with the new (string) DB::raw(...)->getValue(DB::connection()->getQueryGrammar()) as directed in the upgrade guide because the ->withSum() no longer accepts an expression (presumably down to the loss of string casting).

As you can see from the above screenshot the expression has been grossly misinterpreted and the backticks have been placed in some pretty wacky places. Adding the backticks myself didn't work.

Here's a snippet of one of the offending withSum lines

$query->withSum([
            'revenueEntryLineItems as actual_revenue_gross' => function ($query) {
                $query->leftJoin('tax_rates', 'revenue_entry_line_items.tax_rate_id', 'tax_rates.id');
             },
        ], (string) DB::raw('((revenue_entry_line_items.unit_price * revenue_entry_line_items.quantity) / 100) * (100 + COALESCE(tax_rates.rate, 0))')->getValue(DB::connection()->getQueryGrammar()));

Steps To Reproduce

Try a funky expression in a withSum

[morganarnel]

Update: This also happens when avoiding DB::raw entirely and passing an expression as a string

[driesvints]

From the upgrade guide:

Casting an expression to a string using (string) is no longer supported.

I'm sorry, but we no longer support casting to a string. Did you already try just using the DB::raw() call without the type cast?

[morganarnel]

@driesvints The string cast is just to make phpstan as happy as you are at hitting that close as completed button because the ->getValue(...) returns a string|int|float, however in this scenario this is always going to be a string which the second parameter of ->withSum() only accepts.

The issue here is that the wrapping is being performed incorrectly.

[driesvints]

as happy as you are at hitting that close as completed button

I don't really appreciate the passive/aggressive tone here. Feel free to attempt a PR to the docs or the framework if you want.

[morganarnel]

as happy as you are at hitting that close as completed button

I don't really appreciate the passive/aggressive tone here. Feel free to attempt a PR to the docs or the framework if you want.

No worries will do, thanks for the help 👍

[john-dent]

@driesvints

I can see the issue here, the wrap function in Grammar.php does a check to see if the value is an expression, however the changes implemented in #44784 (removing the __toString() method) have broken the ability for this check to work (at least with the relation aggregate methods) as we can no longer pass an expression through to this.

[driesvints]

@tpetry could you wage in here maybe?

[tpetry]

Its probably a missing expression handling somewhere in withSum. I'll look into it today and build a fix.

[driesvints]

Thanks @tpetry

[tpetry]

I've verified the issue and started working on a fix. The aggregate functions hadn't been documented to support expressions.

[john-dent]

Thank you both!

@tpetry, a quick question if you don't mind (this could be me misunderstanding):

This line

framework/src/Illuminate/Database/Query/Expression.php

Line 34 in af2a7ca

public function getValue(Grammar $grammar)

(and in the associated interface) forcibly requires that an instance of Grammar be passed through yet I can see nowhere that it is actually used or required? Without forcibly requiring this then we would be able to re-implement the __toString() method which would fully resolve this issue and I believe would not break your original PR?

As my colleague @morganarnel has pointed out to me this morning, we believe this issue may extend further than just aggregate usages and in fact may impact any DB::raw() instance as we are forcing this to be a string, so the wrapping function will fail on any complex query.

[tpetry]

The __toString method can not be implemented again. The Grammar is passed to the expression. The standard expression does not use it but special expressions like tpetry/laravel-query-expressions use it to generate database-specific output. That was the whole intention of the Grammar rewrite. So expressions can abstract the different syntax or functionality of databases into reusable components.

As my colleague @morganarnel has pointed out to me this morning, we believe this issue may extend further than just aggregate usages and in fact may impact any DB::raw() instance as we are forcing this to be a string, so the wrapping function will fail on any complex query.

The Laravel core has been changed to handle DB::raw() correctly where it was documented that you can provide an expression instead of a string. But not every method in Laravel had been correctly documented.

[john-dent]

@tpetry thank you!

I understand now (I forgot your Expression extensions for a moment there..), greatly appreciate you working on a fix for this! 🙂

[tpetry]

@morganarnel @john-dent PR #49912 will fix this issue