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

V2 Upgrade Guide - macro removal #328

Closed
ce-brex opened this issue Sep 10, 2019 · 3 comments
Closed

V2 Upgrade Guide - macro removal #328

ce-brex opened this issue Sep 10, 2019 · 3 comments
Labels
enhancement New feature or request

Comments

@ce-brex
Copy link
Contributor

ce-brex commented Sep 10, 2019

In the upgrade guide, it mentions replacing all macros calls to using the methods on the QueryBuilderRequest class. The examples don't work as they don't have the proper request object. There are a couple of possible ways to do this however.

  1. Change app(QueryBuilderRequest::class)->includes() to something like app(QueryBuilderRequest::class)->fromRequest(app('request'))->includes()

  2. Inform the user to add a binding for QueryBuilderRequest in AppServiceProvider::register()

$this->app->bind(QueryBuilderServiceProvider::class, function ($app) {
    return QueryBuilderRequest::fromRequest($app['request']);
})
  1. Update the QueryBuilderServiceProvider to include a register method and the binding from above.

The last two offer the ability to also inject the QueryBuilderRequest instead of the Laravel Request class in to controllers.

A note on the guide that these methods do not accept arguments would also be helpful.

@AlexVanderbist
Copy link
Member

You're right! I'll add some info and a link to this issue in the upgrade guide. I didn't feel like having adding the request class to the service container as it didn't have much use in the package but I can see this being valueable in the context of a controller.

On the method arguments: the macros were deprecated a while ago and it's easy enough to call contains on any of the QueryBuilderRequest methods. I'll add a note for that too.

Thank you!

@klimov-paul
Copy link
Contributor

At the current 'master' code, binding for QueryBuilderRequest is already present:
https://github.com/spatie/laravel-query-builder/blob/master/src/QueryBuilderServiceProvider.php#L22-L24

And examples like:

app(QueryBuilderRequest::class)->includes();

works correctly.

See e66471c

@klimov-paul
Copy link
Contributor

I suppose this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants