Can anyone provide an edge case when EloquentBuilder::__call should not return forwardDecoratedCallTo and ditch passthru?

[macropay-solutions]

More details here: #56664

We are thinking of replacing (for our projects) forwardCallTo with forwardDecoratedCallTo in the eloquent builder's _call and make obsolete protected $passthru = [.

forwardDecoratedCallTo will return Eloquent Builder if the return of the function call from Query Builder is the same object as the Query Builder.

    protected function forwardDecoratedCallTo($object, $method, $parameters)
    {
        $result = $this->forwardCallTo($object, $method, $parameters);

        return $result === $object ? $this : $result;
    }

[macropay-solutions]

No one? @shaedrich

[shaedrich]

I'm excited for this change, but I currently can't think of a problem that would arise from that.

[Nabil-nl]

This actually makes sense because the current forwardCallTo will just return the raw result from the Query\Builder. That forces us to rely on $passthru to make sure some methods still return the Eloquent Builder, which feels like a workaround.
By using forwardDecoratedCallTo, we ensure that if the underlying Query\Builder method returns itself, then the Eloquent\Builder can "decorate" that and return $this.
That means methods like where, orderBy, etc. will properly return an Eloquent Builder instance instead of falling back to the base Query Builder. In practice, this removes the need for maintaining the $passthru property and reduces duplication.
So yeah, replacing _call to use forwardDecoratedCallTo seems like a clean improvement it keeps the API consistent and makes the developer experience better (no unexpected return types).

[macropay-solutions]

@Nabil-nl @shaedrich thank you. We merged the change in our framework. Question is, should we do it also in our crud/rest lib suite so whoever uses it can benefit RETROACTIVELY from this change? https://github.com/macropay-solutions/laravel-crud-wizard-free/pull/24/files

[macropay-solutions]

Also

        if (in_array($method, $this->passthru)) {
            return $this->toBase()->{$method}(...$parameters); // toBase is applying scopes
        }

        $this->forwardCallTo($this->query, $method, $parameters); // no scopes

Is that ok?

[macropay-solutions]

Maybe this was the reason for this hack with passthru... We reverted the change in our framework until we find a better solution for this.

[macropay-solutions]

Final thoughts #56664 (comment)

[Nabil-nl]

Yes, I think it’s worth applying the same change in the CRUD/REST suite too. That way users get the benefit retroactively and behavior stays consistent across the ecosystem. As long as it’s backward compatible, it should be a safe improvement.

[macropay-solutions]

@Nabil-nl You missed our latest replies: see #56680 (reply in thread)

[Nabil-nl]

Got it, I just checked your follow-up in #56680 makes sense now why the passthru hack was there in the first place. Reverting until a cleaner solution is found sounds like the right call. I’ll keep an eye on the thread and happy to help test if you explore alternatives.

[macropay-solutions]

@Nabil-nl @shaedrich how about this

This would keep the global scopes applied to the passthru.

[macropay-solutions]

And the backward compatible retroactive fix for it https://github.com/macropay-solutions/laravel-crud-wizard-free/pull/24/files
This will work with both versions of code from the above print screen.

[shaedrich]

Yeah, that should do 👍🏻

[macropay-solutions]

We released 4.3.0 with the retroactive fix