tpetry / laravel-postgresql-enhanced

Support for many missing PostgreSQL specific features
MIT License
772 stars 31 forks source link

Using newEloquentBuilder in model to extend query builder results in: must be of type Illuminate\Database\ConnectionInterface #97

Closed kdevan closed 2 months ago

kdevan commented 2 months ago

With some code like this in the model:

/**
 * @param  \Illuminate\Database\Query\Builder  $query
 * @return UserQueryBuilder<User>
 */
public function newEloquentBuilder($query): UserQueryBuilder
{
    return new UserQueryBuilder($query);
}

And a custom Query Builder that looks like this:

use Tpetry\PostgresqlEnhanced\Query\Builder;

/**
 * @template TModelClass of User
 *
 * @extends Builder<TModelClass>
 */
class UserQueryBuilder extends Builder {}

I get the error:

"Illuminate\Database\Query\Builder::__construct(): Argument #1 ($connection) must be of type Illuminate\Database\ConnectionInterface, Tpetry\PostgresqlEnhanced\Query\Builder given, called in /app/api/app/Models/User.php on line 263"

So the error is in the model on this line:

return new UserQueryBuilder($query);

If I change the query builder to extend the Laravel Eloquent Query Builder like this it works:

use Illuminate\Database\Eloquent\Builder;

/**
 * @template TModelClass of User
 *
 * @extends Builder<TModelClass>
 */
class UserQueryBuilder extends Builder {}

But then later in actual business logic in a controller or something I get this message:

Call to undefined method Tpetry\PostgresqlEnhanced\Query\Builder::customQueryBuilderFunction()

None of my custom functions in the custom query builder are working because of this error.

This used to work when I had the custom query builder extending the Eloquent Builder before, so that last error is a new one for me from some update, not necessarily this library. But I wonder if you have a recommendation for working with custom query builders? I always wanted to be able to extend your builder as that seemed like the right way to do it, but could never get that working because of the ConnectionInterface error.

Gonna keep digging into this for now, I'll update here if I figure anything out!

kdevan commented 2 months ago

I also can't figure out why the Eloquent Builder even meets the requirement of type Illuminate\Database\ConnectionInterface yet.

tpetry commented 2 months ago

Can you create a simple demo repository I can checkout to research the bug?

kdevan commented 2 months ago

Just a quick update, this ended up happening due to an update to the Adjacency List library updating their types. Reverting back to v1.22 "solves" the case with the Call to undefined method Tpetry\PostgresqlEnhanced\Query\Builder::customQueryBuilderFunction() error.

I have a demo repository set up that I'm digging into to see if I can figure out a way to make this more compatible. It's not obvious to me why the update to types in that library has affected anything and also why I can't just extend the builder in this library. I'll update here as I figure some things out!

tpetry commented 2 months ago

It will be impossible to use both libraries at the same time. The Adjacency list package is using the CTE package by staudenmeier - which is extending Eloquent by replacing internal Laravel logic like my package. And only package can do this at the same time. Sorry, there's nothing we can do to solve this problem.

But someone was able to get it working with the adjacency list package: https://github.com/tpetry/laravel-postgresql-enhanced/issues/42#issuecomment-1574539714. Oh, it was you 😅

kdevan commented 2 months ago

Yeah, that part works :)

Actually one question I have that would be helpful to know, when I'm extending my custom Query Builders using the Eloquent Query Builder, am I still getting the Query Builder from this package? As in, does your Query Builder tie into the framework in a way that something like Eloquent Query Builder is just extending yours? Or by extending my custom Query Builder from the Eloquent one, am I losing the functionality I would be getting from yours?

tpetry commented 2 months ago

You have to extend my query builder instead of the Laravel one ;)

kdevan commented 2 months ago

Ok, that's actually the issue I'm trying to solve here. Your query builder does not work with newEloquentBuilder which is how you add a custom query builder to a model. That's what gets the original error:

Illuminate\Database\Query\Builder::__construct(): Argument #1 ($connection) must be of type Illuminate\Database\ConnectionInterface, Tpetry\PostgresqlEnhanced\Query\Builder given, called in /app/api/app/Models/User.php on line 263

I'm trying to figure out what your query builder needs to pass with the Illuminate\Database\ConnectionInterface type. For some reason the Eloquent Builder passes this. If you know offhand that would be great but otherwise I do plan to continue in my repo to figure this out and i'll share what I figure out here.

tpetry commented 2 months ago

can you make a small demo repository that i can look into this? Actually my QueryBuilder is the same as the Laravel one - just some more stuff added to it.

kdevan commented 2 months ago

Ok, this is from a fresh Laravel install from yesterday: https://github.com/kdevan/pg-enhanced-with-custom-query-builder

I use docker compose up to bring up sail with Laravel and postgres. Then ./vendor/bin/sail test to run the QueryBuilderTest

The User model is set up with a custom query builder that extends the query builder from this library. For this test you should be able to see the Illuminate\Database\ConnectionInterface error.

The Comment model is set up with a custom query builder that extends the eloquent query builder. That test should pass.

tpetry commented 2 months ago

Thanks, I'll dig into that later.

tpetry commented 2 months ago

Ok, I was wrong. You just extend the normal Eloquent Builder from Laravel and everything works. All stuff added by my package is also automatically available to you this way.

I thought I've added a custom eloquent builder - but that was probably an experiment which I had to discard because of compatibility issues.