staudenmeir / laravel-cte

Laravel queries with common table expressions
MIT License
514 stars 38 forks source link

Extending query builder with CTE and more #14

Closed chris-rs closed 2 years ago

chris-rs commented 4 years ago

Hi,

This issue is more a question/discussion. I integrated this repo to use the CTE functionality with query builder. Besides CTE I also use some lateral joins. The lateral join clause is not supported by query builder but I created a repo that adds basic support for these type of joins. Now to integrate both CTE and lateral joins in query builder I use these two repo's in a Laravel installation and add a custom service provider. But this service provider has to load a connection, and the connection loads the grammar implementations, and query builder, etc.. I also had to make a new builder and grammar class that inherit both CTE and lateral join logic (by using traits and inheritance).

In other words a lot of work to extend query builder with two 'add-ons' and it is not a very scalable solution.

Do you know about a more elegant way to extend query builder with multiple extensions in separated packages/repos?

Regards, Chris

staudenmeir commented 4 years ago

This is a tricky issue and has come up a few times.

Unfortunately, I don't know of a fundamentally better way to handle this and I don't think there is one. We would basically need multiple inheritance in PHP.

chris-rs commented 4 years ago

Thanks for your response!

chris-rs commented 4 years ago

Just took a fresh look into this and I guess this could be solved a bit more elegant if all Builder and Grammar extensions could be isolated in traits. Then to extend query builder with multiple packages one could create a new custom Builder class (extending from the Illuminate Builder) and import one trait per package. Something like:

class Builder extends Base {
   use BuildsCte, BuildsLateralJoins;
}

Likewise for Grammar classes.

For this package you already did that for the grammar methods. For Builder it is a bit more tricky for this package since it needs a new type of binding. As far as I can see the Builder does not allow to add a new type to the bindings. If Builder allows to add new binding types this could be solved.

Like to know what you think about this extension strategy.

staudenmeir commented 4 years ago

Yes, it's on my TODO list to refactor my packages to simplify this process. Unfortunately, combining multiple packages is still cumbersome, especially when they override the same model/builder/grammar method.

staudenmeir commented 2 years ago

I finally finished refactoring the package and extracting all the Grammar and Builder code into separate traits.

Are you still looking for a solution to use both packages?

chris-rs commented 2 years ago

Yes, thanks for your update! I already implemented a specific Grammar trait for the lateral joins and created a central Grammar class which imports the lateral join grammar and the cue grammar. This central class is used by QueryBuilder. Like to check if your refactoring can result in a more elegant (scalable) approach here.

staudenmeir commented 2 years ago

This is one of the packages I created to merge laravel-cte with another package: https://github.com/staudenmeir/eloquent-eager-limit-x-laravel-cte